p2p: add message size, read error and concurrency metrics - #4680
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.45% 65.47% +0.01%
==========================================
Files 246 246
Lines 30020 30053 +33
==========================================
+ Hits 19651 19678 +27
- Misses 10368 10374 +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
59ad34f to
f3ae169
Compare
|
* app/eth1wrap: avoid startup hang (#4694) * app/eth1wrap: add regression test for ClientVersion reconnect deadlock Reproduce charon#4689: ClientVersion can hang after RPC timeout when reconnectCh is full, wedging WaitConnected / startup against a silent EL. Co-authored-by: Cursor <cursoragent@cursor.com> * app/eth1wrap: avoid startup hang when execution client is silent Make maybeReconnect non-blocking and do not hold the client mutex across RPC calls so WaitConnected can time out instead of deadlocking (charon#4689). Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> * app/eth1wrap: don't close reconnectCh on shutdown (#4699) * core/parsigex: bind partial signature sender to share index (#4705) * p2p: recover stream handler panics to avoid node crash (#4706) Backport of #4706 adapted for the v1.11 release branch: drops the message_handler_panics_total metric, whose supporting infrastructure (#4680) is not present on this branch. * build(deps): Bump the go-dependencies group with 7 updates (#4674) Bumps the go-dependencies group with 7 updates: | Package | From | To | | --- | --- | --- | | [github.com/showwin/speedtest-go](https://github.com/showwin/speedtest-go) | `1.8.1` | `1.8.2` | | [go.opentelemetry.io/otel](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlptrace](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | | [go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | | [go.opentelemetry.io/otel/exporters/stdout/stdouttrace](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | | [go.opentelemetry.io/otel/sdk](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | | [go.opentelemetry.io/otel/trace](https://github.com/open-telemetry/opentelemetry-go) | `1.45.0` | `1.46.0` | Updates `github.com/showwin/speedtest-go` from 1.8.1 to 1.8.2 - [Release notes](https://github.com/showwin/speedtest-go/releases) - [Changelog](https://github.com/showwin/speedtest-go/blob/master/docs/release.md) - [Commits](showwin/speedtest-go@v1.8.1...v1.8.2) Updates `go.opentelemetry.io/otel` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlptrace` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) Updates `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) Updates `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) Updates `go.opentelemetry.io/otel/sdk` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) Updates `go.opentelemetry.io/otel/trace` from 1.45.0 to 1.46.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-go@v1.45.0...v1.46.0) --- updated-dependencies: - dependency-name: github.com/showwin/speedtest-go dependency-version: 1.8.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/exporters/stdout/stdouttrace dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/sdk dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies - dependency-name: go.opentelemetry.io/otel/trace dependency-version: 1.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-dependencies ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * core: perform sync committee duties for exited validators (#4712) * core: perform sync committee duties for exited validators * Add extra test --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: b0a7 <127276179+b0a7@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>



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