WIP: Potel sampling #3457
Closed
WIP: Potel sampling #3457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* create a new otel context `_SCOPES_KEY` that will hold a tuple of
`(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
* note that this is on `attach`, so not on all copy-on-write context
object creation but only on apis such as
[`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
* basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
* these will later be updated, for instance when we update our top
level scope apis that fork isolation scope, that will also have a
corresponding change in this `attach` function
* create a new otel context `_SCOPES_KEY` that will hold a tuple of
`(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
* note that this is on `attach`, so not on all copy-on-write context
object creation but only on apis such as
[`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
* basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
* these will later be updated, for instance when we update our top
level scope apis that fork isolation scope, that will also have a
corresponding change in this `attach` function
Add simple scope management whenever a context is attached
* create a new otel context `_SCOPES_KEY` that will hold a tuple of
`(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
* note that this is on `attach`, so not on all copy-on-write context
object creation but only on apis such as
[`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
* basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
* these will later be updated, for instance when we update our top
level scope apis that fork isolation scope, that will also have a
corresponding change in this `attach` function
* only acts on `on_end` instead of both `on_start/on_end` as before * store children spans in a dict mapping `span_id -> children` * new dict only stores otel span objects and no sentry transaction/span objects so we save a bit of useless memory allocation * I'm not using our current `Transaction/Span` classes at all to build the event because when we add our APIs later, we'll need to rip these out and we also avoid having to deal with the `instrumenter` problem * if we get a root span (without parent), we recursively walk the dict and find the children and package up the transaction event and send it * I didn't do it like JS because I think this way is better * they [group an array of `finished_spans`](https://github.com/getsentry/sentry-javascript/blob/7e298036a21a5658f3eb9ba184165178c48d7ef8/packages/opentelemetry/src/spanExporter.ts#L132) every time a root span ends and I think this uses more cpu than what I did * and the dict like I used it doesn't take more space than the array either * if we get a span with a parent we just update the dict to find the span later * moved the common `is_sentry_span` logic to utils
With this change, we aim to simplify the backwards-compatibility code
for POTel tracing.
We do this as follows:
- Remove `start_*` functions from `tracing`
- Remove unused parameters from `tracing.POTelSpan.__init__`.
- Make all parameters to `tracing.POTelSpan.__init__` kwarg-only.
- Allow `tracing.POTelSpan.__init__` to accept arbitrary kwargs,
which are all ignored, for compatibility with old `Span` interface.
- Completely remove `start_inactive_span`, since inactive spans can
be created by setting `active=False` when constructing a
`POTelSpan`.
* New `PotelScope` inherits from scope and reads the scope from the otel context key `SENTRY_SCOPES_KEY` * New `isolation_scope` and `new_scope` context managers just use the context manager forking and yield with the scopes living on the above context key * isolation scope forking is done with the `SENTRY_FORK_ISOLATION_SCOPE_KEY` boolean context key
❌ 95 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
|
Closing in favor of: #3501 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sampling using Potel
Thank you for contributing to
sentry-python! Please add tests to validate your changes, and lint your code usingtox -e linters.Running the test suite on your PR might require maintainer approval. The AWS Lambda tests additionally require a maintainer to add a special label, and they will fail until this label is added.