Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(metrics)!: expose identify metrics for connected peers only #3325

Merged
merged 10 commits into from
May 15, 2023

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jan 15, 2023

Description

Previously we would increase a counter / gauge / histogram on each received identify information. These metrics are missleading, as e.g. they depend on the identify interval and don't represent the set of currently connected peers.

With this commit, identify information is tracked for the currently connected peers only. Instead of an increase on each received identify information, metrics represent the status quo (Gauge).

Example:

\# HELP libp2p_libp2p_identify_remote_protocols Number of connected nodes supporting a specific protocol, with "unrecognized" for each peer supporting one or more unrecognized protocols...
\# TYPE libp2p_libp2p_identify_remote_protocols gauge
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/id/push/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/id/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/ping/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="unrecognized"} 1
\# HELP libp2p_libp2p_identify_remote_listen_addresses Number of connected nodes advertising a specific listen address...
\# TYPE libp2p_libp2p_identify_remote_listen_addresses gauge
libp2p_libp2p_identify_remote_listen_addresses_total{listen_address="/ip4/tcp"} 1
libp2p_libp2p_identify_remote_listen_addresses_total{listen_address="/ip4/udp/quic"} 1
\# HELP libp2p_libp2p_identify_local_observed_addresses Number of connected nodes observing the local node at a specific address...
\# TYPE libp2p_libp2p_identify_local_observed_addresses gauge
libp2p_libp2p_identify_local_observed_addresses_total{observed_address="/ip4/tcp"} 1

Notes

Depends on new release of https://github.com/prometheus/client_rust that includes prometheus/client_rust#82.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Previously we would increase a counter / gauge / histogram on each received identify information.
These metrics are missleading, as e.g. they depend on the identify interval and don't represent the
set of currently connected peers.

With this commit, identify information is tracked for the currently connected peers only. Instead of
an increase on each received identify information, metrics represent the status quo (Gauge).

Example:

```
\# HELP libp2p_libp2p_identify_remote_protocols Number of connected nodes supporting a specific protocol, with "unrecognized" for each peer supporting one or more unrecognized protocols...
\# TYPE libp2p_libp2p_identify_remote_protocols gauge
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/id/push/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/id/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="/ipfs/ping/1.0.0"} 1
libp2p_libp2p_identify_remote_protocols_total{protocol="unrecognized"} 1
\# HELP libp2p_libp2p_identify_remote_listen_addresses Number of connected nodes advertising a specific listen address...
\# TYPE libp2p_libp2p_identify_remote_listen_addresses gauge
libp2p_libp2p_identify_remote_listen_addresses_total{listen_address="/ip4/tcp"} 1
libp2p_libp2p_identify_remote_listen_addresses_total{listen_address="/ip4/udp/quic"} 1
\# HELP libp2p_libp2p_identify_local_observed_addresses Number of connected nodes observing the local node at a specific address...
\# TYPE libp2p_libp2p_identify_local_observed_addresses gauge
libp2p_libp2p_identify_local_observed_addresses_total{observed_address="/ip4/tcp"} 1
```
@thomaseizinger
Copy link
Contributor

Depends on new release of prometheus/client_rust that includes prometheus/client_rust#82.

Setting to draft until ready for review.

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

This pull request has merge conflicts. Could you please resolve them @mxinden? 🙏

@mxinden mxinden requested a review from thomaseizinger March 26, 2023 14:55
@mxinden
Copy link
Member Author

mxinden commented Mar 26, 2023

@thomaseizinger can you give this pull request a review? It builds on top of prometheus/client_rust#82. prometheus/client_rust#82 has not yet been released. Would be good to get some input on a usage of prometheus/client_rust#82 before I release prometheus/client_rust#82.

@thomaseizinger
Copy link
Contributor

Looks fine to me. I am not fond of the name "collector". What about "static" or "const" metrics?

@mxinden
Copy link
Member Author

mxinden commented Apr 4, 2023

Looks fine to me. I am not fond of the name "collector". What about "static" or "const" metrics?

The name is used across all Prometheus client libraries and well documented. I agree that it is not intuitive. That said, I think staying consistent is the better approach.

misc/metrics/src/identify.rs Outdated Show resolved Hide resolved
misc/metrics/src/identify.rs Show resolved Hide resolved
Comment on lines 202 to 214
let mut protocols: Vec<_> = peer_info
.protocols
.iter()
.map(|p| {
if ALLOWED_PROTOCOLS.contains(&p.as_bytes()) {
p.to_string()
} else {
"unrecognized".to_string()
}
})
.collect();
protocols.sort();
protocols.dedup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extract this into a helper and thus make the loop a bit easier to read?

misc/metrics/src/identify.rs Outdated Show resolved Hide resolved
@mxinden mxinden marked this pull request as ready for review May 14, 2023 06:56
@mxinden
Copy link
Member Author

mxinden commented May 14, 2023

Note that this pull request now updates libp2p-metrics, libp2p-gossipsub and metrics-example to prometheus-client v0.21.0.

@mxinden
Copy link
Member Author

mxinden commented May 14, 2023

This pull request is ready to merge from my end. @thomaseizinger would you mind taking another look?

@mxinden mxinden requested a review from thomaseizinger May 14, 2023 07:06
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM.

misc/metrics/CHANGELOG.md Outdated Show resolved Hide resolved
static PROTOCOLS_DESCRIPTOR: Lazy<Descriptor> = Lazy::new(|| {
Descriptor::new(
"remote_protocols",
"Number of connected nodes supporting a specific protocol, with \
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an r#""# string to avoid the escaping?

misc/metrics/src/identify.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title feat(metrics)!: Expose number of X for connected peers only feat(metrics)!: expose identify metrics for connected peers only May 14, 2023
mxinden and others added 2 commits May 15, 2023 03:50
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@mergify mergify bot merged commit 2d83725 into libp2p:master May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants