Traces - Update custom source toast/error/sorting #2407

TackAdam · 2025-04-08T20:35:22Z

Description

Change all "Trace ID" text for tables to "Trace Id"
Add empty/error state for trace view and service view.
Remove sorting from fields that didn't make sense such as hashed values { Span Id, Trace Id, Trace Group, ect.. }
Adjust "... Load more" based on UX feedback, maintain sorting and page on query. Only show load more option when there is more to load and user is on the last page.
Add handling for custom traces source when there are less than "maxTraces"
Adjust right hand indicator to reflect the number of root spans on the traces page accurately.

"Trace Id" Change:

Traces error state:

TraceError.mov

Services error state:

ServiceError.mov

Sorting removed from hash values:

Load More:

Screen.Recording.2025-04-14.at.3.55.49.PM.mov

Toast for bucket exception:

Screen.Recording.2025-04-14.at.3.53.45.PM.mov

Adjust right hand indicator to reflect the number of root spans on the traces page accurately.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

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: Adam Tackett <[email protected]>

...omponents/trace_analytics/components/common/shared_components/component_helper_functions.tsx

public/components/trace_analytics/components/common/shared_components/custom_datagrid.tsx

Signed-off-by: Adam Tackett <[email protected]>

public/components/trace_analytics/components/services/service_view.tsx

Signed-off-by: Adam Tackett <[email protected]>

codecov · 2025-04-15T23:45:36Z

Codecov Report

Attention: Patch coverage is 31.03448% with 100 lines in your changes missing coverage. Please review.

Project coverage is 55.91%. Comparing base (5cfbba1) to head (e07a5c2).
Report is 42 commits behind head on main.

Files with missing lines	Patch %	Lines
...ace_analytics/components/services/service_view.tsx	36.20%	34 Missing and 3 partials ⚠️
...n/shared_components/component_helper_functions.tsx	15.00%	17 Missing ⚠️
...s/trace_analytics/components/traces/trace_view.tsx	18.75%	13 Missing ⚠️
...nents/common/shared_components/custom_datagrid.tsx	33.33%	10 Missing ⚠️
...ace_analytics/requests/services_request_handler.ts	10.00%	9 Missing ⚠️
...trace_analytics/requests/traces_request_handler.ts	33.33%	8 Missing ⚠️
...ace_analytics/components/traces/traces_content.tsx	58.33%	5 Missing ⚠️
...ace_analytics/requests/queries/services_queries.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2407      +/-   ##
==========================================
- Coverage   56.53%   55.91%   -0.62%     
==========================================
  Files         393      398       +5     
  Lines       15574    16149     +575     
  Branches     4284     4524     +240     
==========================================
+ Hits         8804     9030     +226     
- Misses       6705     7047     +342     
- Partials       65       72       +7

Flag	Coverage Δ
dashboards-observability	`55.91% <31.03%> (-0.62%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ps48 · 2025-04-16T21:49:48Z

public/components/trace_analytics/requests/traces_request_handler.ts

@@ -389,7 +418,7 @@ export const handlePayloadRequest = (
    .catch((error) => {
      console.error('Error in handlePayloadRequest:', error);
      coreRefs.core?.notifications.toasts.addError(error, {
-        title: `Failed to retrieve payload for trace ID: ${traceId}`,
+        title: `Failed to retrieve payload for trace Id: ${traceId}`,


We have toast messaged issued here, why aren't we seeing the toast pop up already when the user lands onto a wrong URL on trace view page today?

Screen.Recording.2025-04-16.at.4.23.30.PM.mov

I believe this is not considered a "error" as it just returns 0. I think our implementation prevents that to stop the toast message spam that was occuring on the landing page before anything was set up by the user.

In that case, may be remove this dead code, if it is not reachable

ps48 · 2025-04-16T21:56:12Z

public/components/trace_analytics/requests/traces_request_handler.ts

+
+  if (setTotalHits && (mode === 'data_prepper' || mode === 'custom_data_prepper')) {
+    const rootSpansDSL = cloneDeep(timeFilterDSL);
+    rootSpansDSL.query.bool.filter.push({


Looks like we are adding a _search request, this should be just _count request as we need just the hits here.

The size 0 part prevents the more intensive aggregation part and acts similar to count.
Keeping current implementation for the time being.

ps48 · 2025-04-16T21:56:59Z

public/components/trace_analytics/requests/traces_request_handler.ts

+  let rootSpansTotalHitsPromise: Promise<number> | undefined;
+
+  if (setTotalHits && (mode === 'data_prepper' || mode === 'custom_data_prepper')) {
+    const rootSpansDSL = cloneDeep(timeFilterDSL);


Question: Why do we need cloneDeep? I might have missed something here.

Clone deep is needed for the query to prevent any possible mutation as we adjust it to account for parentSpanId being empty.

ps48 · 2025-04-16T21:57:38Z

public/components/trace_analytics/components/traces/traces_content.tsx

+
+  useEffect(() => {
+    if (mode !== 'custom_data_prepper') {
+      setMaxTraces(500);


nit. Let's put this in constants

ps48 · 2025-04-16T22:00:31Z

.cypress/integration/trace_analytics_test/trace_analytics_traces.spec.js

@@ -137,7 +137,7 @@ describe('Testing traces table', () => {
  });


Would be great, if we can add tests to capture the error states that we add in this PR.

Added cypress for the empty state / invalid url
Services: (Note it haded to be added to the existing {it} the test suite fails random test if any additional describes/its are added. I believe this is due to memory issues from the upgrade, will look into breaking dashboards/services/traces into separate folders to prevent this in future PR)

describe('Testing service view empty state and invalid url', () => { it('Renders service view empty state and invalid url', () => { cy.visit(`app/observability-traces#/services?serviceId=${SERVICE_NAME}`, { onBeforeLoad: (win) => { win.sessionStorage.clear(); }, }); cy.get('[data-test-subj="globalLoadingIndicator"]').should('not.exist'); cy.contains('frontend-client').should('exist'); cy.contains('No matches').should('exist'); // Renders service view invalid url state cy.visit(`app/observability-traces#/services?serviceId=${INVALID_URL}`, { onBeforeLoad: (win) => { win.sessionStorage.clear(); }, }); cy.get('[data-test-subj="globalLoadingIndicator"]').should('not.exist'); cy.contains(`${INVALID_URL}`).should('exist'); cy.get('.euiCallOut.euiCallOut--danger') .should('exist') .within(() => { cy.get('.euiCallOutHeader__title') .should('contain.text', `Error loading service: ${INVALID_URL}`); cy.get('p') .should( 'contain.text', 'The service name is invalid or could not be found. Please check the URL or try again.' ); }); cy.contains('No matches').should('exist'); }); });

Traces:

describe('Testing trace view invalid url', () => { beforeEach(() => { cy.visit(`app/observability-traces#/traces?traceId=${INVALID_URL}`, { onBeforeLoad: (win) => { win.sessionStorage.clear(); }, }); }); it('Handles a invalid trace url', () => { cy.get('[data-test-subj="globalLoadingIndicator"]').should('not.exist'); cy.contains(`${INVALID_URL}`).should('exist'); cy.get('.euiCallOut.euiCallOut--danger') .should('exist') .within(() => { cy.get('.euiCallOutHeader__title') .should('contain.text', `Error loading Trace Id: ${INVALID_URL}`); cy.get('p') .should( 'contain.text', 'The Trace Id is invalid or could not be found. Please check the URL or try again.' ); }); }); });

ps48 · 2025-04-16T22:49:09Z

public/components/trace_analytics/components/traces/trace_table_helpers.tsx

@@ -219,7 +219,7 @@ export const getTableColumns = (
        field: 'percentile_in_trace_group',
        name: 'Percentile in trace group',
        align: 'right',
-        sortable: true,
+        sortable: false,


Question: Why is percentile not filterable?

The percentile field is only sort-able with the currently displayed on page values as the query can not be fired off with this field. The sorting also doesn't distinguish between the trace groups. Disabling it as the sorting could be misleading as it performs differently then all the others and doesn't provide accurate information.

ps48 · 2025-04-16T22:51:00Z

public/components/trace_analytics/components/traces/trace_view.tsx

+      } else {
+        setTraceIdError(false);


Do we need the else part here? as the page starts with traceIdError: false

handlePayloadResponse is part of the refresh so I think it is safer to maintain as the payload could be re-triggered if the mode changed.

ps48 · 2025-04-16T22:54:33Z

public/components/trace_analytics/components/common/shared_components/custom_datagrid.tsx

@@ -137,14 +137,38 @@ export const RenderCustomDataGrid: React.FC<RenderCustomDataGridParams> = ({
      )
    : [];

+  const isLastPage =
+    tracesTableMode === 'traces' && pagination?.pageSize && maxTraces != null


maxTraces is a number when will it be null?

ps48 · 2025-04-16T22:56:36Z

public/components/trace_analytics/components/common/shared_components/custom_datagrid.tsx

+      // Add 500 more traces at a time until all traces present
+      const nextIncrement = Math.min(500, rowCount - maxTraces);


may be we can declare this function first and then add it as a param to the useInjectElementsIntoGrid function

Updated to

const incrementTraceCount = () => { const nextIncrement = Math.min(500, rowCount - maxTraces); if (nextIncrement > 0 && maxTraces < MAX_DISPLAY_ROWS) { setMaxTraces((prevMax) => Math.min(prevMax + nextIncrement, MAX_DISPLAY_ROWS)); } }; useInjectElementsIntoGrid( rowCount, MAX_DISPLAY_ROWS, tracesTableMode ?? '', incrementTraceCount, maxTraces, isLastPage );

Signed-off-by: Adam Tackett <[email protected]>

TackAdam · 2025-04-18T16:36:29Z

PR updated to included empty state for Services, and differentiate between an empty call due to time filter and an invalid service name.

Screen.Recording.2025-04-18.at.9.35.52.AM.mov

Signed-off-by: Adam Tackett <[email protected]>

* remove sorting on invalid fields, adjust load more ui Signed-off-by: Adam Tackett <[email protected]> * change Trace ID to Trace Id, fix min size for customdatagrid Signed-off-by: Adam Tackett <[email protected]> * add error state for invalid url traces/services Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> * add selected time range callout to service page Signed-off-by: Adam Tackett <[email protected]> * add empty state to services, add cypress test Signed-off-by: Adam Tackett <[email protected]> * update cypress url Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> * cypress fix test Signed-off-by: Adam Tackett <[email protected]> * cypress fix test Signed-off-by: Adam Tackett <[email protected]> * remove aggregation from service name validation Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]> (cherry picked from commit 7dcfbaa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* remove sorting on invalid fields, adjust load more ui * change Trace ID to Trace Id, fix min size for customdatagrid * add error state for invalid url traces/services * address comments * address comments * add selected time range callout to service page * add empty state to services, add cypress test * update cypress url * address comments * cypress fix test * cypress fix test * remove aggregation from service name validation --------- (cherry picked from commit 7dcfbaa) Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Adam Tackett <[email protected]>

Adam Tackett and others added 2 commits April 8, 2025 13:34

remove sorting on invalid fields, adjust load more ui

7e5d651

Signed-off-by: Adam Tackett <[email protected]>

Merge branch 'main' into tracesLoadSort

47b9db9

TackAdam added the bug Something isn't working label Apr 8, 2025

TackAdam and others added 4 commits April 11, 2025 15:52

Merge branch 'main' into tracesLoadSort

ca88c63

Merge branch 'main' into tracesLoadSort

f55de39

change Trace ID to Trace Id, fix min size for customdatagrid

d03d109

Signed-off-by: Adam Tackett <[email protected]>

add error state for invalid url traces/services

be729eb

Signed-off-by: Adam Tackett <[email protected]>

TackAdam changed the title ~~Traces - Adjust sorting / update custom source~~ Traces - Update custom source toast/error/sorting Apr 14, 2025

TackAdam marked this pull request as ready for review April 14, 2025 23:18

paulstn reviewed Apr 15, 2025

View reviewed changes

...omponents/trace_analytics/components/common/shared_components/component_helper_functions.tsx Show resolved Hide resolved

paulstn reviewed Apr 15, 2025

View reviewed changes

public/components/trace_analytics/components/common/shared_components/custom_datagrid.tsx Outdated Show resolved Hide resolved

address comments

b86ce82

Signed-off-by: Adam Tackett <[email protected]>

paulstn reviewed Apr 15, 2025

View reviewed changes

public/components/trace_analytics/components/services/service_view.tsx Outdated Show resolved Hide resolved

address comments

17ed67e

Signed-off-by: Adam Tackett <[email protected]>

paulstn approved these changes Apr 15, 2025

View reviewed changes

add selected time range callout to service page

97cafa0

Signed-off-by: Adam Tackett <[email protected]>

RyanL1997 approved these changes Apr 16, 2025

View reviewed changes

ps48 reviewed Apr 16, 2025

View reviewed changes

TackAdam and others added 3 commits April 17, 2025 10:05

Merge branch 'main' into tracesLoadSort

48de3ba

add empty state to services, add cypress test

09111b8

Signed-off-by: Adam Tackett <[email protected]>

update cypress url

e07a5c2

Signed-off-by: Adam Tackett <[email protected]>

Adam Tackett added 3 commits April 17, 2025 16:12

address comments

58cb07f

Signed-off-by: Adam Tackett <[email protected]>

cypress fix test

9d51911

Signed-off-by: Adam Tackett <[email protected]>

cypress fix test

0a112c0

Signed-off-by: Adam Tackett <[email protected]>

remove aggregation from service name validation

f15098c

Signed-off-by: Adam Tackett <[email protected]>

ps48 approved these changes Apr 18, 2025

View reviewed changes

TackAdam merged commit 7dcfbaa into opensearch-project:main Apr 18, 2025
18 checks passed

TackAdam added the backport 3.0 label Apr 24, 2025

opensearch-trigger-bot bot mentioned this pull request Apr 24, 2025

[Backport 3.0] Traces - Update custom source toast/error/sorting #2427

Merged

		@@ -137,7 +137,7 @@ describe('Testing traces table', () => {
		});

		// Add 500 more traces at a time until all traces present
		const nextIncrement = Math.min(500, rowCount - maxTraces);

Traces - Update custom source toast/error/sorting #2407

Traces - Update custom source toast/error/sorting #2407

Uh oh!

Conversation

TackAdam commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TackAdam commented Apr 18, 2025

Uh oh!

Uh oh!

Uh oh!

TackAdam commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 15, 2025 •

edited

Loading