feat(traceview): Add previous trace link #85627
Open
+156
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
Bundle ReportChanges will increase total bundle size by 194.22kB (0.59%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: app-webpack-bundle-array-pushAssets Changed:
|
Lms24
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #85627 +/- ##
==========================================
- Coverage 87.74% 87.62% -0.12%
==========================================
Files 9863 9650 -213
Lines 558327 544924 -13403
Branches 22022 21472 -550
==========================================
- Hits 489881 477504 -12377
+ Misses 68039 67082 -957
+ Partials 407 338 -69 |
Abdkhan14
reviewed
Mar 18, 2025
| [location.query] | ||
| ); | ||
|
|
||
| const isLinkedTraceAvailable = useIsTraceAvailable(traceLink); |
There was a problem hiding this comment.
Suggested change
| const isLinkedTraceAvailable = useIsTraceAvailable(traceLink); | |
| const {isAvailable: isLinkedTraceAvailable} = useIsTraceAvailable(traceLink); |
Abdkhan14
reviewed
Mar 18, 2025
| return ( | ||
| <TraceLink | ||
| color="gray500" | ||
| to={getTraceDetailsUrl({ |
There was a problem hiding this comment.
Hey, we always pass a relevant timestamp to this link, for performance. If we are linking from a span id it's the timestamp of the span and if are linking from a trace id it's the min(timestamp) of the trace in question. Usually we are linking from tables where we make an /events/ query asking for timestamp/min(timestamp) as a column in the response. In this case it seems like it should be found in traceLink.
We should also pass this timestamp to useTrace instead of reading the timestamp from the url.
There was a problem hiding this comment.
Span links (like this trace link) don't have timestamps. If we need timestamps for the linked trace we'd have to make another call to fetch it.
The extra call doesn't seem worth it for p99, but it does seem to constrain the worst case load times quite a bit:
(above chart for last 7 days, source)
There was a problem hiding this comment.
So this timestamp in the link should show the min(timestamp) of the previous trace (where the button would link to)?
We only have the information of spanId and traceId in traceLink, that's why I was using the timestamp from the query params. But those params are from the trace you are currently looking at - so I understand that this would not be correct in this case.
Is there any other way I could get the timestamp of a trace if I just have the spanId and traceId?
mjq
reviewed
Mar 18, 2025
| return null; | ||
| } | ||
|
|
||
| if (!traceLink.sampled) { |
There was a problem hiding this comment.
Do we want the same behaviour here for both explicitly false (definitely not sampled) and missing value (unknown)?
Actually, would it make sense to first check isLinkedTraceAvailable? So:
- If
isLinkedTraceAvailable, show the trace link. - else, if
traceLink.sampled === false, show the unsampled trace message - else, return null
That way we show the trace if we have it (regardless of sampling flag) and also only warn them about sampling if we were explicitly told the linked trace was not sampled.
mjq
reviewed
Mar 18, 2025
| const trace = useTrace({ | ||
| traceSlug: traceLink?.trace_id, | ||
| timestamp: queryParams.timestamp, | ||
| }); |
There was a problem hiding this comment.
The trace fetching performance looks okay on our current system, but the EAP equivalent is currently too expensive for this purpose IMO. It's still in development (only internal users) so not currently a blocker and hopefully this is resolved before release, but if it doesn't then we'll probably want to replace this with an endpoint specifically for this check (to avoid loading a full trace) as you mentioned on Slack.
There was a problem hiding this comment.
Ah, great call. We could make an explore query for count(span.duration) where trace:<trace_id>, and if the result is > 0, show the trace link?
There was a problem hiding this comment.
I can't see a way to do this in the explore interface, but if we could instead search for min(timestamp) we'd be able to both confirm trace existence and also get the timestamp needed for the trace details URL in a single call, which would be slick.
There was a problem hiding this comment.
@mjq we don't use the events endpoint for that query we use a /traces/ endpoint that comes with numOfspans and the timestamp linked to the trace.
The only concern I have now is that, since the tracelink doesn't come with a timestamp, how do we set the date range for that explore query? Either of, 'querying by the current trace's timestamp with a buffer' or
'quering by max date range at all times' doesn't seem to be right 🤔
There was a problem hiding this comment.
At this point I think I'd suggest putting this behind a feature flag but otherwise merging it as is so we can start gathering real data from a subset of customers. We can loop back on optimizing the call with one of the options in this thread (or something else) after that. None of the options are perfect so I imagine it'll take some iterating, but with the feature flag we don't have to block the work so far on that iteration. How do you feel @s1gr1d @Lms24 ? (I guess this addresses "does this need a feature flag guard?" from the PR description 😄 )
There was a problem hiding this comment.
Based on this thread, I am not 100% sure what should be the path forward now :D
Should we either:
- Leave as-is and (don't?) put it behind a feature flag
- Use another API request and (don't?) put it behind a feature flag
And if we use another request here: Which one do you mean exactly here? I am not too familiar with the codebase so maybe you could tell me which hooks are already available for that.
As for the timestamp: The attached "previous trace" data only provides the spanId and traceId so I cannot know a timestamp at the point of fetching for this trace but we can define a time range. We know that the end timestamp of the previous trace would be anytime before the start timestamp of the trace that is currently shown in the trace view. And if we say that the maximum duration between linked traces is 1h, we can define the start time as "end time minus 1h". So then we would have this for the previous trace query:
- start timestamp: start time of current trace minus 1h
- end timestamp: start time of current trace
There was a problem hiding this comment.
start timestamp: end time of current trace minus 1h
shouldn't this be start time of current trace minus 1h?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Open Question:
Negatively Sampled Previous Trace
If we know that the previous trace was negatively sampled in the SDK, we show this:
Trace Link
Loading State
--> There was the idea to show a skeleton component while loading. But this would cause layout shifts most of the time as most traces don't have a connected trace and then the button would just dissapear.
This screenshot is just kept as reference - we don't show a loading state.
closes #85739
UPDATE: Cleaned up PR description since this is now ready for a review!