cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
21.64k stars 1.2k forks source link

Support for custom option in place of sentry for catching run time panic #352

Open ats1999 opened 2 months ago

ats1999 commented 2 months ago

What is the problem your feature solves, or the need it fulfills?

pingora provides default integration with sentry for caching run time panic. Here is a related issue https://github.com/cloudflare/pingora/issues/188. As a framework, pingora should be open to any other integration for logging. Looks like we'll have to switch to sentry for panic logging.

Describe the solution you'd like

Any plan to make it configurable to use any other solution?

Describe alternatives you've considered

Na

Additional context

NA

drcaramelsyrup commented 2 months ago

The existing Sentry integration has worked for us, which is why there aren't plans to integrate other error handling solutions on our roadmap. Still, we are open to adding support for them.

ats1999 commented 2 months ago

@drcaramelsyrup I ment, we don't use sentry in our stack. We have some other existing solution. We also want to use pingora, but adding sentry for using pingora is challanging for us.

As a framework pingora should be open to any other tool for logging.

What do you think?

ats1999 commented 1 month ago

I can work on it if it can be merged, LMK @drcaramelsyrup

drcaramelsyrup commented 1 month ago

Sure, like I mentioned we're open to it and not trying to be exclusive to Sentry. Correct me if I'm wrong but sounds like you'd add something like the ability to set an optional panic hook, which could be useful to others too.

ats1999 commented 1 month ago

ability to set an optional panic hook

IMO the best possible solution would be to remove the dependency on sentry and allow users to use logging system they want.

As a framework, pingora can provide hook/event for errors in place of sentry as you said.

What’s your take on this?

eaufavor commented 1 month ago

Could you help us understand what problem sentry causes you.

Is sentry as a code dependency an issue? Sentry is only initialized if there is corresponding configuration set. Otherwise it should be inert. You can set up your own panic hook. The Pingora framework doesn't restrict you from doing so. Here is an example I just tested on https://github.com/cloudflare/pingora/blob/main/pingora/examples/server.rs .

pub fn main() {
    std::panic::set_hook(Box::new(|_| {
        println!("Custom panic hook");
    }));
   // the rest pingora framework logic
   // my_server.run_forever();
ats1999 commented 1 month ago

Sentry is not just being used for panic hook, it's being used for reporting errors as well. Check this line https://github.com/cloudflare/pingora/blob/d425379ae35b4265b23acdc829c29f0776a91c1d/pingora-core/src/server/mod.rs#L120

Is there any way to log these errors somewhere else? I would like to log errors to kafka, how can i do that? Let me know if there is any other way to log errors to user defined destination

Thanks for reverting on the issue

eaufavor commented 1 month ago

You are right. There are two places we directly call sentry to capture the errors. Both are to capture very cornered reload/restart failures. Nothing else is tied to sentry.

All runtime request level errors are reported here, where you can plug in your custom error reporting logic https://github.com/cloudflare/pingora/blob/main/docs/user_guide/modify_filter.md#logging