p2p: add a per-message type send and receive metric #9622

williambanfield · 2022-10-24T20:01:41Z

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 Receive method 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 a Sum field 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 of Wrap being implemented on the wrapper type, it's implemented on each of the Sum types 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

Tests written/updated, or no tests needed
CHANGELOG_PENDING.md updated, or no changelog entry needed
Updated relevant documentation (docs/) and code comments, or no
documentation updates needed

consensus/msgs.go

-		rs, err := tmmath.SafeConvertUint8(int64(msg.NewRoundStep.Step))
+	switch msg := p.(type) {
+	case *tmcons.NewRoundStep:
+		rs, err := tmmath.SafeConvertUint8(int64(msg.Step))


p2p/switch.go

@@ -3,12 +3,13 @@
 import (
 	"fmt"
 	"math"
+	"reflect"


williambanfield · 2022-10-25T18:09:04Z

p2p/metrics.go

@@ -1,6 +1,11 @@
 package p2p

 import (
+	"fmt"
+	"reflect"


I don't at all see why we're linting for this. Sometimes reflect is useful to use.

Yeah I couldn't find what the reasoning behind blocking this import is

p2p/mock/peer.go

-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


cmwaters

Seems all in order

p2p/base_reactor.go

p2p/peer.go

p2p/metrics.go

p2p/peer.go

@@ -3,8 +3,11 @@
 import (
 	"fmt"
 	"net"
+	"reflect"


Co-authored-by: Callum Waters <cmwaters19@gmail.com>

* 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

* 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

…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>

…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>

williambanfield added 8 commits October 24, 2022 15:18

p2p: ressurrect the p2p envelope and use to calculate message metric

cbe179c

p2p: re-implement the the wrapper abstraction to de-duplicate logic

0ea99d0

p2p/metrics: add the metricsLabelCache to prevent name recalculation

63a6058

cleanup: remove error from function that doesn't error

c5abbe6

cleanup: remove superflous re-wrap in consensus

624505a

cleanup: remove superflous error in wrap

94f7df7

cleanup: panic in unmarshal error on receive

0b0d921

cleanup: use logger in TrySend and Send

d4fcfa1

williambanfield changed the title ~~Wb/final metric impl~~ p2p: add a per-message type metric Oct 24, 2022

williambanfield changed the title ~~p2p: add a per-message type metric~~ p2p: add a per-message type send and receive metric Oct 24, 2022

github-advanced-security bot found potential problems Oct 24, 2022

View reviewed changes

williambanfield added 3 commits October 24, 2022 16:16

cleanup: test fixup after refactor

60373be

cleanup: fix TestMsgToProto

d3920fe

fix comment on wrap/unwrap

72c0e66

williambanfield marked this pull request as ready for review October 24, 2022 20:43

williambanfield requested a review from a team October 24, 2022 20:43

cmwaters approved these changes Oct 25, 2022

View reviewed changes

p2p/base_reactor.go Outdated Show resolved Hide resolved

p2p/peer.go Outdated Show resolved Hide resolved

p2p/metrics.go Show resolved Hide resolved

williambanfield and others added 2 commits October 25, 2022 14:09

add type name to panic

7db1901

Merge branch 'main' into wb/final-metric-impl

53502b2

github-advanced-security bot found potential problems Oct 25, 2022

View reviewed changes

p2p/peer.go

@@ -3,8 +3,11 @@

import (

"fmt"

"net"

"reflect"

Check failure

Code scanning / gosec

Blocklisted import runtime

Blocklisted import reflect

williambanfield and others added 7 commits October 25, 2022 14:15

Update p2p/base_reactor.go

8e24fab

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

Merge branch 'main' into wb/final-metric-impl

40992b6

use metricsLabelCache constructor

1db0ee8

gofmt -s

074f5f1

lint fixes

e561129

make the linter happier

059e637

Merge branch 'main' into wb/final-metric-impl

10d3eb1

williambanfield added S:automerge Automatically merge PR when requirements pass S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x labels Oct 26, 2022

williambanfield and others added 7 commits October 26, 2022 19:37

update golangci-lint version

36d094e

Merge branch 'main' into wb/final-metric-impl

c330e49

wrap mempool message correctly

d35f1b4

defer stop peers in mempool test

80f3aa8

mempool message conforms to other types

14cd282

Merge remote-tracking branch 'origin/main' into wb/final-metric-impl

fe429ce

no lint errcheck

95d0c31

williambanfield removed the S:automerge Automatically merge PR when requirements pass label Oct 27, 2022

williambanfield added 2 commits October 27, 2022 15:26

stop peer to prevent loop hanging forever

4818ae8

add github issue for bad test

0163e86

williambanfield force-pushed the wb/final-metric-impl branch from 3606d13 to 0163e86 Compare October 27, 2022 19:30

williambanfield added 2 commits October 27, 2022 15:32

send bad message instead

2477e3d

add stop peer for error

d33942b

williambanfield merged commit 09b8708 into main Oct 27, 2022

williambanfield deleted the wb/final-metric-impl branch October 27, 2022 19:46

mergify bot mentioned this pull request Oct 27, 2022

p2p: add a per-message type send and receive metric (backport #9622) #9640

Open

mergify bot mentioned this pull request Oct 27, 2022

p2p: add a per-message type send and receive metric (backport #9622) #9641

Merged

thanethomson mentioned this pull request Nov 8, 2022

docs: Add new per-message type P2P metrics #9676

Merged

3 tasks

jmalicevic mentioned this pull request Jan 29, 2023

p2p: add a per-message type send and receive metric (backport #9622) cometbft/cometbft#230

Merged

This was referenced Jun 11, 2024

Make reactor.onReceive not block unmarshalling more packets from peer cometbft/cometbft#3199

Open

p2p: connection multiplexing does not work as expected on the receiving side cometbft/cometbft#3250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: add a per-message type send and receive metric #9622

p2p: add a per-message type send and receive metric #9622

williambanfield commented Oct 24, 2022 •

edited

williambanfield Oct 25, 2022

cmwaters Oct 26, 2022

cmwaters left a comment

p2p: add a per-message type send and receive metric #9622

p2p: add a per-message type send and receive metric #9622

Conversation

williambanfield commented Oct 24, 2022 • edited

PR checklist

williambanfield Oct 25, 2022

Choose a reason for hiding this comment

cmwaters Oct 26, 2022

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

williambanfield commented Oct 24, 2022 •

edited