p2p: add message size, read error and concurrency metrics - #4680
p2p: add message size, read error and concurrency metrics#4680KaloyanTanev wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new inflight gauge update logic in observeHandlerStart can report incorrect values under concurrent handlers due to out-of-order Set calls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds Prometheus observability for libp2p “wire-level” behavior at the shared p2p choke points (Send, SendReceive, RegisterHandler) to support selecting safe per-protocol transport limits (per #4679).
Changes:
- Add histograms for logical protobuf message sizes (sent/received) and a counter for message read errors.
- Add inbound handling concurrency/latency instrumentation (in-flight gauge, concurrent-at-arrival histogram, handler duration histogram).
- Add internal tests covering the new metrics and update metrics documentation.
File summaries
| File | Description |
|---|---|
| p2p/sender.go | Observes sent request/message sizes and received response sizes; increments read-error counter on response read failure. |
| p2p/receive.go | Instruments inbound handler concurrency/duration; increments read-error counter on request read failures; observes received request and sent response sizes. |
| p2p/metrics.go | Adds the new Prometheus metrics plus helper functions and inflight tracking. |
| p2p/metrics_internal_test.go | Adds coverage validating size histograms, read-error counter behavior, and inflight/concurrency/duration metrics. |
| docs/metrics.md | Documents the newly added p2p_* metrics. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/operational issues in the new metrics instrumentation (sent-size observation timing, inflight map growth, and misleading sent-size help/docs) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
docs/metrics.md:119
- This description says sent messages are not labelled by peer because they are broadcast identically, but sent_message_size_bytes is also recorded for per-peer responses written by handlers. Reword to avoid implying per-peer uniformity and instead note it’s omitted to reduce cardinality.
p2p/metrics.go:115 - sent_message_size_bytes is also observed for per-peer stream responses (see receive.go calling observeSentMessage on resp), so the current help text is misleading when it claims the lack of a peer label is because messages are broadcast identically to all peers. Consider rewording to reflect the actual reason (cardinality reduction) without implying per-peer uniformity.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Relay read failures and the DKG sync protocol remain unobserved, leaving the advertised metrics incomplete.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
p2p/receive.go:57
- This does not cover every Charon protocol as stated:
dkg/sync/server.go:355registers/charon/dkg/sync/1.0.0/directly withSetStreamHandler, and its client opens streams directly atdkg/sync/client.go:231. Consequently that protocol contributes none of the new inbound concurrency/read/size metrics or outbound size metrics, leaving a gap in the fleet transport data. Instrument the sync protocol too, or move the observations to a layer shared by both handler implementations.
p2p/receive.go:88 - Relay resets are still excluded from this counter because the preceding
IsRelayErrorbranch returns first. Those areReadMsgfailures (and the outboundSendReceivepath counts them), sop2p_message_read_errors_totalcurrently has inconsistent and incomplete semantics. Increment the counter before returning from that branch as well.
p2p/metrics_internal_test.go:138 - This test sends only one request, so it never exercises the concurrent increment/decrement ordering or verifies that overlapping arrivals produce depths 1, 2, etc.—the central behavior of these new metrics. Please block the handler, issue multiple simultaneous requests, assert the peak gauge/histogram observations, then release them and assert the gauge returns to zero.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4680 +/- ##
==========================================
+ Coverage 65.40% 65.41% +0.01%
==========================================
Files 247 247
Lines 29983 30016 +33
==========================================
+ Hits 19609 19636 +27
- Misses 10373 10379 +6
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the suppressed comments in b0dfc5d: relay resets now excluded consistently on both read paths, and added a concurrency test asserting depths 1..3 and gauge reset. |
There was a problem hiding this comment.
🟡 Changes recommended
Direct-stream resets are incorrectly excluded from the read-error metrics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
p2p/receive.go:88
- This skips metrics for resets on direct streams too:
IsRelayErrormatches the transport-agnosticnetwork.ErrResetandErrResourceScopeClosedwithout verifying that the connection is relayed (p2p/ping.go:131-133). As a result, genuine direct-peer read failures never reach either new increment. Restrict the early return to relayed connections.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Structurally invalid but successfully read messages are omitted from both size and read-error telemetry.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
p2p/receive.go:108
- This observation runs only after
protonil.Check, so a protobuf that is read and decoded successfully but fails the required-field check is omitted from the size histogram. Since that case is not a read error either, malformed-but-readable traffic becomes invisible to both new metrics. Record the decoded size immediately after the read-error branches, before application-level validation.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed in the latest commit — received size is now observed before |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and coverage align with the stated scope; only a minor timeout comment needs correction.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
p2p/metrics.go:142
- This comment says the default receive timeout is about 10s, but
defaultRcvTimeoutis 5s inp2p/sender.go:28. Please keep the bucket rationale aligned with the actual default.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Add libp2p wire observability metrics, instrumented at the shared
Send/SendReceive/RegisterHandlerchoke points, covering all runtime charon protocols (the DKG-ceremony-onlydkg/syncprotocol registers its own raw stream handler and is out of scope):p2p_received_message_size_bytes{protocol, peer}andp2p_sent_message_size_bytes{protocol}: histograms of logical message sizes, with bucket edges at 8MiB/32MiB/128MiB so read-limit questions are exact bucket arithmetic.p2p_message_read_errors_total{protocol, peer}: read failures excluding benign relay resets, including messages exceeding a protocol read limit.p2p_inflight_requests{protocol, peer},p2p_concurrent_requests{protocol, peer}andp2p_handler_duration_seconds{protocol}: inbound handling concurrency (the histogram observes queue depth at every arrival, catching bursts between scrapes) and processing time.This provides the fleet data needed to pick the transport limits tracked in #4679 without touching honest traffic, and to validate the 32MB parsigex read limit proposed in #4637.
category: feature
ticket: #4679