-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): continueTrace doesn't propagate given trace ID if active span exists #18328
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
…already an active span
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Lms24
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.
Nice work, thanks! (and thanks for fixing that Sveltekit test!)
| // simulating a nested load call: | ||
| await sentryHandle()({ | ||
| event: mockEvent({ route: { id: 'api/users/details/[id]', isSubRequest: true } }), | ||
| event: mockEvent({ route: { id: 'api/users/details/[id]' }, isSubRequest: true }), |
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.
good catch, and very interesting that this passed beforehand 🤔 👀
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.
yeah it is! didn't investigate further though
| }); | ||
| }); | ||
|
|
||
| it('works inside an active span', () => { |
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.
l: let's find a slightly more descriptive description. Feel free to go with something else but suggestion:
| it('works inside an active span', () => { | |
| it('updates the propagation context when called inside an active span', () => { |
| }); | ||
| }); |
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: Let's also add a second test that shows that when you now start another span within the continueTrace callback, it has the traceId and parentSpanId that's passed to continueTrace.
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.
sure, added!
Lms24
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.
Thanks!
During investigation of this issue, we discovered that
continueTracedoes not propagate the given trace ID if it is called from within an already active span (in this instance this happened because of the already active span started by the cloudflare auto-instrumentation.Solution: This PR resolves this behavior by setting the currently active span to undefined when forking the propagation context in
continueTrace.Tests:
@sentry/sveltekitunit tests started to fail. Not sure why this was triggered by this change, but I fixed it by setting theisSubRequestproperty on the event instead of the route, which seems correct to me.@sentry/corethat callscontinueTraceinside an active span and checks that the trace id is now propagated as we would expect.