p2p: add a per-message type send and receive metric#9622
Merged
williambanfield merged 31 commits intomainfrom Oct 27, 2022
Merged
p2p: add a per-message type send and receive metric#9622williambanfield merged 31 commits intomainfrom
williambanfield merged 31 commits intomainfrom
Conversation
| rs, err := tmmath.SafeConvertUint8(int64(msg.NewRoundStep.Step)) | ||
| switch msg := p.(type) { | ||
| case *tmcons.NewRoundStep: | ||
| rs, err := tmmath.SafeConvertUint8(int64(msg.Step)) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
p2p/switch.go
Outdated
| import ( | ||
| "fmt" | ||
| "math" | ||
| "reflect" |
Check failure
Code scanning / gosec
Blocklisted import runtime
|
|
||
| import ( | ||
| "fmt" | ||
| "reflect" |
Check failure
Code scanning / gosec
Blocklisted import runtime
Contributor
Author
There was a problem hiding this comment.
I don't at all see why we're linting for this. Sometimes reflect is useful to use.
Contributor
There was a problem hiding this comment.
Yeah I couldn't find what the reasoning behind blocking this import is
| func (mp *Peer) FlushStop() { mp.Stop() } //nolint:errcheck //ignore error | ||
| func (mp *Peer) TrySend(chID byte, msgBytes []byte) bool { return true } | ||
| func (mp *Peer) Send(chID byte, msgBytes []byte) bool { return true } | ||
| func (mp *Peer) FlushStop() { mp.Stop() } //nolint:errcheck //ignore error |
Check warning
Code scanning / gosec
Errors unhandled.
| import ( | ||
| "fmt" | ||
| "net" | ||
| "reflect" |
Check failure
Code scanning / gosec
Blocklisted import runtime
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
3606d13 to
0163e86
Compare
mergify bot
pushed a commit
that referenced
this pull request
Oct 27, 2022
* p2p: ressurrect the p2p envelope and use to calculate message metric Co-authored-by: Callum Waters <cmwaters19@gmail.com> (cherry picked from commit 09b8708) # Conflicts: # cmd/tendermint/commands/rollback.go # go.mod # go.sum # p2p/peer.go # p2p/peer_set_test.go
mergify bot
pushed a commit
that referenced
this pull request
Oct 27, 2022
* p2p: ressurrect the p2p envelope and use to calculate message metric Co-authored-by: Callum Waters <cmwaters19@gmail.com> (cherry picked from commit 09b8708) # Conflicts: # .github/workflows/lint.yml # blockchain/v0/reactor.go # cmd/tendermint/commands/rollback.go # consensus/msgs.go # consensus/reactor.go # go.mod # go.sum # libs/rand/random.go # p2p/metrics.gen.go # p2p/metrics.go # p2p/pex/pex_reactor.go # proto/tendermint/blockchain/message.go
williambanfield
added a commit
that referenced
this pull request
Nov 1, 2022
…9641) * p2p: add a per-message type send and receive metric (#9622) * p2p: ressurrect the p2p envelope and use to calculate message metric Add new SendEnvelope, TrySendEnvelope, BroadcastEnvelope, and ReceiveEnvelope methods in the p2p package to work with the new envelope type. Care was taken to ensure this was performed in a non-breaking manner. Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>
3 tasks
adrianbrink
pushed a commit
to heliaxdev/tendermint
that referenced
this pull request
May 23, 2023
…int#9622) (#230) * p2p: add a per-message type send and receive metric (tendermint#9622) * p2p: ressurrect the p2p envelope and use to calculate message metric Co-authored-by: Callum Waters <cmwaters19@gmail.com> (cherry picked from commit 09b8708) # Conflicts: # cmd/tendermint/commands/rollback.go # go.mod # go.sum # p2p/peer.go # p2p/peer_set_test.go * all builds except fuzz * all builds except fuzz * New methods call the old methods * Receive called from OnReceive * tests build after reintroducing send and receive * fix fuzz peer * nolint gosec * mocks use the 'New' calls * move nolint * NewReceive called conditionally from switch * add receives to all with NewReceive * all reactors have receive with empty new receive call * reactors have channel and peer in new receive * add proto message unmarshal to reactors * unwrap messages in receive calls * add channel types where absent * fix mempool reactor to include Receive * unwrap messages in receive calls * remove erroneously added blockchain reactors * remove erroneously added maverick reactor * re-add broadcast method * Rename new methods to *Envelope instead of New* * add deprecation notices * fix typos * document new metrics * format after sed changes * remove erroneous broadcast method name update * remove old receives * comment out old print statement * Resolved conflicts and fixed renaming clashes * Reverted renaming of functions to match original backport PR * reverted accidental renaming due to search/replace * Added changelog entry --------- Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request has one primary goal: to add a new metric for measuring the p2p bytes sent and received by message type.
To do this, parts of the p2p.Envelope from v0.35.x were resurrected. This was done to change the p2p.Send and associated methods along with the reactor.Receive method from taking a []byte as an argument to taking an object with an unserialized proto.Message as an argument. Having the actual proto.Message in hand in the p2p code allows for the type of the message to be determined. Otherwise, the p2p code cannot know at all what the type of the message being sent is and the only alternative is to litter p2p message size calculations all over the code, which seems much more awkward.
The p2p.Envelope resurrection required a few steps:
proto.Message must be added to the ChannelDescriptor so that the p2p code knows what to unmarshal into when receiving a message.
All Send and Receive methods must be changed to use the Envelope type. This means that the reactors pass a proto message that has not yet been marshaled to the p2p layer and the p2p layer similarly calls the
Receivemethod with an unmarshaled proto. All code that marshals and unmarshals the protos in the reactors was removed. Finally, many of the reactors have a wrapper type with aSumfield that can contain one of any of the p2p messages that reactor can receive. To actually be able to determine what kind of underlying message was being sent and received for the purposes of the metric, the v0.35 Wrapper was resurrected, albeit with a minor change: instead ofWrapbeing implemented on the wrapper type, it's implemented on each of theSumtypes so that they each know how to Wrap themselves.With all of these changes in place, the only remaining step was to put the metric into both the send and receive code.
closes: #9599
PR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed