feat: Catch panic to generate report and reraise #7341
Conversation
cli/helper.go
Outdated
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Generate report in LOTUS_PATH and re-raise panic | ||
| build.GeneratePanicReport(os.Getenv("LOTUS_PATH"), "app_panic") |
There was a problem hiding this comment.
This isn't guaranteed to be set, and each binary may have it's own location eg $LOTUS_MINER_PATH, $LOTUS_WORKER_PATH, etc.
Have you tried to see if you can run this recovery in the After method of urfave cli? This will give you access to the urfave cli context which will allow you to use the cli flags and avoid using os.Getenv.
Being able to customize the location where the reports are generated would also be amazing. Something like LOTUS_PANIC_REPORTS or similar that is accepted on every binary would be great. It could default into the LOTUS_*_PATH. You can use app.Name to do a lookup for the correct cli flag to read from to get the correct repo path for defaults + overrides using env vars.
The second argument could then also be the application name as well. This in conjunction with a LOTUS_PANIC_REPORTS would mean someone could have all their panics sent to a single location and then identified by the label.
There was a problem hiding this comment.
I used the local context instead of a lookup based on app.Name. LMK if you think this is sufficient. (Was trying to avoid brittle lookup regressions if names change.) I think I got all of your suggestions in here now.
placer14
force-pushed
the
mg/feat/panic-reporter
branch
from
9c83b40 to
78cd690
Compare
placer14
force-pushed
the
mg/feat/panic-reporter
branch
from
78cd690 to
926858a
Compare
Codecov Report
@@ Coverage Diff @@
## master #7341 +/- ##
==========================================
- Coverage 39.09% 39.08% -0.01%
==========================================
Files 614 615 +1
Lines 64997 65125 +128
==========================================
+ Hits 25408 25452 +44
- Misses 35172 35263 +91
+ Partials 4417 4410 -7
Continue to review full report at Codecov.
|
build/panic_reporter.go
Outdated
| ignoreCommitBefore := os.Getenv("LOTUS_VERSION_IGNORE_COMMIT") | ||
| os.Setenv("LOTUS_VERSION_IGNORE_COMMIT", "") //nolint:errcheck | ||
| defer os.Setenv("LOTUS_VERSION_IGNORE_COMMIT", ignoreCommitBefore) //nolint:errcheck |
There was a problem hiding this comment.
It would be much cleaner to just do build.BuildVersion + build.BuildType() + build.CurrentCommit (Just make buildType public)
There was a problem hiding this comment.
I opted to keep version func calls private until we know we want it to live external to build package. Didn't want to expose more if we don't really need to.
There was a problem hiding this comment.
There was no good reason for it to be private other than nothing needed it to be public before
There was a problem hiding this comment.
Okay, sounds like you really want it. 574b5c03d
placer14
force-pushed
the
mg/feat/panic-reporter
branch
from
7525e74 to
574b5c0
Compare
build/panic_reporter.go
Outdated
| } | ||
| } | ||
|
|
||
| syscall.Umask(0) |
There was a problem hiding this comment.
Any specific reason to do this?
There was a problem hiding this comment.
(If yes, would add a comment explaining why)
There was a problem hiding this comment.
I was debugging something and accidentally committed it. Removed.
Co-authored-by: Łukasz Magiera <[email protected]>
an attempt at #7315
Room for improvement?
expose journal lookback param as envvardone