POtel implementation base branch #3152
Draft
+5,325
−6,762
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
sl0thentr0py
force-pushed
the
potel-base
branch
2 times, most recently
from
f7f153c
to
28effd6
Compare
sl0thentr0py
force-pushed
the
potel-base
branch
2 times, most recently
from
16f9341
to
951477f
Compare
❌ 1578 Tests Failed:
View the top 2 failed tests by shortest run time
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
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
szokeasaurusrex
force-pushed
the
potel-base
branch
from
d4b1d00
to
4428ee9
Compare
Remove `instrumenter` parameter from all functions that accept it
(details below), and modify tests to not pass the `instrumenter`
parameter to any functions that used to take it.
Also, delete `tests/tracing/test_noop_span.py`, which tests
functionality removed in this commit.
BREAKING CHANGE:
- Remove `sentry_sdk.init`'s `instrumenter` kwarg.
- Delete `sentry_sdk.contsts.INSTRUMENTER` class.
- Remove `sentry_sdk.hub.Hub.start_span`'s `instrumenter` parameter.
- Remove `sentry_sdk.hub.Hub.start_transaction`'s `instrumenter`
parameter.
- Remove `sentry_sdk.scope.Scope.start_transaction`'s `instrumenter`
parameter.
- Remove `sentry_sdk.scope.Scope.start_span`'s `instrumenter`
parameter.
- Remove `sentry_sdk.tracing.Span.start_child`'s `instrumenter`
parameter.
- Remove `sentry_sdk.tracing.NoOpSpan.start_child`'s
`instrumenter` parameter.
Closes: #3321
Improved extraction of op, description, status and http_status for a Sentry span from an OpenTelemenetry span. Fixes #3236 --------- Co-authored-by: Neel Shah <[email protected]> Co-authored-by: Ivana Kellyerova <[email protected]>
antonpirker
changed the title
Also, remove any tests for `sentry_sdk.configure_scope`. Since Strawberry's deprecated [Sentry tracing extensions](https://strawberry.rocks/docs/extensions/sentry-tracing) import `sentry_sdk.configure_scope`, importing `strawberry.extensions.tracing.SentryTracingExtension` (or `SentryTracingExtensionSync`) will result in an unhandled exception. Therefore, these imports, and any functionality associated with them, have also been removed. This itself is not a breaking change, as it is necessitated by the removal of `sentry_sdk.configure_scope`. BREAKING CHANGE: Remove `sentry_sdk.configure_scope`. Closes: #3402
Also, remove any tests that test `sentry_sdk.push_scope`. BREAKING CHANGE: Remove `sentry_sdk.push_scope`. Closes #3403
This change is a prerequisite for #3404. BREAKING CHANGE: Remove `sentry_sdk.transport.HttpTransport`'s `hub_cls` attribute.
* Skeletons for new components
* 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
* Don't parse DSN twice
* wip
* Skeletons for new components
* Skeletons for new components
* 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
* mypy fixes
* working span processor
* lint
* Port over op/description/status extraction
* defaultdict
* naive impl
* wip
* fix args
* wip
* remove extra docs
* Add simple scope management whenever a context is attached (#3159)
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
* Implement new POTel span processor (#3223)
* 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
* Basic test cases for potel (#3286)
* Proxy POTelSpan.set_data to underlying otel span attributes (#3297)
* ref(tracing): Simplify backwards-compat code (#3379)
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 Scope implementation based on OTel Context (#3389)
* 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
* Fix circular imports (#3431)
* Random tweaks (#3437)
* Origin improvements (#3432)
* Tweak OTel timestamp utils (#3436)
* Create spans on scope (#3442)
* Fill out more property/method stubs (#3441)
* Cleanup origin handling and defaults (#3445)
* add note to migration guide
* Attribute namespace for tags, measurements (#3448)
---------
Co-authored-by: Neel Shah <[email protected]>
Co-authored-by: Neel Shah <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
* Don't default to OK span status for UNSET, this breaks older behavior * Set transaction name manually on scope after isolation (we should generally do this everywhere)
…3748) If this option is on, we will only create a new underlying otel span if there's an active valid parent, otherwise we will just return an invalid `NonRecordingSpan` (`INVALID_SPAN`). All internal integration child `start_span` calls have been modified so that now we will only create spans if there is an active root span (transaction) active.
mainly some middleware span reordering
* Fix transaction name setting and forked some tests to make them work in potel * Transactions in transactins is undefined behavior, so remove this.
- 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).
Remove explicit trace_id passing
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.
Contains:
only_if_parentoption to POTelSpan and use it in integrations #3748_serialize_span_attributeintoset_attribute#3732subprocessbreadcrumbs frommaybe_create_breadcrumbs_from_spanto integration. #3637addto trace state if key does not exist #3645op. #3628make aws lambda layer#3586use_scope,use_isolation_scope#3522use_scope,use_isolation_scope#3500OpenTelemetryIntegrationtoDEFAULT_INTEGRATIONS#3471activeflag onPOTelSpan#3470start_transaction#3379Huband related code for good. #3446Simple test
References
startSpan& friendsstart_span& friendsMisc
In OTel, this:
is equivalent to