p2p: add message type into the send/recv bytes metrics#7155
p2p: add message type into the send/recv bytes metrics#7155mergify[bot] merged 4 commits intomasterfrom
Conversation
| // This method uses a map on the Metrics struct so that each label name only needs | ||
| // to be produced once to prevent expensive string operations. | ||
| func (m *Metrics) ValueToMetricLabel(i interface{}) string { | ||
| t := reflect.TypeOf(i) |
There was a problem hiding this comment.
I do not think it's worthwhile to do the read-write promotion here. Nobody will spend long enough in the critical section to make the swapping worthwhile.
Rationale: Having a reader section makes sense when there are lots of readers and very few writers—but here there are probably not more than a handful of either. I wouldn't even bother benchmarking it—but if you did I bet it's actually faster with a plain mutex. Reader queues are relatively expensive.
There was a problem hiding this comment.
For my own understanding, I created some benchmarks in a branch here. It does look like the RW mutex version runs faster when the map becomes full, but perhaps I've constructed the test incorrectly.
go test -bench=.. -test.v -run Bench -test.count 2
goos: darwin
goarch: amd64
pkg: github.com/tendermint/tendermint/internal/p2p
cpu: Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
BenchmarkValueToMetricsLabel
BenchmarkValueToMetricsLabel/RW_Mutex_Version
BenchmarkValueToMetricsLabel/RW_Mutex_Version-8 9853 117218 ns/op
BenchmarkValueToMetricsLabel/RW_Mutex_Version-8 9170 121957 ns/op
BenchmarkValueToMetricsLabel/Mutex_Version
BenchmarkValueToMetricsLabel/Mutex_Version-8 7255 141405 ns/op
BenchmarkValueToMetricsLabel/Mutex_Version-8 8491 141001 ns/op
BenchmarkValueToMetricsLabelPrefilled
BenchmarkValueToMetricsLabelPrefilled/RW_Mutex_Version
BenchmarkValueToMetricsLabelPrefilled/RW_Mutex_Version-8 17010 70549 ns/op
BenchmarkValueToMetricsLabelPrefilled/RW_Mutex_Version-8 16965 69600 ns/op
BenchmarkValueToMetricsLabelPrefilled/Mutex_Version
BenchmarkValueToMetricsLabelPrefilled/Mutex_Version-8 7302 137573 ns/op
BenchmarkValueToMetricsLabelPrefilled/Mutex_Version-8 7262 144271 ns/op
PASS
ok github.com/tendermint/tendermint/internal/p2p 17.722s
My main consideration here was that eventually all of the labels would be created so this RW lock would really only be accessed for read operations. Mind taking a look to see if I've done anything completely silly?
There was a problem hiding this comment.
This is probably a six-of-one kind of a problem so I'm really fine with either solution.
tychoish
left a comment
There was a problem hiding this comment.
the fact that the chIDstr is just a string of a number is a problem, but probably a problem somewhere else in the code? I think it'd be cool if we could convey what channel each ID/number correlated with would be nice? otherwise we wouldn't quite know.
It's also the case that this is probably going to go away or break once we switch to libp2p (as this is right beneath the line.) I think it's good todo this (and maybe we should back port it to 0.35 so it'll be useful for longer...)
Yeah, the chID is likely meaningless to any user and should be changed, but want to tackle one thing at a time. Do you think that the ability to tell what message types we are passing/how many bytes we pass will also go away? |
I think it'll remain, but will be reported by libp2p rather than us. |
This pull request adds a new "mesage_type" label to the send/recv bytes metrics calculated in the p2p code.
Below is a snippet of the updated metrics that includes the updated label:
```
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="2551a13ed720101b271a5df4816d1e4b3d3bd133"} 652
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="4b1068420ef739db63377250553562b9a978708a"} 631
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="927c50a5e508c747830ce3ba64a3f70fdda58ef2"} 631
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="2551a13ed720101b271a5df4816d1e4b3d3bd133"} 393
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="4b1068420ef739db63377250553562b9a978708a"} 357
tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="927c50a5e508c747830ce3ba64a3f70fdda58ef2"} 386
```
(cherry picked from commit b4bc6bb)
# Conflicts:
# internal/p2p/pqueue.go
… (#7161) * p2p: add message type into the send/recv bytes metrics (#7155) This pull request adds a new "mesage_type" label to the send/recv bytes metrics calculated in the p2p code. Below is a snippet of the updated metrics that includes the updated label: ``` tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="2551a13ed720101b271a5df4816d1e4b3d3bd133"} 652 tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="4b1068420ef739db63377250553562b9a978708a"} 631 tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_HasVote",peer_id="927c50a5e508c747830ce3ba64a3f70fdda58ef2"} 631 tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="2551a13ed720101b271a5df4816d1e4b3d3bd133"} 393 tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="4b1068420ef739db63377250553562b9a978708a"} 357 tendermint_p2p_peer_receive_bytes_total{chID="32",chain_id="ci",message_type="consensus_NewRoundStep",peer_id="927c50a5e508c747830ce3ba64a3f70fdda58ef2"} 386 ``` (cherry picked from commit b4bc6bb)
This pull request adds a new "mesage_type" label to the send/recv bytes metrics calculated in the p2p code.
Below is a snippet of the updated metrics that includes the updated label: