fix(opentelemetry): Always use active span in Propagator.inject
#13381
Conversation
size-limit report 📦
|
| res.send({ | ||
| traceData: Sentry.getTraceData(), | ||
| traceMetaTags: Sentry.getTraceMetaTags(), | ||
| errorTraceContext: event.contexts.trace, | ||
| }); |
There was a problem hiding this comment.
I get this is not the way how we usually test event payloads but I don't think I can assert on a specific traceId in our runner expectError function which we don't know upfront.
| expect(traceData).toEqual({ | ||
| 'sentry-trace': `${traceId}-${spanId}`, | ||
| baggage: expect.stringContaining(`sentry-trace_id=${traceId}`), | ||
| }); | ||
|
|
||
| expect(traceMetaTags).toContain(`content="${traceId}-${spanId}"/>\n`); | ||
| expect(traceMetaTags).toContain(`sentry-trace_id=${traceId}`); |
There was a problem hiding this comment.
these trace and span ids were not the same as the error trace context's trace id without the change
Lms24
changed the title
getInjectionDataPropagator.inject
|
Hmm I don't understand why the node integration tests fail on CI. They pass locally so I suspect it's rather a test problem than a problem with the change. But I'll look into this on Monday. |
Lms24
force-pushed
the
lms/fix-opentelemetry-getInjectionData
branch
from
29e1ae0
to
733483d
Compare
There was a problem hiding this comment.
I am not 100% sure anymore if/why this was necessary in the PR that introduced this (#11564). But if tests pass, this should be fine I'd say 😅
FWIW there is not always a remote span active, only if an incoming trace is continued I believe.
Right... let me check the scenario for when there's no incoming trace then 😅 |
@mydea so the thing is, the test I added has no incoming trace headers. So looks like we always have an active span now? 😅 |
Lms24
force-pushed
the
lms/fix-opentelemetry-getInjectionData
branch
from
733483d
to
79921ff
Compare
Not 100% sure, but I think we always have an active span if there is an incoming request. We probably (I think?) have none when outside of a request context 🤔 I think the propagator always creates a virtual span, but |
|
Ahh I see, so adding a test without a server should cover this scenario then, right? |
I'd say so 👍 |
|
I added a test for non-server/request events. Also rewrote the existing test for events within a request. Looks like TwP mode now has consistent traces everywhere. Just gotta get them passing in CI now 😅 |
|
Ahh yes, I think this makes sense now! In case there's no active span, we simply fall back to reading the trace from the propagationContext on the scope. Which is identical to what we do when populating the trace context. I'm going to merge this PR. |
This PR attempts to fix an inconsistency in TwP mode where we previously returned a different trace id in
getTraceDatathan the one we include in the trace context of error events.Background:
getTraceData, we take the span id fromPropagator.injecthasTracingEnabledto either return the traceId from a span (true) or from the propagationContext on the scope (false).getInjectionDatahelper function=> This PR removes the check for
hasTracingEnabledingetInjectionDataand simply always takes the non-recording span if there is one. Which I guess(?) is always the case 🤔I'm not sure if this is the way to fix this but it is one way and it seems to not break any existing tests.
Also added a regression test that only passes with this fix.
Alternatively, we can adapt the error event creation pipeline to follow the same logic for the trace context as in our propagator. For example, we create a potel/core (acs) implementation for something like
getTraceContext. I can look into this if reviewers prefer it. I guess it comes down to: Do we effectively want to replace the propagationContext with the ambient non-recording span or should we still fall back to the PC if we're in TwP mode?