-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
More events for observability #732
Conversation
fendermint/app/src/app.rs
Outdated
hash = request.hash.to_string(), | ||
proposer = request.proposer_address.to_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we don't add these fields to the accepted event as well? We could refactor this into a single emit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we dont really care who proposed it if accepted, we can get that from cometbft. But reject, we might want to know who. As for two events, I think it was because it's tracked as two events so no extra filtering needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For diagnostics, it's useful to know who you accepted a proposal from explicitly. That allows you to identify patterns when performing diagnostics/forensics. Also: let's err on the side of spitting out more information, we can always reduce the verbosity later. It also simplifies the code and involves less entropy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding more events for: