-
Notifications
You must be signed in to change notification settings - Fork 56
#66 work in progress make print_panic_info() public #67
Conversation
Nice one @yaahc looks like you did all the hard work! |
heh, ya, I got nerd snipped a little and then immediately after a friend asked about passing panics to tracing so I decided to not wait around and test out some things myself 😅 |
@yaahc anything I can do to help with the PR to get it through? |
ooh, I can take care of merging it, just slipped thru the cracks is all, ty for the reminder. |
I've been doing some more testing with this, and some things which would be nice, to be able to control backtrace, spantrace, and color formatting for the eyre report display invocation that gets used for sending data to sentry. At the moment I have (along with a bunch of other code setting up tracing and sentry): let (eyre_panic_hook, eyre_hook) = color_eyre::config::HookBuilder::default()
.into_hooks();
eyre_hook.install()?;
std::panic::set_hook(Box::new(move |panic_info| {
eprintln!("{}", eyre_panic_hook.panic_report(panic_info));
sentry::integrations::panic::panic_handler(panic_info);
})); Basically, it would be nice to have colours and user selectable backtrace/spantrace verbosity for what gets printed to the terminal, and for what gets sent to sentry, no colours and full verbosity on backtrace/spantrace. I've tried setting/resetting env variables on the fly but it doesn't seem to make any difference, presumably because they are only read when the hooks are created. |
Perhaps some of this can be achieved with the |
Okay I understand a bit more now, having backtrace capture enabled has performance implications, thus the decision to make it only enabled at startup via the environment variable makes a lot of sense. I guess the request for somehow being able to choose to format the error without the terminal colours still stands though. Edit: actually I guess color codes could just be stripped from the output instead https://docs.rs/strip-ansi-escapes/0.1.0/strip_ansi_escapes/ much simpler. |
The performance implications are not really the same for |
Okay, thanks! I'll have a bit more of a think about it and submit a formal feature request issue if it makes sense. |
I'm sorry this has taken so long, I've not had enough mental bandwidth to do much open source work recently, but I haven't forgotten about this issue, and I'll get this finalized as soon as I can. |
7d50e89
to
3601d21
Compare
d677655
to
461d069
Compare
alright @kellpossible how does this look? I ended up re-implementing it mostly from scratch because there's another PR that I've been sitting on for a while as well which was going to create the mother of all merge conflicts with this PR thanks to my overzealous code reorganization last time. Dreading dealing with that is a large part of why this has taken so long, once again, sorry about that. Let me know if there are any issues with the current implementation, if not I'll cut a release with this right away. |
No worries, figured something like that might be the case 😅 Sorry for my slow reply! I'll take a look at it today. |
It seems to be working fine with the latest changes |
Draft implementation of #66