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

PeerMetrics: interpolate results when inserting a new peer #3915

Merged
merged 15 commits into from
Aug 17, 2022

Conversation

coot
Copy link
Contributor

@coot coot commented Jul 21, 2022

Description

This PR makes sure that newly added peers to PeerMetrics are never in 20% of worst performing peers. Currently the churn interval and number of records kept by PeerMetrics are in sync, peers added just before the churn event can be removed as they might have too few results. In the future the churn interval and the number of records kept by PeerMetrics might diverge, and thus it's more important to make sure that newly added peers are not immediately removed.

This PR solves this problem by keeping track of average results when a peer joined the leader board and is using linear regression to adjust peer results for the leader board period where the peer was absent from the leader board.

Fixes #3861

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@coot coot requested a review from bolt12 July 21, 2022 16:12
@coot coot marked this pull request as ready for review July 27, 2022 08:27
@coot coot requested a review from dcoutts July 27, 2022 08:38
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

Very nice!

@coot coot force-pushed the coot/peer-metrics branch 2 times, most recently from 819c681 to 9ee1fcf Compare August 9, 2022 14:18
@coot coot force-pushed the coot/peer-metrics branch 4 times, most recently from e47cb1e to ed56f39 Compare August 16, 2022 08:39
@coot coot requested a review from bolt12 August 16, 2022 08:43
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

Looks good! Just minor typo

This patch allows to configure the number of events tracked by
PeerMetrics.
When inserting new peers into `PeerMetricsState`, record the current
average result.  This average will be used to linearly interpolate for
the missing history.  Once a peer is longer than `maxEntriesToTrack`
slots then the average is removed and no longer impacts the results (as
there are enough results in the `PeerMetricsState`).
`Delay` constructor is useful when one needs a finer control over
delays.

`maybeStepScript` and `maybeStepScriptSTM` allow to step through the
script and terminate once the script is over.
Resolve ties of hot demotion with slot number which indicates when
a peer joined the leader board.  Oldest entries are sorted first.  This
means that the peers who have the same low score but are longer are
preferred for demotion (their result is more certain).  This also allows
to keep the newly added peers as hot if the leader board statistic is
skewed (e.g. a log of peers have the same low score).
Two properties are included:

* check that newly added peers are never in 20% of worst performing
  peers
* check that the peer metric map never exceeds its configured sized.
@coot
Copy link
Contributor Author

coot commented Aug 17, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 17, 2022

@iohk-bors iohk-bors bot merged commit 79998cf into master Aug 17, 2022
@iohk-bors iohk-bors bot deleted the coot/peer-metrics branch August 17, 2022 15:06
iohk-bors bot added a commit that referenced this pull request Nov 9, 2022
4120: Cherry picked network changes for cardano-node-1.35.5 release r=coot a=coot

This cherry-picked patches from the following PRs:

* #3794
* #3844
* #3785
* #3904
* #3915
* #3852
* #3970
* #3979
* #4015
* #4067
* #4004
* #4086
* #4113
* #4106
* #4127
* #4103

Also cherry-picked almost all the commits which modify GitHub actions:
* 18c5244 Run GitHub Actions on pull requests   
* 3adf5a9 Use newer version of io-sim           
* ee9b7a6 Fix GH Actions Windows CI: switch from pkgconf to pkg-config 
* e6cf074 github-actions: use `ubuntu-latest`   
* 9a8b959 Updated versions of github actions    
* fc8f8f0 Fix GH Actions Windows CI caching     
* 7f07c40 Windows Github Actions now use MSYS2  
* b21a7ce Fix chocolatey CI error
* #4134               

TODO:

* [x] bump versions of packages
* [x] input-output-hk/cardano-haskell-packages#84

Co-authored-by: Mark Tullsen <[email protected]>
Co-authored-by: Marcin Szamotulski <[email protected]>
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.

Initial score for new peers entering the churn leader board
2 participants