-
Notifications
You must be signed in to change notification settings - Fork 3k
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
op-node: Add a histogram to report current peer scores #5870
Conversation
|
✅ Deploy Preview for opstack-docs canceled.
|
Hey @ajsutton! This PR has merge conflicts. Please fix them before continuing review. |
6e91eb8
to
f04807d
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5870 +/- ##
===========================================
- Coverage 43.00% 40.46% -2.54%
===========================================
Files 477 320 -157
Lines 30816 26110 -4706
Branches 877 0 -877
===========================================
- Hits 13252 10566 -2686
+ Misses 16543 14560 -1983
+ Partials 1021 984 -37
Flags with carried forward coverage won't be shown. Click here to find out more.
|
f04807d
to
7de5042
Compare
…t on each update.
I think the two options we should consider are:
I'm leaning towards option 2 just for the simplicity of the code. I've updated this PR to reflect that but can easily revert the last couple of transactions if we want to keep the band based scores or go with the resetting histogram to only show current peer scores. |
2b93470
to
cada12b
Compare
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.
Changes LGTM, nice to have more detailed score metrics without metrics label churn of peerids.
One nit on doc comment, and I think we need to be more careful with the flag removal.
cada12b
to
74ff953
Compare
74ff953
to
f2e442b
Compare
This PR has been added to the merge queue, and will be merged soon. |
This PR is next in line to be merged, and will be merged as soon as checks pass. |
1 similar comment
This PR is next in line to be merged, and will be merged as soon as checks pass. |
Description
Reports the current peer scores in a histogram that is replaced with each update cycle so that it only shows the current score distribution, not observations of each score as it changes. Preserving the history of observations in the histogram would effectively hide banned peer scores because they get disconnected so quickly and thus spend almost no time with such a low score but the rest of the time their connected would contribute to better score observations. Similarly "good" peers have their observations smeared from 0 up to their current score, making it impossible to see the current score.
The component scores that make up the total are reported in separate labels. Getting these values requires returning the updated values from
SetScores
so thatSnapshotHook
has the full set of scores instead of just the gossip scores. The full set of scores is already loaded as part of applying the diff so can efficiently be returned.Additional context
Need to do some more serious testing with how this works with prometheus histogram functions given the resetting nature of the histogram is a bit unusual. If it works well, we can then remove the older score band based metrics since that data can be derived from the histogram.
Metadata
TODOs