test: Add tests to demonstrate root trace ID behavior #14426
Conversation
There was a problem hiding this comment.
Thanks for writing these tests! Always good to know what's going on.
In node, I would align the behavior to ignore the trace ID when we have no incoming trace (so no parentSpanId), and always use the traceId & parentSpanId if there is a parentSpanId on the scope (even the root scope, to be consistent).
I agree, this sounds like a solid plan. One thing we should keep in mind (and I only thought of this today) is that for use cases like crons, users can pass in trace data via env variables. But I think this should still work, assuming the SENTRY_TRACE env variable is set correctly.
Also agree with that browser behaviour should stay as-is as of now. Once we can link traces, we can shorten the lifetime of the traceId again, meaning we can change the behaviour around propagationContext as well. We could just recycle it whenever we start a root span and it stays valid until then (for errors outside of spans)
| expect(traceId).toBeDefined(); | ||
|
|
||
| // It ignores propagation context of the root context | ||
| expect(traceId).not.toBe('12345678901234567890123456789012'); |
There was a problem hiding this comment.
interesting! I would have thought it picks up the trace id from the propagation context, given that there's no request/non-recording span. But good to know!
There was a problem hiding this comment.
yeah, as you noted below, this is def. weird behavior but happens because of special stuff that happens around the ROOT_CONTEXT 😅
There was a problem hiding this comment.
okay so if we're within the withSpan closure, we do actually take the traceId from the PC of the current scope?
| @@ -176,8 +176,9 @@ function ensureTimestampInMilliseconds(timestamp: number): number { | |||
|
|
|||
| function getContext(scope: Scope | undefined, forceTransaction: boolean | undefined): Context { | |||
| const ctx = getContextForScope(scope); | |||
| // Note: If the context is the ROOT_CONTEXT, no scope is attached | |||
| // Thus we will not use the propagation context in this case, which is desired | |||
There was a problem hiding this comment.
ahh I see, this expains the behaviour I wondered about above
This adds node & browser integration tests to demonstrate the current behavior of "trace propagation" based on the current scope.
It shows that the behavior is not really consistent and we should probably adjust it, but for now this PR does no changes.
Browser
In browser, the propagation context of the current scope is always picked up. This means that if you call
startSpanmultiple times in the root, both started spans will have the same trace ID, and possibly the sameparentSpanIdif there was an incoming trace.Node
In node, the propagation context is ignored for the root context. So in the root, started spans will have different traces, and will also ignore parentSpanId (which is only theoretical, there will never really be a parentSpanId on the root scope in node, realistically).
Outside of the root (so e.g. within any route handler, or even any
withScopecall), the behavior is the same as in browser - any started spans will share the same trace ID/parent span ID.What should we take away from this?
In node, I would align the behavior to ignore the trace ID when we have no incoming trace (so no parentSpanId), and always use the traceId & parentSpanId if there is a parentSpanId on the scope (even the root scope, to be consistent).
In browser, we cannot make this change because we rely on this behavior for the extended traces after pageload/navigation spans have ended.