-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(waterfall): Prefer specific root events in trace root lookup #104050
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
base: master
Are you sure you want to change the base?
Conversation
|
For some reason (and this is also not working on
|
nsdeschenes
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.
This all makes sense to me, tho would like a second set of eyes on it 😄
I debugged this and found that the |
|
Ya you are absolutely right. The environments are different. I'd be up for removing the env filter |
…ts (#104095) Fixes #104050 (comment) where we discovered that if subsequent traces have different environments, our lookup for the next trace fails.
|
(This was closed accidentally) |
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.
This makes sense to me!
@Lms24 lets apply the preferred-root logic to to both eap and non-eap traces, getting rid of the isEAPTransaction(event) check. I'm working towards mitigating type-guard usage in this space.
Also, we never expect there to be a mix of eap and non-eap events, we can get rid of eapRootEvent
|
Thanks for the review @Abdkhan14. I got rid of the EAP-specific checks. Can you double check if this is still correct? My testing still shows the same behavior as before but I'm not sure if there is anything I missed with non-EAP events (are they even still a thing?) Update: made two more changes (thx to AI review bots for catching):
|
| // We prefer certain root events over conventional root events. | ||
| // These make better titles in the waterfall view and make linked | ||
| // trace navigations work. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
nice! totally missed this. But I'm not sure if it's actually a concern (see my Q about none-EAP here). Anyway, I extracted the now common logic.
static/app/views/performance/newTraceDetails/traceApi/utils.tsx
Outdated
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.
Exactly what I was talking about 🚢
This PR fixes two issues but to be honest, I'm a bit unsure of all consequences. Would appreciate a thorough review from someone who actually knows the waterfall view :)
While fixing fix(waterfall): Relax "next_trace" lookup to just the trace id #104047, I also noticed that we had a similar problem for the "previous trace" button:
If we look at a trace in the waterfall where the first root span is not a navigation/pageload span but e.g. a web vital span, we can't look up the trace's
previous_tracelink. Therefore, we can only show the disabled "previous trace" button in the UI, although there actually is a previous trace.For EAP-based traces, it seems like we always took the first transaction event as the title of the current trace. This sometimes led to a bit sub optimal titles in my opinion. I noticed we had a more selective logic for none-EAP events, where we preferred the
pageload,navigationandui.loadevents over the "first" event.To fix both of these issues, this PR ports the none-EAP logic of selecting a trace root to the EAP logic. Also kept the heuristic in place to prefer an EAP root event over a none-EAP event in case of mixed traces (?).
Before:

After:
