ref: Streamline sentry-trace, baggage and DSC handling
#14364
Conversation
| }; | ||
|
|
||
| const trace = evt.contexts && evt.contexts.trace; | ||
| if (!trace && propagationContext) { |
There was a problem hiding this comment.
These checks were not really necessary, I believe - first, propagationContext cannot be empty here. And second, we do not need to check for trace as this will be overwritten anyhow by ...evt.contexts if it already exists.
| }; | ||
|
|
||
| const sentryTraceHeader = | ||
| span && hasTracingEnabled() ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled); |
There was a problem hiding this comment.
Here, we have checked for span && hasTracingEnabled(), while in fetch we did not check for hasTracingEnabled(). I opted to use this logic (just check span) everywhere now...
There was a problem hiding this comment.
For browser, I think that's fine!
size-limit report 📦
|
mydea
force-pushed
the
fn/headers-unify
branch
2 times, most recently
from
63ab398
to
0d74e78
Compare
Hmm, you are right that these are kind of similar 🤔 but |
mydea
force-pushed
the
fn/headers-unify
branch
from
a376535
to
1608c4c
Compare
| request, | ||
| client, | ||
| scope, | ||
| undefined, |
There was a problem hiding this comment.
these just remain for backwards compatibility, because addTracingHeadersToFetchRequest is exported :/
| client.init(); | ||
|
|
||
| return client; | ||
| } | ||
|
|
||
| describe('getTraceData', () => { |
There was a problem hiding this comment.
these tests relied a lot on mocks, and since the underlying implementation was changed a bunch of this broke. So I rewrote these tests to use "real" stuff as much as possible, but tried to keep the scenarios we want to cover intact.
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| @@ -91,9 +88,14 @@ export class SentryPropagator extends W3CBaggagePropagator { | |||
|
|
|||
| const activeSpan = trace.getSpan(context); | |||
| const url = activeSpan && getCurrentURL(activeSpan); | |||
| // We only want to check the URL against tracePropagationTargets for outgoing request spans | |||
| // In other cases, we always inject the trace data | |||
| const shouldCheckUrl = | |||
There was a problem hiding this comment.
Without this, injecting metadata e.g. in remix (but possibly other places!) would fail, because in this scenario the active span may be e.g. the http.server span, which has a http.url attribute that we would check and which may fail to check against tracePropagationTargets, disabling injection and thus not generating any meta tags.
Now, we only check against the http.url if the span is a http.client span.
There was a problem hiding this comment.
sadly span kind is not set in a super consistent way, so to be safe we also check the op to ensure out internal use-cases work.
Just to be clear, for all other spans trace data will always be injected.
|
OK @Lms24 and @lforst I have updated this to now use |
| @@ -7,6 +7,7 @@ sentryTest('should not add source context lines to errors from script files', as | |||
| const url = await getLocalTestUrl({ testDir: __dirname }); | |||
|
|
|||
| const eventReqPromise = waitForErrorRequestOnUrl(page, url); | |||
| await page.waitForFunction('window.Sentry'); | |||
There was a problem hiding this comment.
kind of unrelated but this kept flaking every now and then, just fixing this here...
This reverts commit 58bb2ac.
|
Nevermind, still not working - need another round 😬 |
|
@lforst & @Lms24 OK, Now for real I think it should work. I had to change the custom |
| }; | ||
|
|
||
| const sentryTraceHeader = | ||
| span && hasTracingEnabled() ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled); |
There was a problem hiding this comment.
For browser, I think that's fine!
As a first step for #12385, I looked though our current implementations, and tried to streamline this. We have a bunch of somewhat duplicate code there that handles sentry-trace & baggage headers mostly the same but not fully the same, as well as relatedly the DSC.
To streamline this, I added some new methods in core:
getTraceDataalready exists, but was extended to also allow to pass a specificspanto it. I updated some code to use this method that hasn't used it before, and also added some more tests - also around the ACS and the otel-specific implementation for this.getDynamicSamplingContextFromScopeandgetTraceContextFromScopewhich handle picking these things from given scope etc.While working on this, I also noticed that
captureCheckInin ServerRuntimeClient was actually not properly working in Node (OTEL), as it relied on the core way of scope-span coupling. So I also updated this to actually work as expected.