Uh oh!
There was an error while loading. Please reload this page.
feat(core): Add OpenTelemetry-specific getTraceData implementation - #13281
Conversation
e954a52 to
9729eceCompare| * 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:
functiongetContextForSpan(span){constscopes=getCapturedScopesOnSpan(span);if(scopes.scope){returngetContextFromScope(scopes.scope);}returnundefined;}constctx=(span&&getContextForSpan(span))||context.active();constheaders={};// this mutates the headers object// this also handles it if there is no active span etc.propagation.inject(ctx,headers,headerSetter);returnheaders;size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
getTraceData versiongetTraceData implementation92353fd to
962962dCompare| 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
There was a problem hiding this comment.
nice, IMHO this is much better and simpler/clearer - reduced complexity! 🚀
7dd1123 to
dda6ee6Compare
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.