metrics: Expose litep2p metrics in an agnostic manner #294

lexnv · 2024-11-29T14:53:16Z

This PR optionally exposes litep2p metrics in an agnostic manner.

API Design

Litep2p supports at the moment the following primitives to register and operate on metrics:

//github.com/ A registry for metrics.
pub trait MetricsRegistryT: Send + Sync {
    //github.com/ Register a new counter.
    fn register_counter(&self, name: String, help: String) -> Result<MetricCounter, Error>;

    //github.com/ Register a new gauge.
    fn register_gauge(&self, name: String, help: String) -> Result<MetricGauge, Error>;
}


//github.com/ Represents a metric that can only go up.
pub trait MetricCounterT: Send + Sync {
    //github.com/ Increment the counter by `value`.
    fn inc(&self, value: u64);
}

//github.com/ Represents a metric that can arbitrarily go up and down.
pub trait MetricGaugeT: Send + Sync {
    //github.com/ Set the gauge to `value`.
    fn set(&self, value: u64);

    //github.com/ Increment the gauge.
    fn inc(&self);
...
}

Around these primitives, substrate can expose prometheus metrics, and if need be update seamlessly to other metric crates (like prometheus-client).

Metrics Exposed

The exposed metrics inform the user about the state of litep2p components (like kademlia number of store elements, identify negotiating substreams etc) and help developers detect abnormal behavior (like memory leaks / unbounded growth / stalls in some protocols).

Transport Manager

number of incoming connections
number of outgoing connections
number of opening errors
number of managed peers
number of pending connections

Transport Layer (TCP + Websocket)

number of pending inbound connections
number of pending open connections
number of raw unnegotiated connections
number of pending substreams

Kademlia

number of total peers
number of engine running queries
number of executor queries
number of memory store records
number of memory store local providers
number of memory store providers
number of memory store provider refreshes

Identify / Ping

number of total peers
number of pending inbound substreams
number of pending outbound substreams

Request Response Protocol

These metrics are exposed for every req-resp protocol.

number of connected peers
number of pending dials
number of pending inbound substreams
number of pending inbound requests
number of pending outbound
number of outbound cancels
number of pending outbound responses

Notification Protocol

These metrics are exposed for every notification protocol.

number of connected peers
number of outbound initiated handshakes
number of outbound substreams
number of validation
number of ready substream handshakes
number of timers

Dashboards

Review notes: Metric traits are define in src/metrics.rs, they should give enough background for the metric registration / update that is happening in the rest of the code

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Ideally this should be Into<String>, but that way the we cannot be object safe Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Similar to #296, there is a possibility of leaking memory in the following edge-case: - T0: Connection is established and outbound substream is initiated with peer - This maps the substream ID to the request bytes information - T1: Connection is closed before the service has a chance to report `TransportEvent::SubstreamOpened` or `TransportEvent::SubstreamOpenFailure` In this case, if we connect and immediately disconnect with a request in flight, we are effectively leaking the request bytes. Detected by: - #294 ### Dashboard - We are leaking ~111 requests over 3 days timespan: <img width="1484" alt="Screenshot 2024-12-03 at 10 41 01" src="https://github.com/user-attachments/assets/f6701017-4add-4aa1-aee1-e1f8d33d54f3"> cc @paritytech/networking Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

This PR fixes a subtle memory leak that can happen in the following edge-case situation: - connection is established and substream outbound is initiated with remote peer - the substream ID is tracked until the substream either completes successfully or fails - the connection is closed soon after, leading to no substream events ever being generated For this edge-cases, we need to remove the tracking of the substream ID when the connection is reported as closed. This has been detected after running a node for more than 2 days with the following generic metrics PR: - #294 Closes: #295 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

dmitry-markin · 2025-01-13T08:21:52Z

src/error.rs

+    #[error("Invalid metric: `{0}`")]
+    MetricError(String),


Is this used anywhere?

