sentry-rust icon indicating copy to clipboard operation
sentry-rust copied to clipboard

async with_scope

Open Roguelazer opened this issue 3 years ago • 5 comments

I have an async HTTP server and I'd like to attach transaction information to sentries caught during request processing. This isn't actually straightforward right now because the corresponding method (with_scope) is synchronous. What about an async version? I'm thinking of a function with the given signature:

pub async fn with_scope_async<C, F, R>(scope_config: C, callback: F) -> R where
    C: FnOnce(&mut Scope),
    F: Future<Output=R>;

This would run the given future to completion, setting the scope returned by C as the current scope during each poll.

Here's the prototype I'm using now (which may not be terribly efficient, since it reconstructs the scope from scratch on every poll, which involves a lot of cloning):

use std::future::Future;
use std::pin::Pin;
use std::task::{Context, Poll};

pin_project_lite::pin_project! {
    struct WithScopeAsyncFuture<F, C> {
        #[pin]
        inner: F,
        scope_fn: C,
    }
}

impl<F, R, C> Future for WithScopeAsyncFuture<F, C>
where
    F: Future<Output = R>,
    R: Send,
    C: Fn(&mut sentry::Scope),
{
    type Output = R;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        let this = self.project();
        sentry::with_scope(this.scope_fn, || this.inner.poll(cx))
    }
}

fn with_scope_async<F, R, C>(scope_config: C, callback: F) -> WithScopeAsyncFuture<F, C>
where
    C: Fn(&mut sentry::Scope),
    F: Future<Output = R>,
    R: Send,
{
    WithScopeAsyncFuture {
        inner: callback,
        scope_fn: scope_config,
    }
}

Roguelazer avatar Jan 04 '22 23:01 Roguelazer

Well this is a nice coincidence. We brought up the potential footgun with with_scope and async code internally recently.

The conclusion there was to rather advocate for using push_scope. In rust that returns a scope guard with a Drop impl, so it will pop the scope when it goes out of scope. That way it will naturally work with async code, no need for a custom future.

As for your code example. If you really want to go with with_scope_async and a custom future. Make sure to use push_scope, and keep the guard around in your future. You can then call the scope_config callback right in the beginning with configure_scope. IMO calling that callback multiple times for each poll creates more problems than it would solve.

Swatinem avatar Jan 05 '22 09:01 Swatinem

It looked to me like push_scope maintained a thread-local stack, so it wouldn't work if you reused the same Guard across multiple yield points (because they might be interleaved in a non-stack way). I would think that, in order for it to work, the stack would need to be stored on something like tokio::task_local instead. Did I miss something in my analysis?

Roguelazer avatar Jan 05 '22 17:01 Roguelazer

In fact, I just verified that a fairly naive implementation like

async def handle_request() {
    let hub = sentry::Hub::current();
    let guard = hub.push_scope();
    hub.configure_scope(...);
    // do a bunch of async stuff with yield points
    drop(guard);
}

results in

thread 'tokio-runtime-worker' panicked at 'Tried to pop guards out of order', /Users/jbrown/.cargo/registry/src/github.com-1ecc6299db9ec823/sentry-core-0.23.0/src/scope/real.rs:142:17

Roguelazer avatar Jan 05 '22 17:01 Roguelazer

It might be possible that you are mis-using hubs. We stumbled across this problem recently as well, that hubs in async contexts provide quite a footgun. Especially if you have concurrent futures (things like futures::join), you have to bind a cloned hub to each of those futures, otherwise the concurrent futures are all manipulating the same hub and stepping on each others toes.

Swatinem avatar Jan 10 '22 09:01 Swatinem

I worked around this by using task_local! in combination with the before_send hook.

the before_send:

before_send: Some(Arc::new(|mut event| {
    SENTRY_THING_ID.with(|sentry_thing_id| {
      event.extra.insert("thing_id".to_string(), json!(sentry_thing_id));
});
      Some(event)
})),

and the declaration for the task_local

tokio::task_local! {
    pub static SENTRY_THING_ID: String;
}

and wrap the function in :

    SENTRY_THING_ID.scope("hi-scoped".to_string(), async move {
        ...
    }).await

I could not see that it would work otherwise, since the hub uses or a process or a thread local hub.

klaasjanelzinga avatar Nov 20 '22 11:11 klaasjanelzinga