Fill out more property/method stubs #3441
Merged
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 730 tests with View the full list of failed testspy3.10-potel
py3.11-anthropic-v0.25
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.12-aiohttp-latest
py3.12-anthropic-latest
py3.12-anthropic-v0.25
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-v0.23
py3.7-boto3-v1.23
py3.7-bottle-latest
py3.7-gql-latest
py3.7-gql-v3.4
py3.7-grpc-v1.49
py3.8-aiohttp-latest
py3.8-grpc-latest
py3.8-potel
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
|
antonpirker
approved these changes
Aug 13, 2024
sentry_sdk/tracing.py
Outdated
| self.set_tag( | ||
| "http.status_code", str(http_status) | ||
| ) # we keep this for backwards compatibility | ||
| # XXX do we still need this? ^ |
There was a problem hiding this comment.
I think we can remove this.
Relay checks the tag and the span.data: https://github.com/getsentry/relay/blob/b561ec339a5b1d351f016dd0f8bb01a910821acf/relay-event-normalization/src/normalize/utils.rs#L29
But when removing this we need to make it clear in the migration docs, because people could have alerts or dashboards on that tag
There was a problem hiding this comment.
Removed in 7174739
sentry_sdk/tracing.py
Outdated
Comment on lines
1295
to
1296
| self.op = op | ||
| self.description = description |
There was a problem hiding this comment.
Just for my understanding: why is the op and description handled differently?
There was a problem hiding this comment.
Do you mean the is not None check that's there for op but not description?
This should probably be unified. I wasn't sure about the semantics of setting op or description to None (would anyone want to do that?). More importantly, OTel doesn't really support None as a valid value for span attributes (see also this comment).
So I guess the correct way to go about this would be:
- if an attribute like
opordescriptionis set toNone: do nothing (deleting attributes is not really a thing in OTel as far as I can tell) - if it's set to something non-
None, set the attr
So I'd move this logic to check whether the value is not None to the actual properties for all attrs where it makes sense instead of having it here in the __init__.
sl0thentr0py
approved these changes
Aug 13, 2024
| @@ -11,6 +11,8 @@ Looking to upgrade from Sentry SDK 2.x to 3.x? Here's a comprehensive list of wh | |||
|
|
|||
| ### Removed | |||
|
|
|||
| - When setting span status, the HTTP status code is no longer automatically added as a tag. | |||
There was a problem hiding this comment.
very good point you raise here,
let's make sure to have a follow up with relay to make sure we agree on the 'right way' going forward, both the SDK side and the relay side has a whole bunch of weird logic for getting the status from various sources.
Let's simplify if possible. cc @AbhiPrasad
There was a problem hiding this comment.
ah ok @antonpirker said the same
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.
Add more stubs for things expected on
Spans, fill out some existing stubs.We will have to clean up the inline imports at some point.
Depends on the
convert_to_otel_timestamputil added in https://github.com/getsentry/sentry-python/pull/3436/files