search

getsentry / sentry-rust

Official Sentry SDK for Rust
https://sentry.io/
Apache License 2.0
620 stars 153 forks source link

`tracing::Instrument::instrument`ed futures don't propagate to Sentry #675

Open Ten0 opened 4 months ago

Ten0 commented 4 months ago

It seems that the sentry_tracing Layer doesn't take into account the on_enter event provided by tracing for Instrumented futures: https://github.com/getsentry/sentry-rust/blob/8cc4848f115152af6f2ad66d7aeb17bd3bbe38fd/sentry-tracing/src/layer.rs#L153

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 commented 3 months ago

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 commented 3 months ago

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.