search

intgr / rocket-sentry

Simplifies integration between the Rocket web framework and Sentry application monitoring system.
MIT License
9 stars 10 forks source link

I don't think this fairing can function as intended #24

Open svenstaro opened 2 years ago

svenstaro commented 2 years ago

Hey, I think the Fairing in its current form doesn't do nearly enough to properly handle Sentry. Currently state gets mixed up between requests as all share the same Hub. You'd have to make a Hub per request and then make sure that Sentry uses that particular Hub for the whole duration of that request unless I'm missing something.

See examples of this in the official sentry-tower and sentry-actix integrations.

I don't actually think it's currently possible to do this in the current version of Rocket as I don't think Fairings are a powerful enough abstraction as we can't access the hyper service which we'd need to do in order to attach our Hub via bind_hub.

Maybe you can figure something out but I think it's rather hard.

intgr commented 2 years ago

Agreed, there is lots of room for improvement here.

As for "functioning as intended", my intent was to gain visibility whether there were any panics in my web application. My webapp is quite simple and there have been literally zero panics (apart from ones generated during testing), there hasn't been any motivation to improve this further.

Given that background, I feel like I'm not the right person to develop/maintain this crate. If someone with the right motivations and skills comes along, I would gladly hand over rights to this crate.

But if that doesn't happen, I will continue maintaining dependency versions myself.

PS: There's also the sentry_rocket crate, but it doesn't handle the request lifecycle either.

ondrowan commented 2 years ago

Just thinking out loud since I haven't actually tried it, but shouldn't combination of Fairing::on_request and request-local state (Request::local_cache) solve this problem?

svenstaro commented 2 years ago

I looked into this once and I don't think it'll work even using that though I admit I don't remember why. I'd certainly welcome a solution though and would encourage you to attempt this.