dmitry-markin · 2025-01-13T08:25:05Z

src/protocol/libp2p/identify.rs

+                pending_outbound: registry.register_gauge(
+                    "litep2p_identify_pending_outbound".into(),
+                    "Pending outbound substreams".into(),
+                )?,
+                pending_inbound: registry.register_gauge(
+                    "litep2p_identify_pending_inbound".into(),
+                    "Pending inbound substreams".into(),
+                )?,


Why do we need this info reported?

dmitry-markin · 2025-01-13T08:32:50Z

src/protocol/libp2p/kademlia/mod.rs

+    peers: MetricGauge,
+
+    mem_store_records: MetricGauge,
+    mem_store_providers: MetricGauge,
+    mem_store_local_providers: MetricGauge,
+    mem_store_provider_refresh: MetricGauge,
+
+    engine_queries: MetricGauge,
+    executor_queries: MetricGauge,


It may be of interest to also know the size of the routing table.

dmitry-markin · 2025-01-13T08:39:28Z

src/protocol/libp2p/ping/mod.rs

+                pending_outbound: registry.register_gauge(
+                    "litep2p_ping_pending_outbound".into(),
+                    "Pending outbound substreams".into(),
+                )?,
+                pending_inbound: registry.register_gauge(
+                    "litep2p_ping_pending_inbound".into(),
+                    "Pending inbound substreams".into(),
+                )?,


As with Identify, why do we need this info? Metrics reporting should be optimized in the prometheus libs, but it's not completely free. Also, storing them in the database is not free either. So, I would keep the amount of metrics reported to the necessary minimum.

Also, Identify & Ping peer counts should be equal to the number of connected peers, so I would not report Identify & Ping metrics at all, and just keep one global peer counter.

dmitry-markin · 2025-01-13T08:42:27Z

src/protocol/notification/mod.rs

+struct Metrics {
+    connected_peers: MetricGauge,
+    pending_outbound_num: MetricGauge,
+    pending_outbound_handshake_num: MetricGauge,
+    ready_substreams_handshake_num: MetricGauge,
+    pending_validations_num: MetricGauge,
+    timers_num: MetricGauge,
 }


I would reduce all these counters to just one MetricCounter — the number of discovered peers via mDNS.

dmitry-markin · 2025-01-13T09:09:08Z

src/transport/tcp/connection.rs

+
+    //github.com/ Metric incremented when the connection starts
+    //github.com/ and decremented when the connection closes.
+    pub _active_connections_num: ScopeGaugeMetric,


nit: starting with an underscore looks confusing, like it's not implemented or so. May be #[allow_unused]?

dmitry-markin · 2025-01-13T09:26:59Z

src/transport/tcp/mod.rs

+    //github.com/ The following metrics are shared with all TCP connections.
+    active_connections_num: MetricGauge,
+    pending_substreams_num: MetricGauge,
+}


As with other metrics, I would may be reconsider what we need and what we don't. For example, I am not sure wee need the sizes of all internal containers. At the same time, the total number of substreams may be of interest.

dmitry-markin · 2025-01-13T09:29:33Z

src/transport/webrtc/mod.rs

+    fn new(
+        context: TransportHandle,
+        config: Self::Config,
+        _registry: Option<crate::metrics::MetricsRegistry>,


Do we need an issue to implement this in the future?

dmitry-markin · 2025-01-13T09:32:04Z

src/transport/websocket/mod.rs

+    //github.com/ The following metrics are shared with all TCP connections.
+    active_connections_num: MetricGauge,
+    pending_substreams_num: MetricGauge,
+}


dq: What metrics we need and what don't + number of open substreams.

dmitry-markin · 2025-01-13T09:32:47Z

src/transport/quic/mod.rs

