feat(potel): Make tracing APIs use OTel in the background #3242
Conversation
This file contains 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
| @@ -41,6 +41,7 @@ def get_file_text(file_name): | |||
| install_requires=[ | |||
| "urllib3>=1.26.11", | |||
| "certifi", | |||
| "opentelemetry-distro>=0.35b0", # XXX check lower bound | |||
There was a problem hiding this comment.
let's reconsider this separately before shipping, we really don't want to force the otel dependency on errors-only users, so this needs some thought.
Basically, if people want to use performance, we need to force them to do pip install sentry-sdk[opentelemetry],
There was a problem hiding this comment.
|
|
||
| # XXX The wrapper itself should have as little state as possible | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
js changed the api args completely, we will also need to consider in what steps we change the options for this constructor
https://github.com/getsentry/sentry-javascript/blob/af760a73b9304d1f7dce93458a9377e1bd1c5a39/packages/types/src/startSpanOptions.ts#L4-L53
|
|
||
| def __enter__(self): | ||
| # type: () -> POTelSpan | ||
| # XXX use_span? https://github.com/open-telemetry/opentelemetry-python/blob/3836da8543ce9751051e38a110c0468724042e62/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547 |
There was a problem hiding this comment.
i'd prefer use_span here too because it does a bit of exception/status logic on its own
|
added some basic test cases here that we can build upon |
szokeasaurusrex
force-pushed
the
potel-base
branch
from
d4b1d00 to
4428ee9
Compare
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`.
❌ 3908 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 |
* 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
sentrivana
force-pushed
the
ivana/potel/start-span
branch
from
f98614d to
cb6b686
Compare
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.
start_span,start_transactioncreate OTel spans in the background viastart_as_current_span(orstart_spanwith explicit context management)start_span,start_transactionas one would with a Sentry span/transaction (i.e., find a way to keep the API the same)also contains
start_transaction#3379Simple test
References
startSpan& friendsstart_span& friendsMisc
In OTel, this:
is equivalent to
closes #3212