fix: skip prometheus metrics trackProtocolStream for identify - #8958
fix: skip prometheus metrics trackProtocolStream for identify#8958lodekeeper wants to merge 1 commit into
Conversation
Work around an identify EOF race in @libp2p/prometheus-metrics v5.
trackProtocolStream() attaches a 'message' event listener on protocol
streams immediately after negotiation. For outbound /ipfs/id/1.0.0
streams, this listener can fire (via queueMicrotask in
dispatchReadBuffer) before identify's pb.read() attaches its own
reader, consuming the identify response data and causing EOF.
Root cause trace:
1. connection.newStream() negotiates /ipfs/id/1.0.0 via MSS
2. connection.js calls metrics.trackProtocolStream(stream) which
adds addEventListener('message', ...) to count bytes
3. MSS unwrap() pushes unread protocol data back to the stream
4. dispatchReadBuffer fires via queueMicrotask — metrics listener
consumes all data from readBuffer
5. finally block: readBuffer.byteLength === 0 && remoteWriteStatus
=== 'closed' → sets readStatus = 'closed'
6. identify's pb.read() sees EOF — peer stays Unknown
A/B validation (90s local mainnet samples):
- With tracking enabled: 35 opens, 28 failures (80%)
- With tracking disabled: 24 opens, 0 failures (0%)
- Skip identify only: 33 opens, 0 failures (0%)
The fix wraps the prometheus metrics service to skip
trackProtocolStream for /ipfs/id/1.0.0 streams only, preserving
all other protocol stream metrics.
Summary of ChangesHello @lodekeeper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical regression introduced with the libp2p v3 upgrade, where a race condition in the Prometheus metrics library caused identify protocol streams to fail. By implementing a targeted workaround that prevents metrics from prematurely consuming data from identify streams, the change significantly improves peer identification reliability and reduces the number of "Unknown" peers in the network. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides an effective and well-validated workaround for a critical regression introduced by the libp2p v3 upgrade. The detailed root cause analysis and A/B testing results clearly demonstrate the necessity and efficacy of skipping Prometheus metrics tracking for "/ipfs/id/1.0.0" streams to prevent identify stream failures. This targeted fix ensures that a high percentage of connected peers are no longer misidentified as 'Unknown' clients, significantly improving network monitoring and stability.
| const originalTrackProtocolStream = metrics.trackProtocolStream.bind(metrics); | ||
| metrics.trackProtocolStream = ((stream) => { | ||
| if (stream.protocol === "/ipfs/id/1.0.0") { | ||
| return; | ||
| } | ||
| originalTrackProtocolStream(stream); | ||
| }) as typeof metrics.trackProtocolStream; |
There was a problem hiding this comment.
While this direct method reassignment (metrics.trackProtocolStream = ...) effectively implements the workaround, directly modifying methods of external library instances can be fragile. If the prometheusMetrics library's internal implementation of trackProtocolStream changes (e.g., how it uses this or other internal state), this patch might break or behave unexpectedly. For a more robust solution, consider creating a custom metrics wrapper class that implements the Metrics interface and explicitly delegates calls to an internal prometheusMetrics instance, overriding trackProtocolStream with your custom logic. This provides better encapsulation and makes the dependency on the external library's internal structure less direct.
There was a problem hiding this comment.
@gemini-code-assist Good point about fragility. This is intentionally a minimal workaround — the proper fix belongs upstream in @libp2p/prometheus-metrics (the _track() method should use a non-consuming observer pattern rather than addEventListener('message', ...)). A full wrapper class adds complexity for what should be a temporary patch. If the upstream fix lands, we remove this entirely. If trackProtocolStream changes signature, the TypeScript compiler will catch it at build time since we preserve the type via as typeof metrics.trackProtocolStream.
There was a problem hiding this comment.
That's a fair point regarding the temporary nature of the fix and the upstream root cause. Given that the proper solution lies in the _track() method of @libp2p/prometheus-metrics and that TypeScript provides compile-time checks for signature changes, this minimal workaround is a pragmatic approach for the interim.
|
merged changes into #8960 for further testing |
|
Thanks for picking this up! Let me know if you need anything else for #8960 testing. 👍 |
Squash merged ChainSafe#8958 and reverted ChainSafe#8955. Deployed to `feat1`
2 participants
Motivation
After the libp2p v3 upgrade (#8890), ~20-30% of connected peers remain as "Unknown" client on monitored nodes (feat1), compared to ~3-5% on stable/unstable. This is caused by identify stream failures — the identify response data is consumed before
identify()can read it.Root Cause
@libp2p/prometheus-metricsv5trackProtocolStream()adds amessageevent listener on protocol streams immediately after negotiation. For outbound/ipfs/id/1.0.0streams, this listener fires (viaqueueMicrotaskindispatchReadBuffer) before identify'spb.read()attaches its own reader, consuming all identify response data and causing EOF.Trace:
connection.newStream()negotiates/ipfs/id/1.0.0via MSSconnection.jscallsmetrics.trackProtocolStream(stream)→ addsaddEventListener('message', ...)to count bytesunwrap()pushes unread protocol data back to the streamdispatchReadBufferfires viaqueueMicrotask— metrics listener consumes all data fromreadBufferfinallyblock:readBuffer.byteLength === 0 && remoteWriteStatus === 'closed'→ setsreadStatus = 'closed'pb.read()sees EOF → peer stays UnknownThis is a libp2p v3 regression: in v2, the stream API was iterator-based (pull model) so the metrics listener was harmless. In v3, it's event-based (push model) where
addEventListeneractively consumes from thereadBuffer.Fix
Wrap the prometheus metrics service to skip
trackProtocolStreamfor/ipfs/id/1.0.0streams only, preserving all other protocol stream byte-counting metrics.Validation
A/B testing on local mainnet (90-second samples each):
Extended clean validation run (120s,
--logLevel debug, no debug patches):Error setting agentVersion for the peer: 0Upstream
The underlying issue is in
@libp2p/prometheus-metrics_track()method which uses a consumingaddEventListener('message', ...)pattern that races with protocol handlers. An upstream fix should use a non-consuming observer, but that requires changes to@libp2p/utilsAbstractMessageStream. This PR provides a targeted Lodestar-side workaround.Note
This PR was authored with AI assistance (Lodekeeper 🌟). Root cause analysis, instrumentation, A/B validation, and fix implementation were performed by the AI agent with human oversight.