feat(core): Add OpenTelemetry-specific getTraceData implementation#13281
feat(core): Add OpenTelemetry-specific getTraceData implementation#13281
getTraceData implementation#13281Conversation
Lms24
force-pushed
the
lms/fix-getTraceData-otel
branch
from
e954a52 to
9729ece
Compare
| * Otel-specific implementation of `getTraceData`. | ||
| * @see `@sentry/core` version of `getTraceData` for more information | ||
| */ | ||
| export function getTraceData(span?: Span): TraceData { |
There was a problem hiding this comment.
Would appreciate a review @mydea - I might be far off here. Basically what I need to get is a Context to pass to propagation.inject. I followed the code in our node fetch integration to get it but I'm not sure if this is correct:
sentry-javascript/packages/node/src/integrations/node-fetch.ts
Lines 84 to 100 in 21830b1
There was a problem hiding this comment.
Or IOW, maybe we can get rid of most code here and instead just somehow get to a Context directly from the SpanContex of spanToUse.spanContext()?
There was a problem hiding this comment.
The tricky thing is getting the context from the span, if one is passed in. but it should be possible, you can get the scope via getCapturedScopesOnSpan(span).scope, and from there use getContextFromScope(scope) to get the context.
Then, we can really just do this:
function getContextForSpan(span) {
const scopes = getCapturedScopesOnSpan(span);
if (scopes.scope) {
return getContextFromScope(scopes.scope);
}
return undefined;
}
const ctx = (span && getContextForSpan(span)) || context.active();
const headers = {};
// this mutates the headers object
// this also handles it if there is no active span etc.
propagation.inject(ctx, headers, headerSetter);
return headers;
size-limit report 📦
|
Lms24
changed the title
getTraceData versiongetTraceData implementation
Lms24
force-pushed
the
lms/fix-getTraceData-otel
branch
from
92353fd to
962962d
Compare
| const context = api.context.active(); | ||
|
|
||
| // This should never happen, given we always create an ambient non-recording span if there's no active span. | ||
| if (!context) { |
There was a problem hiding this comment.
I think this cannot be empty? 🤔 What does the type say?
There was a problem hiding this comment.
I could have sworn TS screamed at me previously that the type was Context | undefined but after re-checking you're right, we always get back a context. Adjusted it
| @@ -1,4 +1,4 @@ | |||
| export type { ClientClass } from './sdk'; | |||
| export type { ClientClass as SentryCoreCurrentScopes } from './sdk'; | |||
There was a problem hiding this comment.
m: is this on purpose? 😅 seems unintentional (and probably breaking, technically...)
mydea
left a comment
mydea
left a comment
There was a problem hiding this comment.
nice, IMHO this is much better and simpler/clearer - reduced complexity! 🚀
Lms24
force-pushed
the
lms/fix-getTraceData-otel
branch
from
7dd1123 to
dda6ee6
Compare
2 participants
This PR adds an Otel-specific version of
getTraceDataand adds thegetTraceDatafunction to theAsyncContextStrategyinterface. This allows us to dynamically choose either the default implementation (which works correctly for browser/non-POTEL SDKs) and the Otel-specific version.Our previous, universal implementation of
getTraceDatadidn't propagate the correct trace data in POTEL SDKs in Tracing Without Performance mode. The reason is that even in TwP mode, we start a non-recording span which is available viagetActiveSpan. The previous implementation would therefore incorrectly take trace data from this span and even more incorrectly add a negative sampling decision to the trace data instead of deferring it.Another change in this PR is that I removed the optional parameters for
getTraceDataandgetTraceMetaTags. This is breaking but the function wasn't documented publicly at all yet. The reason is that this was premature API expansion because we realized we didn't need this functionality in our internal use cases. Furthermore, it would have severely complicated obtaining the correct trace data in the Otel implementation.