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

Update custom traces table with filters #2178

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ps48
Copy link
Member

@ps48 ps48 commented Sep 20, 2024

Description

  • The issue with original custom source table was that it says traces on top but shows data only from trace root spans. This is a problem cause it breaks users expectation of filtering traces across various fields like services and resource attributes.

Sorting and Pagination push down won’t be part of this PR.

Update custom traces table with filters added below:

  • All Spans - All spans from all traces
  • Traces - Aggregates all spans by traceId to show all traces
  • Service Entry Spans - The spans that mark start of server-side processing (SPAN_KIND_SERVER)
  • Trace Root Spans - The root spans which represent the starting point of a trace

Functional updates:

  • The trace table in custom source defaults to all spans
  • The trace table mode is stored in session storage, to persist across tab refreshes and page changes

Issues Resolved

  • Default table view
    Screenshot 2024-09-20 at 9 26 28 AM

  • Table filter options
    Screenshot 2024-09-20 at 9 26 38 AM

  • Table showing trace root spans
    Screenshot 2024-09-20 at 9 26 50 AM

  • Table showing attributes
    Screenshot 2024-09-20 at 9 27 00 AM

Demo Video

traces-table-filter.mov

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: Shenoy Pratik <sgguruda@amazon.com>
@ps48 ps48 added bug Something isn't working backport 2.x labels Sep 20, 2024
},
];

export const TRACE_TABLE_TYPE_KEY = 'TraceAnalyticsTraceTableType';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally the constants are in common only if they are shared between client (public) and server. if they are only used on one side they can go directly there

Copy link
Member Author

@ps48 ps48 Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Will keep this one here for now. Will sort out this and others in common/constants during a mini refactor.

public/components/trace_analytics/index.scss Show resolved Hide resolved
Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
should: [],
must_not: [],
},
},
...(sort && { sort: [{ [sort.field]: { order: sort.direction } }] }),
track_total_hits: false,
};

if (queryMode === 'trace_root_spans') {
dataPrepperQuery.query.bool.filter.push({
Copy link
Member

@vamsi-amazon vamsi-amazon Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not Related: we should have used an opensource name instead of data prepper. Its actually open telemetry schema.

{
label: TRACE_TABLE_TITLES.service_entry_spans,
key: 'service_entry_spans',
'aria-describedby': 'The spans that mark start of server-side processing (SPAN_KIND_SERVER)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel SPAN_KIND_SERVER is not needed. If required, bring in the exact description of SPAN_KIND_SERVER from open telemetry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spans marking the start of server-side processing
??

{
label: TRACE_TABLE_TITLES.traces,
key: 'traces',
'aria-describedby': 'Aggregates all spans by traceId to show all traces',
Copy link
Member

@vamsi-amazon vamsi-amazon Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spans aggregated by traceId to show all traces
Maintaining consistency where we start all the descriptions by Spans.

Do we need to show all traces?

How is this different from first option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is the old way of showing just traces using the aggregation query. Kept this option if users still want aggregation and are not concerned about the wait time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only table where we shows traces and not spans

traces = 'Traces',
}

export const TRACE_TABLE_OPTIONS = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users might ask for caching selected option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, for now I've added the selection in the session storage similar to how we store filters. So the table type selection is persisted in the session lifecycle.

{
label: TRACE_TABLE_TITLES.all_spans,
key: 'all_spans',
'aria-describedby': 'All spans from all traces',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Span from all traces.
??

{
label: TRACE_TABLE_TITLES.trace_root_spans,
key: 'trace_root_spans',
'aria-describedby': 'The root spans which represent the starting point of a trace',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root spans representing the starting point of each trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants