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.

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

chore: Add metrics that shows the current height of approvals #9458

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

kostekIV
Copy link
Contributor

fixes #7846
I have labelled this as chore, but I am not sure if it is an appropriate label.

@nikurt
Copy link
Contributor

nikurt commented Aug 28, 2023

Thank you @kostekIV for picking up that issue!
Do you know why shouldn't we export largest_final_height? Would it be the same as last_final_block_height (in info.rs)?
I'd prefer to also export largest_final_height for completeness.

The code in the PR is fine.
My only suggestion is to replace assignments with setter methods. That way, we'll never forget to update the metrics when the values are changed.

@kostekIV
Copy link
Contributor Author

Thank you @kostekIV for picking up that issue! Do you know why shouldn't we export largest_final_height? Would it be the same as last_final_block_height (in info.rs)? I'd prefer to also export largest_final_height for completeness.

The code in the PR is fine. My only suggestion is to replace assignments with setter methods. That way, we'll never forget to update the metrics when the values are changed.

Setters sounds nice. Im not sure why largest_final_height was not mentioned in the issue, I think I will just add it.

@kostekIV
Copy link
Contributor Author

So I have ended up adding TrackableBlockHeightValue which guards values and sets appropriate gauge on every change to the inner value. It may be overkill tho. Added also largest_final_height. Should I also mention somewhere new metrics?

Copy link
Contributor

@nikurt nikurt left a comment

Choose a reason for hiding this comment

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

Thank you, looks good!

@nikurt
Copy link
Contributor

nikurt commented Aug 29, 2023

The setters interface looks good.

mention new metrics

Please add a line to non-protocol changes in CHANGELOG.md, and then I'll press the "merge" the PR.

@kostekIV
Copy link
Contributor Author

The setters interface looks good.

mention new metrics

Please add a line to non-protocol changes in CHANGELOG.md, and then I'll press the "merge" the PR.

Done

@near-bulldozer near-bulldozer bot merged commit dce4c51 into near:master Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metric that shows the current height of approvals
2 participants