Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v9] Rethink spanId on propagationContext #12385

Open
mydea opened this issue Jun 6, 2024 · 2 comments · May be fixed by #14733
Open

[v9] Rethink spanId on propagationContext #12385

mydea opened this issue Jun 6, 2024 · 2 comments · May be fixed by #14733
Assignees
@mydea
Labels
Feature: Tracing
Milestone
9.0.0

Comments

@mydea
Copy link
Member

mydea commented Jun 6, 2024

Today, we have a propagation context on every span that receives a random traceId and spanId (unless it is continued from an external service). We use these IDs to populate trace headers for further propagation.

A scope always has a propagation context, and you can't really tell if this is a "random" one or one that is continued from somewhere...

We should maybe reconsider this, as today this leads to a bunch of confusion, and may lead to cases where we send a parent_span_id that does not exist (because it comes from the propagationContext, not an actual span).
@AbhiPrasad AbhiPrasad added this to the 9.0.0 milestone Jun 6, 2024
@lforst lforst mentioned this issue Oct 31, 2024
Open
@mydea
Copy link
Member Author

mydea commented Nov 11, 2024

More concretely, I propose the following changes:

  1. Make the propagation context optional on the scope.
  2. Conceptionally the propagation context should be owned by the isolation scope, not the current scope - we will only set it on the isolation scope, so unless a user specifically overwrites this on the current scope, we'll always use the isolation scope one.
  3. The isolation scope will always have a propagation context, so we ensure TWP still works.
  4. Update the type of propagation context like so (naming TBD but you get the idea):
interface LocalPropagationContext {
  traceId: string;
  virtualSpanId: string;
}

interface ContinuedPropagationContext {
  traceId: string;
  sampled?: boolean;
  parentSpanId: string;
  dsc?: Partial<DynamicSamplingContext>
}

interface Scope {
  // undefined: this is probably not an isolation scope. should never be undefined on an isolation scope
  // LocalPropagationContext: TWP mode (else this is ignored anyhow), default
  // ContinuedPropagationContext: Incoming trace is continued
  propagationContext: undefined | LocalPropagationContext | ContinuedPropagationContext;
}

This way, you can always differentiate what state you are in, if you are using a "local/virtual" propatation context (by checking if virtualSpanId exists), or if you are continuing a trace. This gets rid of the somewhat confusing spanId property.

@lforst lforst mentioned this issue Nov 13, 2024
Closed
@lforst lforst modified the milestone: 9.0.0 Nov 13, 2024
@mydea mydea mentioned this issue Nov 19, 2024
Merged
mydea added a commit that referenced this issue Nov 26, 2024
@mydea
feat: Streamline sentry-trace, baggage and DSC handling (#14364)
b59ce07 
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:

* `getTraceData` already exists, but was extended to also allow to pass
a specific `span` to 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.
* For this and edge cases, there are new primitives
`getDynamicSamplingContextFromScope` and `getTraceContextFromScope`
which handle picking these things from given scope etc.

While working on this, I also noticed that `captureCheckIn` in
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.
@lforst
Copy link
Member

lforst commented Nov 27, 2024

Our current view on this topic is that span ID on the prop context can be safely removed.

Architecturally we need to ensure that performance and the prop context are completely decoupled.

Performance data may feed into the prop context but not the other way around. The prop context was designed for tracing without performance.

As part of this task we need to go through the entire SDK and make sure that the any performance tracking mechanisms don't read from the propagation context.

@mydea mydea self-assigned this Dec 16, 2024
mydea added a commit that referenced this issue Dec 16, 2024
@mydea
feat!: Remove spanId from propagation context
d7c8b25 
Closes #12385

This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there...
@mydea mydea linked a pull request Dec 16, 2024 that will close this issue
Open
mydea added a commit that referenced this issue Dec 16, 2024
@mydea
feat!: Remove spanId from propagation context
4757ebc 
Closes #12385

This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there...
mydea added a commit that referenced this issue Dec 17, 2024
@mydea
feat!: Remove spanId from propagation context
5299ebe 
Closes #12385

This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there...
mydea added a commit that referenced this issue Dec 17, 2024
@mydea
feat!: Remove spanId from propagation context
05ca00e 
Closes #12385

This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mydea mydea

Labels
Feature: Tracing
Projects
None yet
Milestone
9.0.0
Development

Successfully merging a pull request may close this issue.

feat!: Remove spanId from propagation context getsentry/sentry-javascript
3 participants
@mydea @lforst @AbhiPrasad