@@ -215,6 +215,7 @@ impl TransportBuilder for QuicTransport {
    fn new(
        context: TransportHandle,
        mut config: Self::Config,
+        _registry: Option<crate::metrics::MetricsRegistry>,


Do we need an issue to implement this?

lexnv · 2025-01-13T12:27:51Z

Thanks for the review here @dmitry-markin 🙏 Offhand, everything makes sense!

Will try to get to this soon, immediately after we gather a bit more info on: paritytech/polkadot-sdk#7076 (comment)

lexnv added 12 commits November 29, 2024 13:18

cargo: Add metrics feature flag with prometheus

2245703

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

litep2p/config: Expose prometheus metrics in config

c6154da

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

cargo: Make litep2p agnostic of metrics

f3948c1

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metrics: Add abstraction layers over metrics

31144bf

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Expose metrics to litep2p transport layers

77332aa

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tcp: Extend collected metrics at intervals for code simplicity

3ec010c

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tcp: Selectively poll interval if the metrics are enabled

5936083

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metric: Extend Gauge methods with inc / dec

c68ef20

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metrics: Add scope RAII metric counter

907312f

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metrics: Make register methods less restrictive

4362919

Ideally this should be Into<String>, but that way the we cannot be object safe Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tcp: Extend with connection metrics

5d91741

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

websocket: Add specific metrics

05f5741

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv added the enhancement New feature or request label Nov 29, 2024

lexnv self-assigned this Nov 29, 2024

lexnv added 10 commits November 29, 2024 17:43

req-resp: Propagate metrics

f6d478d

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

notifications: Expose metrics

b5e80e2

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

ping: Add metrics

fc70eea

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

identify: Propagate metrics

b91b8a6

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

kad: Add metrics

48cb7ca

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

transport/manager: Add metrics

27042cb

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Simplify metrics collection

2de8a68

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tests: Adjust testing

142e08f

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

protocol: To metric name string

7079ade

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

transport: Simplify collection

e543853

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

This was referenced Dec 2, 2024

notifications: Investigate possible memory leak with pending outbound substream tracking #295

Closed

notification: Fix memory leak of pending substreams #296

Merged

req-resp: Fix memory leak of pending substreams #297

Merged

lexnv added 2 commits December 3, 2024 10:17

tcp: Decrement metrics on connection closed

17786b6

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

websocket: Decrement metrics on connection closed

e43ee71

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv added 6 commits December 3, 2024 13:58

websocket: Use ref mut self

fd084c8

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

websocket: Fix clone move

09bd786

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metrics: Add MeteredFuturesStream

cbc273d

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

metrics: Use MeteredFuturesStream for tcp and websocket

deb8b1a

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Merge remote-tracking branch 'origen/master' into lexnv/metrics

cf3faf1

Fix docs

125ee19

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv changed the title ~~wip: Metrics~~ metrics: Expose litep2p metrics in an agnostic manner Dec 3, 2024

lexnv and others added 2 commits December 3, 2024 16:08

Merge branch 'master' into lexnv/metrics

8ccf710

Merge remote-tracking branch 'origen/master' into lexnv/metrics

dfd4b0d

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv mentioned this pull request Dec 17, 2024

quic: Expose transport layer metrics #309

Open

dmitry-markin reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: Expose litep2p metrics in an agnostic manner #294

metrics: Expose litep2p metrics in an agnostic manner #294

lexnv commented Nov 29, 2024 •

edited

Loading

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

dmitry-markin Jan 13, 2025

lexnv commented Jan 13, 2025

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

metrics: Expose litep2p metrics in an agnostic manner #294

Are you sure you want to change the base?

metrics: Expose litep2p metrics in an agnostic manner #294

Conversation

lexnv commented Nov 29, 2024 • edited Loading

API Design

Metrics Exposed

Transport Manager

Transport Layer (TCP + Websocket)

Kademlia

Identify / Ping

Request Response Protocol

Notification Protocol

Dashboards

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv commented Jan 13, 2025

lexnv commented Nov 29, 2024 •

edited

Loading