Skip to content

fix(op-node): Peer Scoring Metrics#5204

Merged
mergify[bot] merged 11 commits intodevelopfrom
refcell/fix/pscores
Mar 22, 2023
Merged

fix(op-node): Peer Scoring Metrics#5204
mergify[bot] merged 11 commits intodevelopfrom
refcell/fix/pscores

Conversation

@refcell
Copy link
Contributor

@refcell refcell commented Mar 20, 2023

Description

Fixes CLI-3646

Adds a quick-fix to the peer scoring p2p flags in op-node and adds back the peer_id so we can track peer scores in testing environments.

@refcell refcell requested review from a team as code owners March 20, 2023 16:37
@refcell refcell requested review from trianglesphere and zhwrd March 20, 2023 16:37
@changeset-bot
Copy link

changeset-bot bot commented Mar 20, 2023

⚠️ No Changeset found

Latest commit: 787db60

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@refcell refcell self-assigned this Mar 20, 2023
@netlify
Copy link

netlify bot commented Mar 20, 2023

Deploy Preview for opstack-docs ready!

Name Link
🔨 Latest commit 787db60
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/641b6ebd21b8c20008664eaa
😎 Deploy Preview https://deploy-preview-5204--opstack-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@refcell refcell requested a review from protolambda March 20, 2023 16:46
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #5204 (787db60) into develop (8dd0248) will decrease coverage by 3.81%.
The diff coverage is 50.58%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5204      +/-   ##
===========================================
- Coverage    40.00%   36.19%   -3.81%     
===========================================
  Files          371      217     -154     
  Lines        23512    19082    -4430     
  Branches       832        0     -832     
===========================================
- Hits          9405     6907    -2498     
+ Misses       13402    11503    -1899     
+ Partials       705      672      -33     
Flag Coverage Δ
bedrock-go-tests 36.19% <50.58%> (+0.06%) ⬆️
common-ts-tests ?
contracts-bedrock-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/flags/p2p_flags.go 100.00% <ø> (ø)
op-node/metrics/metrics.go 1.31% <0.00%> (-0.02%) ⬇️
op-node/p2p/cli/load_config.go 0.00% <0.00%> (ø)
op-node/p2p/gossip.go 38.66% <ø> (ø)
op-node/p2p/mocks/ConnectionGater.go 5.06% <0.00%> (-0.10%) ⬇️
op-node/p2p/mocks/PeerGater.go 25.00% <ø> (ø)
op-node/p2p/mocks/Peerstore.go 0.00% <ø> (ø)
op-node/p2p/prepared.go 0.00% <0.00%> (ø)
op-node/p2p/peer_scorer.go 87.50% <84.44%> (-12.50%) ⬇️
op-node/p2p/config.go 31.48% <100.00%> (+2.63%) ⬆️
... and 2 more

... and 155 files with indirect coverage changes

@refcell refcell requested a review from trianglesphere March 20, 2023 21:17
@sebastianst sebastianst self-requested a review March 20, 2023 22:23
@refcell refcell requested a review from sebastianst March 20, 2023 22:56
@ajsutton
Copy link
Contributor

The Bucketable interface is a really nice little abstraction. Good thinking.

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

please make sure to clean up old implementation artifacts

@sebastianst sebastianst requested a review from ajsutton March 21, 2023 12:12
@refcell refcell requested a review from sebastianst March 21, 2023 16:40
@refcell refcell requested a review from protolambda March 21, 2023 19:33
@refcell refcell force-pushed the refcell/fix/pscores branch from 699ee3b to ddc14c4 Compare March 21, 2023 19:39
@refcell refcell requested a review from tynes March 21, 2023 20:14
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

I really like the score bands & stream_count metrics, but I'm a little confused on how the BandScorer is being used.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Looks ok to me. Left a couple of suggestions but will defer to Seb and Josh for the bigger picture stuff.

@ajsutton
Copy link
Contributor

hmm, running this locally with --p2p.scoring.peers=light --p2p.ban.peers I always get:

op_node_default_p2p_peer_scores{band=""} 1

and my very malicious peer that's spamming invalid gossip isn't being disconnected.
Even when I specify --p2p.score.bands="-40:graylist;-20:restricted;0:nopx;20:friend;" as well I still don't get any named bands in metrics.

Copy link
Member

@sebastianst sebastianst 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 to me now, excited to see this in action soon 🤩 Just some minor suggestions for further simplification.

@refcell refcell requested a review from sebastianst March 22, 2023 15:58
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

ops changes lgtm. Only one q about the prepared p2p stuff

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot merged commit 6450bcc into develop Mar 22, 2023
@mergify mergify bot deleted the refcell/fix/pscores branch March 22, 2023 21:28
@mergify mergify bot removed the on-merge-train label Mar 22, 2023
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.

6 participants