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.

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

Consolidate propagation and dynamic sampling contexts on Scope #10095

Closed
Lms24 opened this issue Jan 8, 2024 · 2 comments
Closed

Consolidate propagation and dynamic sampling contexts on Scope #10095

Lms24 opened this issue Jan 8, 2024 · 2 comments

Comments

@Lms24
Copy link
Member

Lms24 commented Jan 8, 2024

Problem Statement

Right now, we already have PropagationAPIs on the Scope ((get|set)PropagationContext). The propagation context also holds a dynamic sampling context. However, we also have/had (#10094) APIs like transaction.getDynamicSamplingContext which in many situations returned different DSC data than propagation context.

This leads to the inconvenience that everywhere where we need to propagate tracing data, we kinda have to check if there's an active txn, if yes, get the DSC from there and only otherwise use the DSC from the propagation context.

Solution Brainstorm

An ideal solution IMO is to always view Scope.getPropagationContext as the source of truth for trace propagation.

  • Calling getIsolationScope().getPropagationContext always returns the correct distributed tracing data (that is, traceparent data and DSC). Regardless of if there is an active span or not (TwP).
  • There should be no need to call any additional function or method when propagating tracing information
  • Ensuring DSC is immutable for transactions should still work, although it’s not enforced at the moment
    • In this case, the first getPropagationContext call would “freeze” the DSC for the current active transaction/root span

Also, this is how propagation context should work according to our distributed tracing dev spec.

getPropagationContext Cases (with immutability)

  • No active span
    • return fallback DSC (i.e. the one created initially TwP OR the one that was added via setPropagationContext
  • Active span
    • There already is DSC for this span
      • Return this one in the PC
    • No propagationContext for this span (or we don’t care about immutability)
      • populate new one in the PC

Note: Current State of mutability of DSC

  • If DSC is set on the transaction when it’s created (e.g. via startTxn or startSpan), this DSC will be returned from calling getDynamicSamplingContext
  • Otherwise, calling getDynamicSamplingContext will always return a fresh DSC and ignore previously calculated DSC.
    • This differs from the DSC spec that postulates that once populated for a trace, the DSC must not be mutated.
    • We did this on purpose for a experiment and never revisited the logic, hence it’s still active
    • Talked with Jan: Let’s leave it as is for now, document it well, but once things stabilize with new APIs and the major is out, we should reevaluate if the current data works for our server-side DS logic or if we should re-introduce immutability.
@Lms24 Lms24 changed the title Consolidate propagation and dynamic sampling context on Scope Consolidate propagation and dynamic sampling contexts on Scope Jan 8, 2024
@Lms24 Lms24 added this to the 8.0.0 milestone Jan 8, 2024
@Lms24
Copy link
Member Author

Lms24 commented Jan 8, 2024

To add: This is behaviourally breaking in the sense that loose/inactive transactions and root spans won't be able obtain a DSC with mutability guarantees in the same way as active transactions.

@Lms24
Copy link
Member Author

Lms24 commented Jan 9, 2024

Closing in favour of #10119

@Lms24 Lms24 closed this as completed Jan 9, 2024
@Lms24 Lms24 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
Lms24 added a commit that referenced this issue Jan 10, 2024
…ur of `getDynamicSamplingContextFromSpan` (#10094)

Deprecate `Transaction.getDynamicSamplingContext` and
introduce its direct replacement, a top-level utility function. Note
that this only is an intermediate step we should take to rework how
we generate and handle the DSC creation. More details in #10095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant