Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type attribute to search query record #157

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

deshsidd
Copy link
Collaborator

@deshsidd deshsidd commented Nov 13, 2024

Description

Add group_by attribute to search query record to differentiate between query and group.

group_by = none for type = query

This is needed when displaying queries and groups together on the QI dashboards as part of the Top N query groups feature.

Note:

  • group_by will now be part of the API response
  • group_by will be stored in local index as part of historical top N data
  • group_by will be displayed on the Top N dashboards on the details page. type will be inferred from group_by and we will be able to filter the table using type. type will have values query or group.

Added unit tests to cover this change.

Issues Resolved

Addresses: opensearch-project/query-insights-dashboards#14

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>
@dzane17
Copy link
Collaborator

dzane17 commented Nov 15, 2024

Overall, it looks good. My only question is whether the "group_by" type will be displayed on the dashboard for grouped entries. Currently, we only have similarity, but if we add other grouping mechanisms in the future, the dashboard should clearly indicate the type of grouping. For example, it should distinguish whether the group is related by similarity or by some other criteria. In that case, we could use more descriptive labels like single_query, similarity_group, or other_group.

Another consideration is whether we’re only distinguishing between two options—individual queries and groups. If that’s the case, a boolean flag might be sufficient instead of using a string. Alternatively, if we need to handle groups differently on the dashboard, we could use an explicit flag, like:

attributes.put("isGroup", true);

@deshsidd
Copy link
Collaborator Author

Overall, it looks good. My only question is whether the "group_by" type will be displayed on the dashboard for grouped entries.

Yes, we will aim to display this on the dashboard.

Another consideration is whether we’re only distinguishing between two options—individual queries and groups.

IMO Would be good to explicitly mention group and query since this field will be pushed with the record to the local index and used to display on the dashboard directly.

@dzane17
Copy link
Collaborator

dzane17 commented Nov 15, 2024

I think we should make the labels here more descriptive so the dashboard can use them directly without any translation.

If we only have group & query options, the dashboard will need to append some info to indicate that group records are similarity groups. Also when another group type is added, group & query labels will not be sufficient.

@deshsidd
Copy link
Collaborator Author

I think we should make the labels here more descriptive so the dashboard can use them directly without any translation.

If we only have group & query options, the dashboard will need to append some info to indicate that group records are similarity groups. Also when another group type is added, group & query labels will not be sufficient.

This change is not to indicate the grouping type. the grouping type is already stored and can be retrieved from the cluster settings on the UI. This changes is to denote whether the searchqueryrecord entry (in local index or dashboard) is a group entry or a query entry. This will be useful while querying and filtering.

@deshsidd
Copy link
Collaborator Author

@dzane17 Please take a look at these UX screens for more details : opensearch-project/query-insights-dashboards#14

@ansjcy
Copy link
Member

ansjcy commented Nov 15, 2024

I'm fine with either displaying only "group" or more detailed "group by " on the overview page. IMO the pro of adding the dimensions here is it can give us extra information thus I think it could be a better way.

Let's check with the ux team on this as well. They should be able to give us suggestions from the end user's experience perspective.

@dzane17
Copy link
Collaborator

dzane17 commented Nov 15, 2024

Cluster settings will only tell us the current group_by setting. We don't know the value for historical data.

@deshsidd
Copy link
Collaborator Author

Cluster settings will only tell us the current group_by setting. We don't know the value for historical data.

Yes my bad, I thought we can get this from the measurements object where aggregation type will give us NONE or AVERAGE. However, this is not extensible if we add more grouping mechanisms in the future. Would be good to store the following:

type: group/query
group_by: similarity/none/tenant-id

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>
Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>
@ansjcy
Copy link
Member

ansjcy commented Nov 18, 2024

LGTM, merging the PR now!

@ansjcy ansjcy merged commit b5e2d07 into opensearch-project:main Nov 18, 2024
15 of 16 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 18, 2024
* Add type attribute to search query record

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>

* Make type attribute enum

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>

* Use group_by attribute

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>

---------

Signed-off-by: Siddhant Deshmukh <deshsid@amazon.com>
(cherry picked from commit b5e2d07)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants