feat(opentelemetry): Ensure DSC & attributes are correctly set #10806
Conversation
mydea
force-pushed
the
fn/otel-dsc-attributes
branch
from
85a7887 to
b35d861
Compare
size-limit report 📦
|
There was a problem hiding this comment.
LGTM!
Should we remove the otel.attributes context data then?
What field is this exactly on the event? I don't think we should remove stuff from event.contexts.trace.data (which IIUC span attributes end up in) because IIRC either ingest or the product later need these fields.
mydea
force-pushed
the
fn/otel-dsc-attributes
branch
from
b35d861 to
0796c63
Compare
What I mean is that we send the "root span" attributes in |
There was a problem hiding this comment.
I have a feeling that we are missing the inverse logic in inject. More concretely we need to take the dynamic sampling context of the current span when inject is called, e.g. by calling getDynamicSamplingContextFromSpan([current span]) on the current span. I think I might have missed this previously.
| // If event has already set `trace` context, use that one. | ||
| event.contexts = { | ||
| trace: { | ||
| trace: dropUndefinedKeys({ |
There was a problem hiding this comment.
Why do we have to add dropUndefinedKeys here?
There was a problem hiding this comment.
I figured it's nicer to not send an undefined parent_span_id (this was one debugging step when I was fixing tests etc), but we can also leave it!
| const rootSpan = getRootSpan(span); | ||
| const transactionName = spanHasName(rootSpan) ? rootSpan.name : undefined; | ||
| if (transactionName) { | ||
| event.tags = { transaction: transactionName, ...event.tags }; |
There was a problem hiding this comment.
Is transaction really a tag? I thought the transaction for an error was stored in the top level transaction field.
There was a problem hiding this comment.
we store both of these right now - we can revisit later if this is (still?) needed, but I figured to maintain parity for now this makes sense!
| startTimestamp: convertOtelTimeToSeconds(span.startTime), | ||
| metadata: { | ||
| sampleRate: span.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as number | undefined, | ||
| ...dropUndefinedKeys({ |
There was a problem hiding this comment.
Same q: Why dropUndefinedKeys here?
There was a problem hiding this comment.
here I think it is important, because dynamicSamplingContext may be undefined in which case we don't want to set it, because later in the pipeline if you have e.g. this:
metadata: {
dynamicSamplingContext: xx,
...event.metadata
}
this would overwrite xx even if it is undefined, I think?
There was a problem hiding this comment.
right... makes sense! Thank you!
|
|
||
| if (capturedSpanScopes) { | ||
| setCapturedScopesOnTransaction(transaction, capturedSpanScopes.scope, capturedSpanScopes.isolationScope); | ||
| // Ensure the `transaction` tag is correctly set on the transaction event |
There was a problem hiding this comment.
Same q: Is the transaction value really a tag? It shows up in the interface as tag but I think it is top level on the event, eg: https://us.sentry.io/api/0/projects/sentry/sentry/events/9f6a861ccacf43868a4d7d6d63b9d922/json/
|
OK, I also updated the propagator to look at the span trace state first, and only then fall back to propagation context! I also added tests to cover this, and also to ensure trace state is correctly inherited & applied for spans. |
mydea
force-pushed
the
fn/otel-dsc-attributes
branch
from
bbb7c84 to
cabb3d4
Compare
mydea
force-pushed
the
fn/otel-dsc-attributes
branch
from
cabb3d4 to
39d43cc
Compare
3 participants
This PR fixes a few things about the OTEL handling of our SDK.
sentry.sample_ratewhich we don't do in "main" SDK. Question: Should we remove theotel.attributescontext data then? As this is kind of redundant at this point with thedataon the trace?transactiontag correctly on the transaction, to align with "main" SDK.