Query grouping dashboard changes and extensive tests #33

deshsidd · 2024-12-23T08:36:12Z

Description

Query grouping dashboard changes based on the proposed UX. Support grouping related configurations setting and viewing query groups using the query insights dashboards.

Currently, dashboards only supports Top N queries. With the introduction of grouping Top N queries by similarity, this PR aims to integrate query insights dashboard with the query grouping changes.

Change Details:

Modify the Top N queries page to now display groups
Add a filter type to filter out groups or queries
Create a new Query Group Details page which displays additional information regarding the query group
The query group details page will have a section displaying aggregate statistics followed by another section to display details for a sample query from the group.
Added extensive tests

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].
Closes : #14

Screenshots

Top N queries/groups page displaying multiple query groups:

Option to filter by group/query:

Query Group Details Page after clicking on the query group row in the Top N page:

Sample Query Details section on the query group details page:

Query Configuration Page to configure Top N Query Groups:

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.

public/pages/QueryGroupDetails/QueryGroupDetails.tsx

public/pages/QueryInsights/QueryInsights.tsx

types/types.ts

derek-ho · 2024-12-26T17:45:34Z

Description

Query grouping dashboard changes based on the proposed UX. Support grouping related configurations setting and viewing query groups using the query insights dashboards.

Currently, dashboards only supports Top N queries. With the introduction of grouping Top N queries by similarity, this PR aims to integrate query insights dashboard with the query grouping changes.

Change Details:

Modify the Top N queries page to now display groups

Add a filter type to filter out groups or queries

Create a new Query Group Details page which displays additional information regarding the query group

The query group details page will have a section displaying aggregate statistics followed by another section to display details for a sample query from the group.

Added extensive tests

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...]. Closes : #14

Screenshots

Top N queries/groups page displaying multiple query groups: Option to filter by group/query: Query Group Details Page after clicking on the query group row in the Top N page: Sample Query Details section on the query group details page: Query Configuration Page to configure Top N Query Groups:

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.

Maybe I am not fully understanding the group-by feature, but if you do not do group_by on the table, shouldn't it show all of the single queries by themselves? Or does group-by have a different meaning in terms of the data?

ansjcy

@derek-ho Could you please also take a look?

Note after we add the cypress tests, we need to follow up on this PR to add integ tests for query group details page and also all other related changes as well.

ansjcy · 2025-01-10T18:23:08Z

package.json

@@ -45,7 +45,7 @@
    "@types/react-dom": "^16.9.8",
    "@types/object-hash": "^3.0.0",
    "@types/react-router-dom": "^5.3.2",
-    "cypress": "9.5.4",
+    "cypress": "12.17.4",


we are not adding cypress tests, why is this change needed?

Getting error that version is not matching the version in OSD without this change

You can add single-version=loose flag as true: https://github.com/opensearch-project/dashboards-observability/blob/453cca820efec085eeb922e0b200d63fa6207880/.github/workflows/lint.yml#L49

But there is also no harm that I can see in bumping cypress version

We need to update the package lock file if we want to bump up the version. Let's rebase the main branch to pick up the cypress version bump.

public/components/app.test.tsx

public/pages/Configuration/Configuration.tsx

public/pages/QueryDetails/Components/QuerySummary.tsx

public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx

public/pages/QueryInsights/QueryInsights.tsx

ansjcy · 2025-01-10T18:49:07Z

but if you do not do group_by on the table, shouldn't it show all of the single queries by themselves?

@derek-ho , group_by is a backend feature, (see https://opensearch.org/docs/latest/observing-your-data/query-insights/grouping-top-n-queries/), if group_by is enabled by similarity, it means when we collect the top n queries, all similar queries will be grouped into the same group. Since there could be a lot of queries in one group, we only keep one sample of the queries in the group.

ansjcy · 2025-01-10T18:53:28Z

Also please avoid creating branches in the original repo, follow the best practices in OpenSearch-Dashboards developer guide: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/DEVELOPER_GUIDE.md#fork-and-clone-opensearch-dashboards

...All local development should be done in a forked repository....

public/pages/QueryInsights/QueryInsights.tsx

ansjcy · 2025-01-10T19:04:36Z

I tried sorting on the timestamp field, it doesn't looks correct to me:

public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx

ansjcy · 2025-01-10T19:29:56Z

Also please wait until this PR is merged: opensearch-project/query-insights#184
And test the integration with the exporter. See if the UI can correctly display and do filter by date correctly for the group records.

derek-ho · 2025-01-13T21:22:03Z

public/pages/QueryInsights/QueryInsights.tsx

-      render: (indices: string[]) => Array.from(new Set(indices.flat())).join(', '),
+      render: (indices: string[], query: SearchQueryRecord) => {
+        const isSimilarity = query.group_by === 'SIMILARITY';
+        return isSimilarity ? '-' : Array.from(new Set(indices.flat())).join(', ');


Why are we not showing the indices of the group here?

The group only contains indices of the first query in the group. Since displaying this information might be misleading we decided to not display this here after consulting with UX.

derek-ho · 2025-01-13T21:23:50Z

public/pages/QueryInsights/QueryInsights.tsx

+      render: (nodeId: string, query: SearchQueryRecord) => {
+        const isSimilarity = query.group_by === 'SIMILARITY';
+        return isSimilarity ? '-' : nodeId;
+      },
      sortable: true,
      truncateText: true,
    },
    {
      field: TOTAL_SHARDS_FIELD,
      name: TOTAL_SHARDS,
+      render: (totalShards: number, query: SearchQueryRecord) => {
+        const isSimilarity = query.group_by === 'SIMILARITY';
+        return isSimilarity ? '-' : totalShards;
+      },


Some of these don't seem to make sense if looking at group_by. Should we remove some columns from the table depending on the type you are looking at? Which brings up another question, I wasn't able to get to a situation where both group and single queries show up. If it is only one or the other, IMO it is confusing to put type filter - it should just be one or the other based on your configuration.

Only if the user just switched from single query to group (or vice versa), the type parameter would be useful.

I think it makes sense to change the table column based on the data. A lot of the columns can be hidden if there's only one type of data.

public/pages/QueryGroupDetails/QueryGroupDetails.tsx

derek-ho

Some minor implementation concerns but nothing blocking - they can be addressed in follow ups. I am also not loving re-creating a component for the possibility they will diverge in the future, but if that is a UX call then let's go with it.

ansjcy · 2025-01-13T21:46:03Z

re-creating a component for the possibility they will diverge in the future

+1. I still think we should reuse the previous component. Splitting it out to a separate page would make it harder to maintain - for example, updating the chart library etc - Or you can abstract all the "sample query panels" into one component, and reuse it across query details page and sample queries page

ansjcy · 2025-01-13T21:47:03Z

Also please add e2e tests for the grouping features as well.

This reverts commit 9efdbaf. Signed-off-by: Siddhant Deshmukh <[email protected]>

Signed-off-by: Siddhant Deshmukh <[email protected]>

deshsidd · 2025-01-15T20:14:48Z

Also please add e2e tests for the grouping features as well.

Will do this as a followup

deshsidd · 2025-01-15T20:17:17Z

Some minor implementation concerns but nothing blocking - they can be addressed in follow ups. I am also not loving re-creating a component for the possibility they will diverge in the future, but if that is a UX call then let's go with it.

Discussed this specific point with Kevin and UX before implementation and they recommended creating a new component. Keeping this in mind lets keep it as is for now and take a call in the future based on how the pages evolve.

opensearch-trigger-bot · 2025-01-16T00:22:51Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/query-insights-dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/query-insights-dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-33-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 67fd811437662e9b69ff5943440e2d5ec489d476
# Push it to GitHub
git push --set-upstream origin backport/backport-33-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/query-insights-dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-33-to-2.x.

…ect#33) * Revert "remove records with grouping (opensearch-project#26)" This reverts commit 9efdbaf. Signed-off-by: Siddhant Deshmukh <[email protected]> * Query grouping dashboard changes and extensive tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Fix tests and linting Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments and update tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Update cypress tests, hashcode to id, refactoring Signed-off-by: Siddhant Deshmukh <[email protected]> * Lint Signed-off-by: Siddhant Deshmukh <[email protected]> --------- Signed-off-by: Siddhant Deshmukh <[email protected]>

* Revert "remove records with grouping (#26)" This reverts commit 9efdbaf. * Query grouping dashboard changes and extensive tests * Fix tests and linting * Address review comments * Address review comments and update tests * Update cypress tests, hashcode to id, refactoring * Lint --------- Signed-off-by: Siddhant Deshmukh <[email protected]> Co-authored-by: Siddhant Deshmukh <[email protected]>

deshsidd requested review from ansjcy, jainankitk and dzane17 as code owners December 23, 2024 08:36

ansjcy reviewed Dec 23, 2024

View reviewed changes

public/pages/QueryGroupDetails/QueryGroupDetails.tsx Outdated Show resolved Hide resolved

public/pages/QueryInsights/QueryInsights.tsx Outdated Show resolved Hide resolved

public/pages/QueryInsights/QueryInsights.tsx Outdated Show resolved Hide resolved

derek-ho reviewed Dec 26, 2024

View reviewed changes

types/types.ts Outdated Show resolved Hide resolved

deshsidd force-pushed the sid/query-grouping branch 4 times, most recently from 480693b to 7336988 Compare January 8, 2025 20:16

deshsidd mentioned this pull request Jan 8, 2025

[FEATURE] Add an ID Field to Search Query Records opensearch-project/query-insights#159

Closed

ansjcy reviewed Jan 10, 2025

View reviewed changes

public/pages/QueryInsights/QueryInsights.tsx Outdated Show resolved Hide resolved

ansjcy reviewed Jan 10, 2025

View reviewed changes

public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx Outdated Show resolved Hide resolved

public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx Outdated Show resolved Hide resolved

derek-ho reviewed Jan 13, 2025

View reviewed changes

public/pages/QueryGroupDetails/QueryGroupDetails.tsx Show resolved Hide resolved

derek-ho reviewed Jan 13, 2025

View reviewed changes

public/pages/QueryGroupDetails/QueryGroupDetails.tsx Show resolved Hide resolved

derek-ho approved these changes Jan 13, 2025

View reviewed changes

deshsidd added 5 commits January 14, 2025 11:04

Revert "remove records with grouping (#26)"

6d748b7

This reverts commit 9efdbaf. Signed-off-by: Siddhant Deshmukh <[email protected]>

Query grouping dashboard changes and extensive tests

ce60cde

Signed-off-by: Siddhant Deshmukh <[email protected]>

Fix tests and linting

09c722b

Signed-off-by: Siddhant Deshmukh <[email protected]>

Address review comments

e9431b0

Signed-off-by: Siddhant Deshmukh <[email protected]>

Address review comments and update tests

6312c3d

Signed-off-by: Siddhant Deshmukh <[email protected]>

deshsidd force-pushed the sid/query-grouping branch from 956c0ff to 6312c3d Compare January 14, 2025 21:01

deshsidd added 2 commits January 15, 2025 12:04

Update cypress tests, hashcode to id, refactoring

10793f1

Signed-off-by: Siddhant Deshmukh <[email protected]>

Lint

e0e1f24

Signed-off-by: Siddhant Deshmukh <[email protected]>

ansjcy approved these changes Jan 16, 2025

View reviewed changes

ansjcy merged commit 67fd811 into main Jan 16, 2025
9 checks passed

ansjcy added the backport 2.x label Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query grouping dashboard changes and extensive tests #33

Query grouping dashboard changes and extensive tests #33

deshsidd commented Dec 23, 2024

derek-ho commented Dec 26, 2024

Description

Issues Resolved

Screenshots

ansjcy left a comment

ansjcy Jan 10, 2025

deshsidd Jan 10, 2025

derek-ho Jan 13, 2025

derek-ho Jan 13, 2025

ansjcy Jan 13, 2025

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

derek-ho Jan 13, 2025

deshsidd Jan 14, 2025

derek-ho Jan 13, 2025

ansjcy Jan 13, 2025 •

edited

Loading

derek-ho left a comment •

edited

Loading

ansjcy commented Jan 13, 2025

ansjcy commented Jan 13, 2025

deshsidd commented Jan 15, 2025

deshsidd commented Jan 15, 2025

opensearch-trigger-bot bot commented Jan 16, 2025

Query grouping dashboard changes and extensive tests #33

Query grouping dashboard changes and extensive tests #33

Conversation

deshsidd commented Dec 23, 2024

Description

Issues Resolved

Screenshots

derek-ho commented Dec 26, 2024

Description

Issues Resolved

Screenshots

ansjcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

ansjcy commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansjcy Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

derek-ho left a comment • edited Loading

Choose a reason for hiding this comment

ansjcy commented Jan 13, 2025

ansjcy commented Jan 13, 2025

deshsidd commented Jan 15, 2025

deshsidd commented Jan 15, 2025

opensearch-trigger-bot bot commented Jan 16, 2025

ansjcy Jan 13, 2025 •

edited

Loading

derek-ho left a comment •

edited

Loading