-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Earliest Available Slot metrics #16069
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
base: develop
Are you sure you want to change the base?
Conversation
beacon-chain/custody/metrics.go
Outdated
| var ( | ||
| // EarliestAvailableSlotP2P tracks the earliest available slot in the p2p service | ||
| EarliestAvailableSlotP2P = promauto.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "custody_earliest_available_slot_p2p", |
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.
Why not just earlied_available_slot_p2p?
beacon-chain/custody/metrics.go
Outdated
| // EarliestAvailableSlotP2P tracks the earliest available slot in the p2p service | ||
| EarliestAvailableSlotP2P = promauto.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "custody_earliest_available_slot_p2p", | ||
| Help: "The earliest available slot tracked by the p2p service for custody purposes", |
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.
Why for custody purposes?
beacon-chain/custody/metrics.go
Outdated
|
|
||
| // EarliestAvailableSlotDB tracks the earliest available slot in the database | ||
| EarliestAvailableSlotDB = promauto.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "custody_earliest_available_slot_db", |
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.
Why not just earliest_available_slot_db?
beacon-chain/custody/metrics.go
Outdated
| // EarliestAvailableSlotDB tracks the earliest available slot in the database | ||
| EarliestAvailableSlotDB = promauto.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "custody_earliest_available_slot_db", | ||
| Help: "The earliest available slot tracked by the database for custody purposes", |
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.
Why for custody purposes?
beacon-chain/custody/metrics.go
Outdated
| @@ -0,0 +1,32 @@ | |||
| // Package custody provides common custody-related metrics | |||
| package custody | |||
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.
Please do not create a whole package only to add 2 metrics.
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 did not read all the PR.
My main question is:
Why not simply update the metrics in the function actually changing the corresponding values, or when the correspond values are first pulled at the start of the node?
You can start by identifying all places where the eas is updated, then the place where eas if first pulled from the DB.
Then, just add metrics update in those places.
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR adds metrics for earliest available slot. Two metrics are added corresponding to the DB update and p2p update. They have been tested on kurtosis to exactly match.
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements