-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(trace-waterfall): Add previous and next trace buttons (EAP) #96510
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
Conversation
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Outdated
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Outdated
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceWaterfall.tsx
Outdated
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Show resolved
Hide resolved
|
@DominikB2014 @Abdkhan14 would you mind taking a look at this? If you have any questions about span link storage, I can help, but we need your eyes on how this is rendered, and whether the changes to the data-fetching hooks makes sense 🙏🏻 |
|
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Show resolved
Hide resolved
| projectId: _projectId, | ||
| environment: _environment, | ||
| }; | ||
| }, [attributes]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(M - I'm not sure how much attention we give to objects in dependency arrays): As attributes is a non-primitive type in JS, the useMemo will probably create even a little performance overhead. As the object reference is always different, useMemo will probably be called on every render (and thus, not really memoize, if even if the array is the same).
There are different methods to account for this, I like this comment: facebook/react#14476 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed offline, turns out the useMemo call here makes the attribute computation run less often than the component rerendering. So seems like this is fine 🤔
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Outdated
Show resolved
Hide resolved
s1gr1d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my side but would wait for the others to approve as well :)
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts
Show resolved
Hide resolved
static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx
Show resolved
Hide resolved
Abdkhan14
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets give this a shot 🚢
) Reworks the previous implementation of showing previous/next trace buttons: - Linked Trace buttons are now compatible with EAP (only)! - For now, we solely rely on the `sentry.previous_trace` span attribute. Meaning, we don't use span links at the moment, because span links in EAP are WIP. In the near future they'll be stored as a JSON blob. For us to rely on them, we need them to be queryable again though. Therefore, the SDK currently sends the `sentry.previous_trace` span attribute, which we can use to search for specific spans on the EAP dataset. - the buttons are still feature-flagged for the `sentry` and `sentry-sdks` orgs. We can remove the flags once we dogfooded for a bit and things seem to work as expected. - Use the `useSpans` query to find the next trace linking to the current trace (reverse span link lookup) - Use the `useSpans` instead of `useTrace` query to check if the previous trace is actually available (vs. dropped server-side/not ingested) - Rework the logic around showing "previous trace not available|sampled" errors - Use two dedicated hooks `findPreviousTrace` and `findNextTrace` instead of once combined - Add placeholders to avoid the waterfall panel's lower end shifting upwards once links are loaded (see video) - This has the inherent disadvantage that the vertical space for the waterfall becomes smaller. We can explore alternative positions for the previous/next buttons to get rid of this limitation again. I'd argue that for now, as long as things are feature flagged, having the placeholders is better than causing layout shift and repaints.
) Reworks the previous implementation of showing previous/next trace buttons: - Linked Trace buttons are now compatible with EAP (only)! - For now, we solely rely on the `sentry.previous_trace` span attribute. Meaning, we don't use span links at the moment, because span links in EAP are WIP. In the near future they'll be stored as a JSON blob. For us to rely on them, we need them to be queryable again though. Therefore, the SDK currently sends the `sentry.previous_trace` span attribute, which we can use to search for specific spans on the EAP dataset. - the buttons are still feature-flagged for the `sentry` and `sentry-sdks` orgs. We can remove the flags once we dogfooded for a bit and things seem to work as expected. - Use the `useSpans` query to find the next trace linking to the current trace (reverse span link lookup) - Use the `useSpans` instead of `useTrace` query to check if the previous trace is actually available (vs. dropped server-side/not ingested) - Rework the logic around showing "previous trace not available|sampled" errors - Use two dedicated hooks `findPreviousTrace` and `findNextTrace` instead of once combined - Add placeholders to avoid the waterfall panel's lower end shifting upwards once links are loaded (see video) - This has the inherent disadvantage that the vertical space for the waterfall becomes smaller. We can explore alternative positions for the previous/next buttons to get rid of this limitation again. I'd argue that for now, as long as things are feature flagged, having the placeholders is better than causing layout shift and repaints.
This PR reworks the previous implementation of showing previous/next trace buttons:
sentry.previous_tracespan attribute. Meaning, we don't use span links at the moment, because span links in EAP are WIP. In the near future they'll be stored as a JSON blob. For us to rely on them, we need them to be queryable again though. Therefore, the SDK currently sends thesentry.previous_tracespan attribute, which we can use to search for specific spans on the EAP dataset.sentryandsentry-sdksorgs. We can remove the flags once we dogfooded for a bit and things seem to work as expected.useSpansquery to find the next trace linking to the current trace (reverse span link lookup)useSpansinstead ofuseTracequery to check if the previous trace is actually available (vs. dropped server-side/not ingested)findPreviousTraceandfindNextTraceinstead of once combinedDemo
Screen.Recording.2025-07-28.at.13.06.38.mov
Notes for reviewers
useSpansquery API a bit because apparently the custompageFiltersprop was only passed down to the underlyingeventView(not exactly sure what this is tbh) but not to theuseWrappedDiscoverQueryhook. This hook would instead call the globalusePageFiltersbut not respect any custompageFilters, leading to my queries being disabled instead of sent. To solve this, I added aqueryWithoutPageFiltersoption, which iftruecallsuseWrappedDiscoverQueryWithoutPageFiltersinstead.I'm more than happy to rework this part but need some advice from people who know how this is actually supposed to work (any input appreciated)!
In general, if you see any nonsense regarding React patterns, please call it out! (I'm by far not fluent in React)
ref: https://linear.app/getsentry/issue/JS-795/linked-traces-find-previous-and-next-traces-based-on-temporary-span
closes #90047