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.

Sign up for GitHub

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

Jump to bottom

Fix dynamically display columns #103

Open

KishoreKicha14 wants to merge 11 commits into opensearch-project:main from KishoreKicha14:Fix-Dynamically-display-columns

Contributor

KishoreKicha14 commented Feb 7, 2025 •

edited

Loading

Screenshot 2025-02-19 at 5 40 48 PM

Screenshot 2025-02-19 at 10 50 36 PM

Screenshot 2025-02-19 at 10 50 44 PM

Screenshot 2025-02-19 at 10 50 51 PM

Screenshot 2025-02-19 at 10 50 59 PM

Screenshot 2025-02-19 at 10 51 37 PM

Screenshot 2025-02-19 at 10 51 48 PM

Description

Problem:
The Top N queries table can display:

Only queries
Only groups
Combination of queries/groups
Some columns are applicable to only queries (eg: indices) and others to only groups (eg: query count).

Table should dynamically display the applicable columns based on the data in the table and only display the applicable columns.

This PR ensures that even when no selection is made and all query types are groups, the applicable columns are dynamically displayed.

Issues Resolved

Closes: [#45]

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.

Kishore Kumaar Natarajan added 2 commits

February 7, 2025 01:47


          Fix-Dynamically-display-columns

c0d7f88

Signed-off-by: Kishore Kumaar Natarajan <kkamazonn@amazon.com>


          Fix-Dynamically-display-columns-correction

b0fbe2f

Signed-off-by: Kishore Kumaar Natarajan <kkamazonn@amazon.com>

KishoreKicha14 requested review from ansjcy, jainankitk, deshsidd and dzane17 as code owners

February 7, 2025 10:01


          Fix-Dynamically-display-columns-snaps

9c6465b

Signed-off-by: Kishore Kumaar Natarajan <kkamazonn@amazon.com>

Collaborator

deshsidd commented Feb 7, 2025

Why are the latency, cpu and memory columns displayed twice in each screenshot? @KishoreKicha14

Collaborator

deshsidd commented Feb 7, 2025

If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this?

ansjcy reviewed

View reviewed changes

Member

ansjcy left a comment •

edited

Loading

Other than the issues @deshsidd mentioned above, the rest of the logic looks good to me!
@KishoreKicha14 Thanks for the change, could you also add unit tests and cypress tests in this PR as well?

public/pages/QueryInsights/QueryInsights.tsx Outdated

@@ @@ -259,6 +275,15 @@ const QueryInsights = ({ @@
                     truncateText: true,
                   },
                 ];
+                const columnsToShow = selectedFilter === 'SIMILARITY' ? groupColumns : queryColumns;
+                const filteredQueries = queries.filter((query) => {

Member

ansjcy Feb 7, 2025

How about

const filteredQueries = queries.filter(query => 
  !selectedFilter || query.group_by === selectedFilter
);

?

Contributor Author

KishoreKicha14 Feb 20, 2025

Updated

Contributor Author

KishoreKicha14 commented Feb 7, 2025 •

edited

Loading

@deshsidd @ansjcy Updated the screenshots.

Kishore Kumaar Natarajan added 2 commits

February 12, 2025 10:26


          Updated the Unit test and Cypress test

d279922

Signed-off-by: Kishore Kumaar Natarajan <kkamazonn@amazon.com>


          Updated the Unit test and Cypress test

c201aec

Signed-off-by: Kishore Kumaar Natarajan <kkamazonn@amazon.com>

ansjcy reviewed

View reviewed changes

cypress/e2e/1_top_queries.cy.js Outdated

@@ @@ -22,7 +22,7 @@ const clearAll = () => { @@
               };
               describe('Query Insights Dashboard', () => {
-                // Setup before each test
+                // // Setup before each test

Member

ansjcy Feb 13, 2025

please remove

Contributor Author

KishoreKicha14 Feb 20, 2025

Removed

cypress/e2e/1_top_queries.cy.js Outdated

+                    cy.contains('.euiTableHeaderCell', header).should('exist');
+                  });
+                });
+                beforeEach(() => {

Member

ansjcy Feb 13, 2025

Why do we need beforeEach here? this looks identical to

query-insights-dashboards/cypress/e2e/1_top_queries.cy.js

Line 26 in c201aec

beforeEach(() => {

Contributor Author

KishoreKicha14 Feb 20, 2025

Removed

cypress/e2e/1_top_queries.cy.js Outdated

+                  cy.get('.euiFilterSelectItem').contains('group').click();
+                  // Wait for table update
+                  cy.get('.euiTableHeaderCell').should('have.length.lessThan', 20);

Member

ansjcy Feb 13, 2025

Why dont we assert a fixed length here? The number of headers in each scenarios should be deterministic.

Contributor Author

KishoreKicha14 Feb 20, 2025

changed to fixed length

cypress/e2e/1_top_queries.cy.js Outdated

+                  ];
+                  // Wait for the table to load by checking if headers are visible
+                  cy.get('.euiTableHeaderCell').should('have.length.greaterThan', 6);

Member

ansjcy Feb 13, 2025

why greater then instead of equals?

Contributor Author

KishoreKicha14 Feb 20, 2025

changed to fixed length

public/pages/QueryInsights/QueryInsights.test.tsx Outdated

+                  await waitFor(() => expect(screen.getByRole('table')).toBeInTheDocument());
+                  const headers = await waitFor(() => screen.getAllByRole('columnheader', { hidden: false }));
+                  console.log(

Member

ansjcy Feb 13, 2025

Please remove the console log.

Contributor Author

KishoreKicha14 Feb 20, 2025

removed

public/pages/QueryInsights/QueryInsights.test.tsx Outdated

+                  const expectedHeaders = ['ID', 'Type', 'Query Count', 'Latency', 'CPU Time', 'Memory Usage'];
+                  expectedHeaders.forEach((header) => {
+                    expect(
+                      headers.some((h) => h.textContent?.trim().toLowerCase() === header.toLowerCase())

Member

ansjcy Feb 13, 2025

why headers.some?
we should also compare the actual value without ignoring the case.

Contributor Author

KishoreKicha14 Feb 20, 2025

modified

public/pages/QueryInsights/QueryInsights.test.tsx Outdated

+                  renderQueryInsights();
+                  await waitFor(() => expect(screen.getByRole('table')).toBeInTheDocument());
+                  const typeElements = screen.getAllByText('Type');
+                  const typeFilterButton = typeElements.find((el) => el.closest('button')); // Ensure it's a button

Member

ansjcy Feb 13, 2025

It is a button. why use closest?

Contributor Author

KishoreKicha14 Feb 20, 2025

removed

public/pages/QueryInsights/QueryInsights.test.tsx Outdated

+                  await waitFor(() => {
+                    expectedHeaders.forEach(async (header) => {
+                      expect(await screen.findByText(header)).toBeInTheDocument();

Member

ansjcy Feb 13, 2025

instead of finding on the whole screen and assert to be in document, could you narrow down the test to that specific table?

Contributor Author

KishoreKicha14 Feb 20, 2025

modified

public/pages/QueryInsights/QueryInsights.test.tsx Outdated

+                  ];
+                  await waitFor(() => {
+                    expectedHeaders.forEach(async (header) => {

Member

ansjcy Feb 13, 2025

Also, the sequence matters here. let's also test the header sequence.

Contributor Author

KishoreKicha14 Feb 20, 2025

modified checking sequence as well

cypress/e2e/1_top_queries.cy.js Outdated

+                  // Verify each expected header exists
+                  expectedHeaders.forEach((header) => {
+                    cy.contains('.euiTableHeaderCell', header).should('exist');

Member

ansjcy Feb 13, 2025

The sequence matters here. let's also test the header sequence.

Contributor Author

KishoreKicha14 Feb 20, 2025

modified checking sequence as well

kishore.nat98@gmail.com added 4 commits

February 18, 2025 16:51


          updated cypress test

a61a55e

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>


          updated QueryInsights.tsx

d46d826

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>


          updated unit test and cypress- corrections

da33167

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>


          updated QueryInsights - Lint

0618fc7

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>

Collaborator

deshsidd commented Feb 19, 2025 •

edited

Loading

Cypress tests failing, please take a look
(Retried and looks like it was a transient issue on windows)

Collaborator

deshsidd commented Feb 19, 2025 •

edited

Loading

If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this?

@KishoreKicha14 Looks like this change might not be addressed? Please update the screenshots based on the latest code if these have been addressed. Thanks

Contributor Author

KishoreKicha14 commented Feb 19, 2025

If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this?

@KishoreKicha14 Looks like this change might not be addressed? Please update the screenshots based on the latest code if these have been addressed. Thanks

Should we have separate columns for average latency and average CPU, or should the column names dynamically change? The challenge arises when both group and query are selected. How should we handle this?

Collaborator

deshsidd commented Feb 19, 2025

If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this?

@KishoreKicha14 Looks like this change might not be addressed? Please update the screenshots based on the latest code if these have been addressed. Thanks

Should we have separate columns for average latency and average CPU, or should the column names dynamically change? The challenge arises when both group and query are selected. How should we handle this?

column names can dynamically change when only groups data is in the table.

Contributor Author

KishoreKicha14 commented Feb 19, 2025

If it’s not too big a change, we can debate about changing the column names to average latency, average cpu…… and so on for only groups? What do others think about this?

@KishoreKicha14 Looks like this change might not be addressed? Please update the screenshots based on the latest code if these have been addressed. Thanks

Should we have separate columns for average latency and average CPU, or should the column names dynamically change? The challenge arises when both group and query are selected. How should we handle this?

column names can dynamically change when only groups data is in the table.

discussed this with @ansjcy , and he mentioned regarding column naming:

Only groups selected → Show averaged column names (e.g., "Average Latency," "Average CPU").
Only queries selected → Show raw column names (e.g., "Latency," "CPU Usage") and remove the query count.
Both groups and queries selected → Use combined column names like "Latency / Avg Latency" and include the query count.
@deshsidd Any thoughts on this approach?

Collaborator

deshsidd commented Feb 19, 2025

Only groups selected → Show averaged column names (e.g., "Average Latency," "Average CPU"). Only queries selected → Show raw column names (e.g., "Latency," "CPU Usage") and remove the query count. Both groups and queries selected → Use combined column names like "Latency / Avg Latency" and include the query count. @deshsidd Any thoughts on this approach?

For both can use the same approach we have today, i.e Latency, etc.

kishore.nat98@gmail.com added 2 commits

February 20, 2025 00:41


          dynamically changing column name

656b9e2

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>


          dynamically changing column name - lint

4e863f0

Signed-off-by: kishore.nat98@gmail.com <kkamazonn@amazon.com>

Contributor Author

KishoreKicha14 commented Feb 20, 2025

Discussed with UX offline, we agreed to use:
Only groups are selected → Shows averaged value for metric (ie column name : Average Latency) and include query count
Only queries are selected → Shows raw value for metric (ie column name : Latency) and exclude the query count
Both groups and queries are selected → Shows averaged and non averaged values for metric ( ie column name: Avg Latency/ Latency) and include query count

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ansjcy ansjcy left review comments

jainankitk Awaiting requested review from jainankitk jainankitk is a code owner

deshsidd Awaiting requested review from deshsidd deshsidd is a code owner

dzane17 Awaiting requested review from dzane17 dzane17 is a code owner

At least 1 approving review is required to merge this pull request.

Labels

None yet