Skip to content
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

tracing::Instrument::instrumented futures don't propagate to Sentry #675

Open
Ten0 opened this issue Jul 25, 2024 · 2 comments
Open

tracing::Instrument::instrumented futures don't propagate to Sentry #675

Ten0 opened this issue Jul 25, 2024 · 2 comments

Comments

@Ten0
Copy link
Contributor

Ten0 commented Jul 25, 2024

It seems that the sentry_tracing Layer doesn't take into account the on_enter event provided by tracing for Instrumented futures:

impl<S> Layer<S> for SentryLayer<S>

This implies that Sentry scopes are not properly entered and exited as tracing scopes are entered/exited.

It seems that the Span's existence and finish should be controlled by the existing on_new_span/on_close, but scope configuration should be tied to on_enter/on_exit instead of also being controlled by on_new_span and on_close.

@Swatinem
Copy link
Member

Conceptually, a Sentry Scope is not tied to tracing spans (or Sentry Spans for that matter) at all.

As you mention Futures and their lifecycle, you must use the SentryFuture wrapper as well, using .bind_hub(), otherwise concurrent futures will get a messed up Scope. Unfortunately, its a bit manual to get this right.

@Ten0
Copy link
Contributor Author

Ten0 commented Jul 29, 2024

As you mention Futures and their lifecycle, you must use the SentryFuture wrapper as well, using .bind_hub(), otherwise concurrent futures will get a messed up Scope. Unfortunately, its a bit manual to get this right.

I have noticed that, and there is the same thing in tracing: futures are made to correspond with span entering with the Instrument trait, which also needs to be called manually.

Conceptually, a Sentry Scope is not tied to tracing spans (or Sentry Spans for that matter) at all.

Looking at the concepts of both projects, it looks like there is:

  1. Sentry TransactionOrSpan & Tracing spans that represent duration of execution of something
  2. Sentry Scopes & Tracing "spans enters" that represent context of execution of code (I'm executing this code in the context of this span, which allows sentry to link events to spans (along with other things), and for tracing allows tools like tokio/console to work)

So I'm struggling to understand why the best mapping for a tracing integration wouldn't be to map 1 to 1 and 2 to 2 rather than leaving tracing's 2 unmapped and mapping part of tracing's 1 to Sentry's 2 which works incorrectly in multi-threaded executor contexts.
Mapping these would allow people who instrument their futures with #[tracing::instrument] async fn to not need to also re-instrument them in Sentry's concepts to get both "execution contexts" to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants