feat: add custom panic hook #168
Conversation
synth/src/cli/telemetry.rs
Outdated
| @@ -40,7 +101,11 @@ where | |||
| Fut: Future<Output = Result<T, E>>, | |||
| E: AsRef<dyn Error + 'static> | |||
| { | |||
| let client = TelemetryClient::new(); | |||
| if is_enabled() { | |||
| let synth_command = serde_json::to_string(&args).unwrap(); | |||
There was a problem hiding this comment.
I couldn't find a way to delay this serialization until inside the panic hook, unfortunately.
There was a problem hiding this comment.
Short of adding #[derive(Clone)] to the Args and moving a clone of args into the closure, I don't see a way to do it either.
juniorbassani
force-pushed
the
custom-panic-handler
branch
from
9cd5122 to
510359b
Compare
juniorbassani
force-pushed
the
custom-panic-handler
branch
from
510359b to
832bb3c
Compare
synth/src/cli/telemetry.rs
Outdated
| let mut username = None; | ||
| let mut email = None; | ||
|
|
||
| println!("{}", "Synth encountered a fatal error. Because telemetry is enabled, an anonymous \ |
There was a problem hiding this comment.
Isn't it better if those are going to stderr instead?
There was a problem hiding this comment.
That makes sense. But only this first message should go to stderr, right? Otherwise, if the user, e.g., redirects stderr, they wouldn't see the other messages that wait for a response.
There was a problem hiding this comment.
That's a good point! Actually we should check synth is being run interactively before this otherwise this will deadlock.
This can be done with something like console::user_attended_stderr for an easy OOB solution.
I would still say it should all go to stderr, as this is, I think, where most people would expect diagnostic output to be sent.
There was a problem hiding this comment.
Right, I haven't thought of that. I will follow your suggestions to fix the issue.
synth/src/cli/telemetry.rs
Outdated
| @@ -40,7 +101,11 @@ where | |||
| Fut: Future<Output = Result<T, E>>, | |||
| E: AsRef<dyn Error + 'static> | |||
| { | |||
| let client = TelemetryClient::new(); | |||
| if is_enabled() { | |||
| let synth_command = serde_json::to_string(&args).unwrap(); | |||
There was a problem hiding this comment.
Short of adding #[derive(Clone)] to the Args and moving a clone of args into the closure, I don't see a way to do it either.
synth/src/cli/telemetry.rs
Outdated
| if is_enabled() { | ||
| let synth_command = serde_json::to_string(&args).unwrap(); | ||
| // Register a custom panic hook that will send a bug report to posthog | ||
| panic::set_hook(Box::new(move |_| { report_panic(&synth_command, &TELEMETRY_CLIENT); })); |
There was a problem hiding this comment.
Can we have the report_panic hook run after the "default" panic hook? That would still print the standard panic message which could be useful.
We could use panic::take_hook and move the old hook into the new one for that.
This replaces the default panic hook by one that will run the default hook and send the panic occurrence to PostHog, with privacy sensitive information erased (e.g., database names and user directories). The hook also asks whether the user would like to send their name and e-mail in case it is deemed necessary to enter in contact to obtain further information. The hook is only enabled when telemetry is enabled.
juniorbassani
force-pushed
the
custom-panic-handler
branch
from
832bb3c to
537d1e4
Compare
|
I believe the issues pointed previously have now been fixed. I also refactored the code a little bit, because my previous changes left it messy. 😅 |
There was a problem hiding this comment.
LGTM.
Thanks @juniorbassani for addressing everything so well and so quickly 😄
Let me know what you think.
Closes #114