Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Add from field for message tracking#32725

Merged
gregcusack merged 9 commits intosolana-labs:masterfrom
gregcusack:add-from-field-for-message-tracking
Aug 7, 2023
Merged

Add from field for message tracking#32725
gregcusack merged 9 commits intosolana-labs:masterfrom
gregcusack:add-from-field-for-message-tracking

Conversation

@gregcusack
Copy link
Copy Markdown
Contributor

Problem

We want to track how push messages propagate through the network from node to node. There is currently no way to track this. This builds off of PR: #32708.

Summary of Changes

Add a from field to GossipRoute::PushMessage --> GossipRoute::PushMessage(/*from:*/ Pubkey)
For extracting that pubkey for reporting to metrics, an std:: fmt::Display was added for GossipRoute::PushMessage(Pubkey).

Fixes #

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2023

Codecov Report

Merging #32725 (9813294) into master (8495257) will decrease coverage by 0.1%.
Report is 10 commits behind head on master.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #32725     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         785      785             
  Lines      211084   211085      +1     
=========================================
- Hits       173107   173098      -9     
- Misses      37977    37987     +10     

@gregcusack gregcusack requested a review from behzadnouri August 7, 2023 15:38
Comment thread gossip/src/crds.rs Outdated
PushMessage(/*from:*/ Pubkey),
}

impl std::fmt::Display for GossipRoute {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need this; instead record_insert can do something like:

let GossipRoute::PushMessage(from) = route else {
  return;
}

Comment thread gossip/src/crds.rs Outdated
PullRequest,
PullResponse,
PushMessage,
PushMessage(/*from:*/ Pubkey),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be reference so we don't copy the pubkey for each value?
something like

pub enum GossipRout<'a> {
   // ...
   PushMessage(&'a Pubkey),
}

Comment thread gossip/src/crds.rs Outdated
}

if matches!(route, GossipRoute::PushMessage)
if matches!(route, GossipRoute::PushMessage(_))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think instead of this we can do

let GossipRoute::PushMessage(from) = route else {
  return;
}

before the if branch.

@gregcusack gregcusack requested a review from behzadnouri August 7, 2023 17:53
Copy link
Copy Markdown
Contributor

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

lgtm,
please wait for the CI to come green before merging.

@gregcusack gregcusack merged commit 5c86f89 into solana-labs:master Aug 7, 2023
@gregcusack gregcusack added the v1.16 PRs that should be backported to v1.16 label Aug 7, 2023
mergify Bot pushed a commit that referenced this pull request Aug 7, 2023
* we only want to report received message signatures on PUSH requests, not PULL requests

* woops accidently had it has LocalMessage not PushMessage

* switch from match to if let statement

* convert if let to matches macro

* add in from field in PushMessage for message tracking

* update with cargo fmt

* remove display for gossip route and add lifetime param to pubkey reference in gossiproute enum

* forgot to run fmt

(cherry picked from commit 5c86f89)

# Conflicts:
#	gossip/src/crds.rs
@gregcusack gregcusack added v1.16 PRs that should be backported to v1.16 and removed v1.16 PRs that should be backported to v1.16 labels Aug 9, 2023
gregcusack pushed a commit that referenced this pull request Aug 9, 2023
* we only want to report received message signatures on PUSH requests, not PULL requests

* woops accidently had it has LocalMessage not PushMessage

* switch from match to if let statement

* convert if let to matches macro

* add in from field in PushMessage for message tracking

* update with cargo fmt

* remove display for gossip route and add lifetime param to pubkey reference in gossiproute enum

* forgot to run fmt
gregcusack pushed a commit that referenced this pull request Aug 9, 2023
Add from field for message tracking (#32725)

* we only want to report received message signatures on PUSH requests, not PULL requests

* woops accidently had it has LocalMessage not PushMessage

* switch from match to if let statement

* convert if let to matches macro

* add in from field in PushMessage for message tracking

* update with cargo fmt

* remove display for gossip route and add lifetime param to pubkey reference in gossiproute enum

* forgot to run fmt

Co-authored-by: Greg Cusack <greg.cusack@solana.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

v1.16 PRs that should be backported to v1.16

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants