feat(p2p): pre-allocate static labels used to produce metrics by cason · Pull Request #2851 · cometbft/cometbft

cason · 2024-04-18T15:52:29Z

Addresses #2840.

The same approach taken for the send() method is also adopted for the onReceive() method.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

cason · 2024-04-18T16:41:10Z

p2p/peer.go

 	} else if !p.hasChannel(chID) {
 		return false
 	}
-	metricLabelValue := p.mlc.ValueToMetricLabel(msg)


I wonder whether there was a reason for calling this here, as msg can be changed in line 276...

Indeed, it breaks everything. Restored.

cason · 2024-04-18T16:42:40Z

p2p/metrics.go

@@ -43,6 +43,19 @@ type Metrics struct {
 type metricsLabelCache struct {
 	mtx               *sync.RWMutex
 	messageLabelNames map[reflect.Type]string


We could probably also pre-allocate this guy, which would allow us for getting rid of the lock.

andynog

added a comment around metric allocation, I don't understand the metric package well but just looking at the dependency I think there might still some problems with allocations.

andynog · 2024-04-18T18:36:10Z

p2p/peer.go

 		}
-		p.metrics.PeerReceiveBytesTotal.With(labels...).Add(float64(len(msgBytes)))
-		p.metrics.MessageReceiveBytesTotal.With("message_type", p.mlc.ValueToMetricLabel(msg)).Add(float64(len(msgBytes)))
+		p.metrics.PeerReceiveBytesTotal.


in the go-kit/metrics dependency, using the With factory I believe might allocate a Counter (at least a pointer) for every combination of peer ID and channel ID on a received message ? If so maybe the logic to cache should be not on labels but on the Counter (pair or peer ID and chID) and then you just call the Add in the receive msg routine ?
https://github.com/go-kit/kit/blob/78fbbceece7bbcf073bee814a7772f4397ea756c/metrics/prometheus/prometheus.go#L36

This is my understanding of the bottleneck here as well. (That Allocations come from the With call, and thats what we want to cache)

Just got a recent CPU profile for a synced full node for 1 hour, the With call is definitely what needs to be cached:

(Ideally we can remove the Add call's makeLabel work subsequently as well, but tbh I don't understand that)

Thanks for the insights @ValarDragon

We would need to cache the metrics.Counter instances. The problem is the cardinality.

We have 22 message types, if I am not wrong, and 10 channels. With standard configuration, we can have from 10 to 50 peers. Ok, at the end is not that impossible.

cason · 2024-04-19T07:16:39Z

I don't understand the metric package well but just looking at the dependency I think there might still some problems with allocations.

The problem is that this is very hard to evaluate without running a bunch of nodes...

ValarDragon · 2024-04-19T08:37:49Z

Can help with getting more mainnet profiles!

melekes

👍

cason · 2024-04-19T16:53:59Z

Maybe we can merge this, while it is pretty clear that it does not solve the problem...

ValarDragon · 2024-04-20T05:44:46Z

That sgtm! These sprintf calls were avoiding do show up in cpu profiles so absolutely helpful to get this eliminated!

ValarDragon · 2024-04-20T06:10:41Z

CPU benchmark for the curious:

This is on a caught up Osmosis full node for 1 hr

(6.25s were the sprintf call here thats getting eliminated, 1s in the string(peer.ID()) call)

cason · 2024-04-22T07:17:04Z

Not sure about backporting this code though.

andynog

lgtm, I believe if it doesn't fully resolve the problems related to With and Add it does change the logic to pre-allocate of the Sprintf. Let's merge this @cason and ask Dev to test. Then we can decide to backport it.

Addresses #2840. The same approach taken for the `send()` method is also adopted for the `onReceive()` method. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 7673d74)

#2851) (#2877) Addresses #2840. The same approach taken for the `send()` method is also adopted for the `onReceive()` method. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #2851 done by [Mergify](https://mergify.com). Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…ng (#2984) In the metric `MessageReceiveBytesTotal`, the only value produced in the `message_type` label is `v1_Message`. This is because we are reading the `msg` variable before the message is unwrapped. This bug was recently introduced in #2851. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

…ng (#2984) In the metric `MessageReceiveBytesTotal`, the only value produced in the `message_type` label is `v1_Message`. This is because we are reading the `msg` variable before the message is unwrapped. This bug was recently introduced in #2851. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 33729e6)

…ng (backport #2984) (#2985) In the metric `MessageReceiveBytesTotal`, the only value produced in the `message_type` label is `v1_Message`. This is because we are reading the `msg` variable before the message is unwrapped. This bug was recently introduced in #2851. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #2984 done by [Mergify](https://mergify.com). Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

p2p/metrics: pre-allocate channel ID labels

c4f8046

cason mentioned this pull request Apr 18, 2024

Lower allocation overhead of metrics in peer.Send #2840

Closed

Daniel Cason added 2 commits April 18, 2024 17:58

p2p: refactor PeerReceiveBytesTotal metric labels

4a2dea4

p2p/metrics: pre-allocate peer ID label

0d33f38

cason linked an issue Apr 18, 2024 that may be closed by this pull request

Lower allocation overhead of metrics in peer.Send #2840

Closed

cason marked this pull request as ready for review April 18, 2024 16:02

cason requested a review from a team as a code owner April 18, 2024 16:02

cason requested a review from a team April 18, 2024 16:02

cason self-assigned this Apr 18, 2024

cason added backport-to-v0.34.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Apr 18, 2024

cason changed the title ~~feat(p2p): pre-allocate channel ID labels for metrics~~ feat(p2p): pre-allocate static labels used to produce metrics Apr 18, 2024

cason marked this pull request as draft April 18, 2024 16:07

Daniel Cason added 5 commits April 18, 2024 18:14

p2p/metrics: labelsByChID as a peer type field

5e3dec1

p2p: move labels cache to metricsLabelCache type

ff19e8e

p2p/metrics: comments documenting added methods

f8d25e2

p2p: minor refactor on send/receive metric labels

97de12e

p2p/metrics: renamed AddChID to RegisterChID

e01603b

cason marked this pull request as ready for review April 18, 2024 16:37

cason commented Apr 18, 2024

View reviewed changes

Daniel Cason added 2 commits April 18, 2024 18:48

p2p/metrics: restoring metricLabelValue var

55701df

p2p/metrics: restoring metricLabelValue var

7bac634

andynog reviewed Apr 18, 2024

View reviewed changes

melekes approved these changes Apr 19, 2024

View reviewed changes

ValarDragon approved these changes Apr 20, 2024

View reviewed changes

adizere removed the backport-to-v0.34.x label Apr 22, 2024

andynog approved these changes Apr 23, 2024

View reviewed changes

cason removed backport-to-v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Apr 23, 2024

cason added this pull request to the merge queue Apr 23, 2024

Merged via the queue into main with commit 7673d74 Apr 23, 2024

cason deleted the cason/2840-chID-labels branch April 23, 2024 15:43

mergify bot mentioned this pull request Apr 23, 2024

feat(p2p): pre-allocate static labels used to produce metrics (backport #2851) #2877

Merged

4 tasks

hvanz mentioned this pull request May 2, 2024

fix(metric): message type in metric MessageReceiveBytesTotal is wrong #2984

Merged

4 tasks

mergify bot mentioned this pull request May 2, 2024

fix(metric): message type in metric MessageReceiveBytesTotal is wrong (backport #2984) #2985

Merged

4 tasks

Conversation

cason commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

cason Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

cason Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

cason Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

andynog Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ValarDragon Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andynog Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

cason Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

cason commented Apr 19, 2024

Uh oh!

ValarDragon commented Apr 19, 2024

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

cason commented Apr 19, 2024

Uh oh!

ValarDragon commented Apr 20, 2024

Uh oh!

ValarDragon commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason commented Apr 22, 2024

Uh oh!

andynog left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cason commented Apr 18, 2024 •

edited

Loading

ValarDragon Apr 19, 2024 •

edited

Loading

ValarDragon Apr 19, 2024 •

edited

Loading

ValarDragon commented Apr 20, 2024 •

edited

Loading

andynog left a comment •

edited

Loading