fix(cli): install panic hook so panics flow through OpenTelemetry (OTel) - #187
fix(cli): install panic hook so panics flow through OpenTelemetry (OTel)#187aatran14 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8233fa12d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| puffgres_cli::observability::install_panic_hook(); |
There was a problem hiding this comment.
Flush telemetry before exiting on panic
In an OTEL-enabled command such as puffgres run, apply, or remove that panics after observability::init, this hook only enqueues a tracing::error! and then invokes the default panic hook; because the unwind never returns from run().await, the Telemetry::shutdown() call in main is skipped. The log this change is intended to export can therefore remain buffered and be lost at process exit unless the panic path catches the unwind and flushes/shuts down telemetry synchronously.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think agree with this
To my understanding:
- This PR makes panic visible to the Otel pipeline; for example, structured payload & location
- The "flush-on-panic" gap is existing behavior. Any batched telementry not yet exported is lost when a panic unwinds past main's shutdown(), regardless of this change.
- This PR happens to be the first log emitted at panic time. I think that flushing on the panic path whether that be a sync force_flush in run() or the hook ultimately entails a lot more work which justifies its own PR.
To clarify:
In the interest of simplicity, scope this PR to
puffgres_cli::observability::install_panic_hook();incli/src/main.rs- the testing for it
5634835 to
8233fa1
Compare
useful for folks who will end up using the open telemetry
Why
install_panic_hookwas defined ,but never called. As a result of this behavior, the panics never reach OTel.Add
Calls
install_panic_hook()at startup.Testing
Lacks unit test, which I suspect belongs in
cli/src/observability.rs. Worth making sure those test(s) age well.I was able to get a clean build
cargo build -p puffgres-cli