Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Prometheus Metrics: Turn notifications_total counter into notifications_sizes histogram #5535

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Apr 6, 2020

Instead of reporting just the number of notifications sent and received through a counter, this PR now also reports their sizes in bytes through a histogram.

Consequently I renamed notifications_total to notifications_sizes.

@tomaka tomaka added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Apr 6, 2020
@tomaka tomaka requested a review from mxinden April 6, 2020 09:04
@tomaka tomaka added this to the 2.0 milestone Apr 6, 2020
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Small suggestion. Otherwise this looks great!

"sub_libp2p_notifications_sizes",
"Sizes of the notifications send to and received from all nodes"
),
buckets: vec![64.0, 256.0, 1024.0, 4.0 * 1024.0, 64.0 * 1024.0, 256.0 * 1024.0, 1024.0 * 1024.0],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
buckets: vec![64.0, 256.0, 1024.0, 4.0 * 1024.0, 64.0 * 1024.0, 256.0 * 1024.0, 1024.0 * 1024.0],
buckets: vec![64.0, 256.0, 1024.0, 4.0 * 1024.0, 16.0 * 1024.0, 64.0 * 1024.0, 256.0 * 1024.0, 1024.0 * 1024.0],

Aren't you missing this one here?

Also why not use https://docs.rs/prometheus/0.8.0/prometheus/fn.exponential_buckets.html?

@gnunicorn gnunicorn added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 6, 2020
@gnunicorn
Copy link
Contributor

gnunicorn commented Apr 6, 2020

benchmark test failing. please merge master. (changed title and tag to have it show up nicely in the changelogs)

@gnunicorn gnunicorn changed the title Turn notifications_total into notifications_sizes Prometheus Metrics: Turn notifications_total counter into notifications_sizes histogram Apr 6, 2020
@gnunicorn gnunicorn added B1-clientnoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Apr 6, 2020
@tomaka tomaka merged commit fc6ddaf into paritytech:master Apr 6, 2020
@tomaka tomaka deleted the notifs-sizes-prometheus branch April 6, 2020 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants