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

Several tweaks to networking Prometheus metrics #5636

Merged
merged 7 commits into from
Apr 16, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Apr 14, 2020

@mxinden Sorry to make you review all these changes, but you're "the Prometheus expert"!

This PR contains a couple of tweaks to the Prometheus metrics:

  • Removed connections (a gauge) and replaced it with connections_opened_total (a counter), so that we no longer lose information about connections opening and closing quickly.
  • Splits connections_closed by in and out for parity with connections_opened_total.
  • Removed opened_notification_streams (a gauge) and replaced it with notifications_streams_opened and notifications_streams_closed, for the same reason.
  • Added channel names to num_channels.
  • Added a reason label to the sub_libp2p_incoming_connections_handshake_errors_total metric.

@tomaka tomaka added A0-please_review Pull request needs code review. B1-clientnoteworthy labels Apr 14, 2020
@tomaka tomaka requested a review from mxinden April 14, 2020 15:46
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.

@mxinden Sorry to make you review all these changes, but you're "the Prometheus expert"!

More than happy to review all of these. Even happier that Prometheus adoption is happening so rapidly.

Comment on lines 848 to 849
connections_opened_total: CounterVec<U64>,
connections_closed_total: CounterVec<U64>,
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
connections_opened_total: CounterVec<U64>,
connections_closed_total: CounterVec<U64>,
connections_closed_total: CounterVec<U64>,
connections_opened_total: CounterVec<U64>,

Friendly reminder from Pierre from the past that this list is ordered alphabetically.

@@ -885,7 +886,7 @@ impl Metrics {
"sub_libp2p_connections_closed_total",
"Total number of connections closed, by reason"
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
"Total number of connections closed, by reason"
"Total number of connections closed, by reason and direction"

@NikVolf NikVolf added this to the 2.0 milestone Apr 16, 2020
@NikVolf NikVolf added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 16, 2020
@gnunicorn gnunicorn merged commit 36683a1 into master Apr 16, 2020
@gnunicorn gnunicorn deleted the tka-even-moaaaaaaaaaaaaaar-metrics branch April 16, 2020 13:18
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.

4 participants