-
-
Notifications
You must be signed in to change notification settings - Fork 151
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(sentry-tracing): switch sentry spans on enter and exit #686
base: master
Are you sure you want to change the base?
Conversation
7a4b72c
to
a138e3c
Compare
a138e3c
to
3636ca1
Compare
a138e3c
to
7a4b72c
Compare
7a4b72c
to
6f19f1d
Compare
For those cases, one should usually fork the Sentry Hub in order to have them fully isolated. While you are right that enter/exit is a better fit, and when someone is manually hooking up spans, this distinction can lead to some confusion. |
Hi @saiintbrisson, are you able to solve this problem with hub isolation? I would honestly be a bit hesitant to change how we enter and exit Sentry spans, since others might be relying on the current behavior. But, we can look into it if you are unable to find a way to work around this problem. |
I ended up just using my fork at my company. Hub isolation is pretty verbose as I would have to bind it to every future as it currently stand. I already isolate it for my root future (I have a Axum middleware binding a hub to the handler call). The problem arises on child futures, like Probably manually binding a hub to every call inside my join would work, but I don't want to make my coworkers handle this kind of detail during development. We have a couple of hundreds of mentions of If this happens to be a wont-fix, I'll probably just create my own tracing layer with this impl, but I do think |
Yes that is a bit unfortunate, but ultimately the correct thing to do. Apart from proper span hierarchy, this also relates to Sentrys Scope and other concepts, which could go out of sync if multiple concurrent futures are messing with them. |
I don't think I understand how Scopes are to be used. I currently create a new Hub, create a transaction and set it to the created Hub, that I than use to bind it to the handler call (one scope per request). The way you described it, it seems like every future should have its own Scope? In any case, I expected Let me know if you folks come to any conclusion about this PR. For now, I'll create my own layer impl and move the Thanks for your time and keep up the good work, we've been enjoying Sentry a lot around here :). |
All in all, thank you for the great feedback, this is very valuable.
I might have added a bit to the confusion here. The Rust SDK still has a couple of concepts that have since been removed from the wider "Sentry unified API". For example, there are no Anyhow, in essence you want every future to have its own Scope (which in the Rust SDK is still named
The As I mentioned, the Rust SDK is quite a bit behind in terms of features. By now, the Sentry JS SDK has fully embraced opentelemetry, the SDK and instrumentation side that is. I could maybe imagine Sentry doing something similar in the sense of not duplicating all this span tracking, etc, but fully embracing |
I think this is useful even outside of Futures. My application doesn't do any async but whenever we create a new thread we also create a new tracing span for that thread. The span is created before the new thread (to inherit the current span as the parent) but then only enter it inside the new thread. If I am understanding this issue correctly, currently any events between the creation of the span and the entering of the span will have the wrong span information. While I understand this is a change in behavior I think the current behavior is fairly unexpected and not intuitive. |
I've been facing a bug with Sentry Trace where concurrent spans end up wrongly nested. At first, I thought this was some strange autogrouping bug and event sent a feedback (sorry Sentry support member 😅).
After digging a little bit, I discovered that the current tracing layer implementation sets the sentry span as active as soon as a tracing span is created, which should not be the case, as spans are only active once entered. Following the same logic, it also reverts to the parent span only when a span is closed, and not when it's exited.
A span can be entered and exited multiple times, for example, when a tracing
Instrumented
future polls and returns Pending. In case of concurrent Instrumented futures, the current impl ends up nesting Futures under the first created one.sentry-tracing
layer (wrong)tracing-opentelemetry
layer (correct)