Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

Add Dynamic Sampling Context + Baggage Spec #611

Closed
smeubank opened this issue Jun 20, 2022 · 9 comments · Fixed by #613
Closed

Add Dynamic Sampling Context + Baggage Spec #611

smeubank opened this issue Jun 20, 2022 · 9 comments · Fixed by #613
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@smeubank
Copy link
Member

smeubank commented Jun 20, 2022

In Develop Docs

Write down baggage propagation “spec” to define clear rules of how baggage is created and passed along downstream.

clarify confusion around naming:

  • trace context vs baggage
  • "propagation mechanism"
  • content - what is in it?
    • call it dynamic sampling context
    • meaning: the key values included in the baggage header

The following list is probably not complete but it should include (writing this down after the meeting on Daily from 18th):

  • Check if incoming request has baggage header and parse it to see if there’s sentry- entries in it
  • If not, create the baggage content (i.e. the sentry- keys) and propagate it with the outgoing requests
    • include the specific contect types (release, environment, user, transaction, usergroup)
  • If yes, leave it untouched (don’t add/change stuff) and send it along with the outgoing request
  • In any of those two cases, “foreign” baggage entries must be passed along with the next outgoing requests (if they exist)

reference existing docs

#425

#409

@smeubank smeubank added the documentation Improvements or additions to documentation label Jun 20, 2022
@smeubank
Copy link
Member Author

Problem:
As customer adopt DS with in product Sentry server side traces and transaction sample rate rules, then they should change the sample rate in their applications. In the situations where there is distributed tracing, there could be conflicting sample rates on different projects, leading to issues where we cannot provide DS

where should it be sent?

  • in dynamic sample context (by baggage)
  • in the envelope header
  • meta data (today in JS, but will become unnecessary)

Does what we sent in the envelope header need to be same as http header (baggage)?

  • where does the public key need to be?

@antonpirker
Copy link
Member

antonpirker commented Jun 21, 2022

A list of PRs that @Lms24 sent me with all the baggage functionality in JS. Maybe this helps writing the documentation?

  1. Baggage data structures and operations

  2. Baggage data in Envelope Header

  3. Baggage passing on to outgoing requests (without data population)

  4. Release and Environment propagation

  5. Mutability of Baggage:

@smeubank
Copy link
Member Author

getsentry/sentry-javascript#5290 (comment)

for reference of what concerns to address see this comment. Notes from SDK DS sync. Transactions have platform specific complexities. Maybe a table where devs can include descriptions and examples of such can be included

@smeubank
Copy link
Member Author

confirming list of relevant values:

Trace-aware head sampling based on trace state information from the frontend SDK (requires SDK support):

  • Transaction name
  • Release
  • Environment
  • User ID
  • User Segment
  • No condition - uniform sampling rate

@ale-cota can you clarify what "No condition" means?

@Lms24
Copy link
Member

Lms24 commented Jun 21, 2022

@smeubank to add on the list posted above:

@lforst and I talked about names/keys of values to be propagated via baggage and sent via an envelope header. He'll include a detailed list once his PR is ready but this is what we came up with:

Baggage keys:

  • sentry-traceid
  • sentry-publickey
  • sentry-samplerate
  • sentry-environment
  • sentry-release
  • sentry-transaction
  • sentry-userid
  • sentry-usersegment

Items in trace envelope header
Envelope header items for trace header are described in Develop docs as well as implemented in Relay.
Additionally, in Relay a field must be added for the sample rate, which we propose to call

  • sample_rate (for consistency with other fields)

@AbhiPrasad
Copy link
Member

As part of these docs, I would like to take some time to establish the terms we refer to these different concepts.

Traces, transactions, and spans have all been long defined in our product docs.

In event payload (so both errors and transactions), there exists the contexts interface, one of which is the Trace Contexts (a contexts keyed by trace). I believe this should continue to be referred to as such.

We then have the key-value bag needed to make dynamic sampling decisions. Previously this was called Trace Contexts also, but I propose it be called Dynamic Sampling Context (thanks @lobsterkatie for the suggestion). This dynamic sampling context is used in two ways:

  1. It is sent to Sentry through the envelope header, keyed by trace.

  2. It is propogated between services (and so between different Sentry SDKs running in those services) so that dynamic sampling can occur on entire traces. Currently the mechanism for propogation we have chosen for the SDKs is leveraging the baggage HTTP header, based on the w3c baggage spec.

@mitsuhiko
Copy link
Contributor

I like the idea of calling this dynamic sampling context, so that gets a buy-in from me. Annoyingly on the relay side the TraceContext and the dynamic sampling context are in fact synonyms right now I believe. I think that in the protocol it's in fact currently the case that the trace item in the envelope headers have exactly the same format as the trace context in the protocol but someone from relay would need to double check that.

@AbhiPrasad AbhiPrasad changed the title [DEV-DOCS] Add Baggage Spec Add Dynamic Sampling Context + Baggage Spec Jun 21, 2022
@lforst
Copy link
Contributor

lforst commented Jun 22, 2022

I just opened #613 to get this specced. It would be cool if we could get some early feedback.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants