feat: add metric to track default validator configuration #7194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7194 +/- ##
============================================
- Coverage 49.21% 49.20% -0.01%
============================================
Files 598 598
Lines 39794 39795 +1
Branches 2091 2097 +6
============================================
Hits 19583 19583
Misses 20171 20171
- Partials 40 41 +1 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
No, 5932 is about having a counter with with a single label that tracks local vs builder block building // produceEngineOrBuilderBlock
const executionPayloadSource: "builder" | "engine" = ...;
metrics?.blockProductionStrategy.inc({source: executionPayloadSource}); |
I don't think we would have caught the issue noted in #5932 even if we had this metric, I think the why is the important part. eg. metrics?.blockProductionSelection.inc({source: "engine", reason: "builder_no_bid"});
metrics?.blockProductionSelection.inc({source: "engine", reason: "builder_disabled"});
metrics?.blockProductionSelection.inc({source: "engine", reason: "builder_error"});
metrics?.blockProductionSelection.inc({source: "builder", reason: "block_value"});That way we could have told that builder had errors and it was not just due to min-bid that we only proposed local blocks. Maybe in addition add a histogram that tracks builder and engine block values to get a better overview on block values. |
|
Curious about the usefulness of the metrics added in this PR, the default configs are also printed out on startup so we might not need it. Maybe something could help infra team @Faithtosin @philknows ? If not we should probably close this and revert the dashboard |
|
Imo its nice to have |
I also think it's nice to have, honestly! |
wemeetagain
approved these changes
Oct 28, 2024
|
🎉 This PR is included in v1.23.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Related to #5932, it seems useful to see in grafana how the validator client is configured and detect misconfigurations.
Description
Add metric to track default validator configuration
We can extend the tracked default configuration values as needed by simply adding a new label to the metric.
@wemeetagain does this resolve #5932?
This will require more work and will have to be tracked on the beacon node side, we have #7190 now which shows engine and builder errors which gives some insights but in case that both blocks succeed, we have to add more metrics to properly track the reason why we selected builder over engine block or vice-versa.