Fixing start_transaction
#3379
Merged
Fixing start_transaction
#3379
+13
−32
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
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 860 tests with View the full list of failed testspy3.10-potel
py3.11-aiohttp-latest
py3.11-aiohttp-v3.8
py3.11-anthropic-latest
py3.11-anthropic-v0.16
py3.11-ariadne-latest
py3.11-ariadne-v0.20
py3.11-arq-latest
py3.11-arq-v0.23
py3.11-boto3-latest
py3.11-boto3-v1.34
py3.11-clickhouse_driver-v0.2.0
py3.11-grpc-latest
py3.11-opentelemetry
py3.11-trytond-latest
py3.12-aiohttp-latest
py3.12-anthropic-latest
py3.12-anthropic-v0.16
py3.12-ariadne-latest
py3.12-arq-latest
py3.12-asgi
py3.12-boto3-latest
py3.12-boto3-v1.34
py3.12-grpc-latest
py3.7-aiohttp-v3.4
py3.7-anthropic-latest
py3.7-anthropic-v0.25
py3.7-arq-latest
py3.7-arq-v0.23
py3.7-boto3-v1.12
py3.7-falcon-latest
py3.7-graphene-latest
py3.7-graphene-v3.3
py3.7-grpc-v1.59
py3.7-opentelemetry
py3.8-aiohttp-latest
py3.8-ariadne-latest
py3.8-ariadne-v0.20
py3.8-clickhouse_driver-v0.2.0
py3.8-grpc-latest
py3.8-potel
py3.8-trytond-latest
py3.9-aiohttp-v3.8
py3.9-cohere-latest
py3.9-cohere-v5
py3.9-grpc-v1.39
py3.9-httpx-latest
py3.9-opentelemetry
py3.9-rq-v0.13
|
sl0thentr0py
requested changes
Jul 31, 2024
sentry_sdk/api.py
Outdated
| global _potel_span_init_kwargs | ||
|
|
||
| if _potel_span_init_kwargs is None: | ||
| _potel_span_init_kwargs = inspect.signature(tracing.POTelSpan).parameters.keys() |
There was a problem hiding this comment.
heavily against this kind of code, sorry
please write stupid code and list all the args, no introspection is necessary
There was a problem hiding this comment.
This kind of obfuscates what's going on -- I'd also prefer us to simply be stupidly explicit here since it makes it clearer what is happening.
There was a problem hiding this comment.
Fair enough, my reasoning was to avoid needing to update the arguments list in two places if it ever changes. But this way, it is perhaps less clear what is going on, so I will make this more explicit, as suggested.
szokeasaurusrex
force-pushed
the
szokeasaurusrex/potel/start-span
branch
from
04b3ce8
to
f04f589
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`.
szokeasaurusrex
force-pushed
the
szokeasaurusrex/potel/start-span
branch
from
f04f589
to
2064e0c
Compare
szokeasaurusrex
changed the base branch from
potel-base
to
ivana/potel/start-span
szokeasaurusrex
commented
Jul 31, 2024
|
@sl0thentr0py and @sentrivana, I addressed the PR comments, and rebased the PR on |
szokeasaurusrex
force-pushed
the
szokeasaurusrex/potel/start-span
branch
from
2064e0c
to
4668c3e
Compare
sentrivana
added a commit
that referenced
this pull request
Aug 26, 2024
* 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 <neel.shah@sentry.io>
Co-authored-by: Neel Shah <neelshah.sa@gmail.com>
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
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.
This is branched off of
ivana/potel/start-spanand rebased on latestpotel-base. I added commit 971642b to (hopefully) fixstart_transaction.