-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rust tracing #3817
Fix rust tracing #3817
Conversation
f7a6c1f
to
5870c74
Compare
sentry_span = parent_sentry_span.start_child(**kwargs) | ||
else: | ||
sentry_span = scope.start_span(**kwargs) | ||
span = sentry_sdk.start_span( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matt-codecov @antonpirker I simplified this part since we don't want users to do scope stuff in integrations, it is anti-patterny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a quick look at the code, it looks like __enter__()
will still update the current span and keep track of the parent so that __exit__()
can restore it, right? if so then i am fine with this. not qualified to stamp, and also CI is unhappy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes exactly, span management should be done in the core and will just work
ci is unhappy because this is a dev branch
❌ 922 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 |
PR title says "fix" - is this broken in 2.19? also why does this need POtelSpan? to be clear, the Rust side of this is not OTEL |
this is a very large refactor of the SDK to use otel under the hood for the next major. And yes it's fine that the rust side is not otel. You can ignore all the other stuff tbh, I just pinged you to take a look at the changes because I was changing your integration. |
oh interesting the |
possibly! we can investigate that once we ship this and if it's feature compatible, we can remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.