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

Rework sampling_context #3746

Open
8 of 12 tasks
sentrivana opened this issue Nov 7, 2024 · 1 comment
Open
8 of 12 tasks

Rework sampling_context #3746

sentrivana opened this issue Nov 7, 2024 · 1 comment
Assignees

Comments

@sentrivana
Copy link
Contributor

sentrivana commented Nov 7, 2024

(custom_)sampling_context is a Sentry-specific concept that doesn't fit with how OTel works.

The purpose of a sampling_context is to provide extra external data to use for decision making in the traces_sampler. Right now, we're using it to make things like the transaction name, parent sampled decision, request object or queue name accessible in the sampler.

The goal is to store everything necessary on the span as attributes instead and expose those. However, since attribute types are very limited, we can't just save a custom object like e.g. the request as an attribute. Instead, we need to preprocess it into serializable attributes. For each integration, we need to see what important data we can extract and save on the span.

The idea:

  • no custom_sampling_context argument for start_transaction/start_span anymore
  • start_span instead gets an extra attributes argument which, when provided, prepopulates the attributes
  • integrations that were providing a custom_sampling_context will instead provide a set of prepopulated attributes (that can't be objects)
  • the sampling_context accessible in traces_sampler will change:
    • it'll contain all span attributes by default
    • it'll also contain the current data (transaction_context.name, transaction_context.op, parent_sampled) in some slightly modified form

Integrations that are currently setting a custom_sampling_context that will now have to set specific attributes:

Related:

@antonpirker
Copy link
Member

This is a user facing feature: https://docs.sentry.io/platforms/python/configuration/sampling/

So we need to update the docs too if we remove it and also have it in the migration guide

@sentrivana sentrivana changed the title Remove custom_sampling_context Remove sampling_context Nov 7, 2024
@sentrivana sentrivana changed the title Remove sampling_context Rework sampling_context Nov 7, 2024
sentrivana added a commit that referenced this issue Nov 12, 2024
- Add support for the sampled flag for start_span and respect it when making sampling decisions.
- Rework sampling_context in traces_sampler to work with span attributes instead. Make sure we still have the same data accessible as now.

We could go one step further and change the format of sampling_context to just be the actual span attributes without any postprocessing into the current format. I kept the format in line with what we have now to make it easier to update.

See #3746
Closes #3739

This is a breaking change since we're removing custom_sampling_context. It'll break multiple integrations until we fix them (see #3746).
@sentrivana sentrivana self-assigned this Nov 18, 2024
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

2 participants