Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly log panics with slog #5075

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Properly log panics with slog #5075

merged 3 commits into from
Jan 31, 2024

Conversation

jxs
Copy link
Member

@jxs jxs commented Jan 16, 2024 •
edited
Loading

Questions

  • Feel free to suggest a better location for the panic::set_hook as I am not 100% sure about it.

example of logs with this PR:

Jan 16 20:58:59.698 INFO Updated local ENR                       udp6: None, tcp6: None, tcp4: Some(9000), udp4: None, ip4: 
None, id: 0x43c7..6f12, seq: 245, enr: enr:-La4QHvi7qEh13FeUFdp-c-4Uokn17MoHAEqFxmLKpKaWjRBFWWNKHDYw2zLMSWZlkz0GGdP0D3tsUIKYX_E0R2C9ziB9YdhdHRuZXRziAAAAAAAYAAAhGV0aDKQYolB7wQAECAAiQMAAAAAAIJpZIJ2NIRxdWljgiMpiXNlY3AyNTZrMaEDShdD_qHrRAQ0Ogm2JuIneubXoaYuvFt4X8d_eFAeTkiIc3luY25ldHMAg3RjcIIjKA, service: libp2p
Jan 16 20:58:59.699 INFO Updated local ENR                       udp6: None, tcp6: None, tcp4: Some(9000), udp4: None, ip4: None, id: 0x43c7..6f12, seq: 246, enr: enr:-La4QJ3XfEKbI7K7FMjynvr0CwNSy6hYxZ1KystbfbKp4fC_bju5Xg-rOVJOJV_viQBaagBzvKII1SlsUfIl4_n8XniB9odhdHRuZXRziAAAAAAAYAAAhGV0aDKQYolB7wQAECAAiQMAAAAAAIJpZIJ2NIRxdWljgiMpiXNlY3AyNTZrMaEDShdD_qHrRAQ0Ogm2JuIneubXoaYuvFt4X8d_eFAeTkiIc3luY25ldHMAg3RjcIIjKA, service: libp2p
Jan 16 20:59:00.028 CRIT Task panic. This is a bug!              message: Panic in the streets of London!, location: /home/jxs/dev/oss/rust-libp2p/protocols/gossipsub/src/behaviour.rs:3344:19
Jan 16 20:59:00.038 INFO Saved DHT state                         service: network
Jan 16 20:59:00.039 INFO Network service shutdown                service: network
Jan 16 20:59:00.039 INFO Internal shutdown received              reason: Panic (fatal error)
Jan 16 20:59:00.040 INFO Shutting down..                         reason: Failure("Panic (fatal error)")
Jan 16 20:59:00.263 INFO Saved beacon chain to disk              service: beacon
Jan 16 20:59:00.733 INFO Execution payloads are pruned           service: freezer_db
Jan 16 20:59:09.819 INFO UPnP not available                      error: IO error: Resource temporarily unavailable (os error 11), service: UPnP
Panic (fatal error)

thanks

dapplion
dapplion reviewed Jan 29, 2024
View reviewed changes
beacon_node/src/lib.rs Outdated
std::panic::set_hook(Box::new(move |info| {
crit!(
log,
"Task panic. This is a bug!";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it suggest to contact developers / open a Github issue if this happens?

At least like in the previous version

                            "advice" => "Please check above for a backtrace and notify \
                                         the developers"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense to keep it, thanks!

@dapplion dapplion added the ready-for-review The code is ready for review label Jan 29, 2024
AgeManning
AgeManning approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AgeManning
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at dada575

mergify bot added a commit that referenced this pull request Jan 31, 2024
@mergify
Merge of #5075
e174eb3
@mergify mergify bot mentioned this pull request Jan 31, 2024
Closed
5 tasks
@mergify mergify bot merged commit dada575 into sigp:unstable Jan 31, 2024
29 checks passed
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
@jxs @danielrachi1
Properly log panics with slog (sigp#5075)
216ff85 
* log panics with slog

* update set_hook location

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into slog-panics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dapplion dapplion dapplion left review comments

@AgeManning AgeManning AgeManning approved these changes

@michaelsproul michaelsproul Awaiting requested review from michaelsproul

Assignees
No one assigned
Labels
ready-for-review The code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jxs @AgeManning @dapplion