-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Remote Signer: Electra #14477
base: develop
Are you sure you want to change the base?
Remote Signer: Electra #14477
Conversation
blockElectraSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{ | ||
Name: "remote_web3signer_block_electra_sign_requests_total", | ||
Help: "Total number of block electra sign requests", | ||
}) | ||
blindedBlockElectraSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{ | ||
Name: "remote_web3signer_blinded_block_electra_sign_requests_total", | ||
Help: "Total number of blinded block electra sign requests", | ||
}) |
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.
This pattern seems bad. Why not just add a label to mention the fork?
blockSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{
Name: "remote_web3signer_block_sign_requests_total",
Help: "Total number of block sign requests",
})
blindedBlockSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{
Name: "remote_web3signer_blinded_block_sign_requests_total",
Help: "Total number of blinded block sign requests",
})
Then use it with appropriate value
blockSignRequestsTotal.WithLabelValues("fork", "electra").Inc()
I think we should do this for the v6 release. There is no reason to report old fork metrics by registering a counter for every fork when we could use labels. Can you add this for Electra, then we can make the breaking change for old metrics in v6?
What type of PR is this?
Feature
What does this PR do? Why is it needed?
updates the remote signing to handle changes from PRs ethereum/remote-signing-api#15 and ethereum/remote-signing-api#17
Kurtosis testing with the following settings
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements