feat: add opened/closed counters to protocol stream metrics#3407
feat: add opened/closed counters to protocol stream metrics#3407Nkovaturient wants to merge 8 commits intolibp2p:mainfrom
Conversation
|
How does this compare to #3374 ? |
|
@Faolain : sure, let me highlight how I approached this issue:-
|
tabcat
left a comment
There was a problem hiding this comment.
looking good. just some linting errors and a couple nit picks.
- Remove trailing spaces (libp2p, metrics-prometheus, metrics-simple) - Use TypedEventEmitter in track-protocol-stream spec instead of EventTarget + cast - Fix import order and add early-return comment in metrics-simple
|
Everything looks good. Just add the sinon-ts devDeps in the util package and ci should pass. Verify by running npm the dep-check script inside of the utils package. |
Done. |
|
@tabcat : LGTM, appreciate the detailed review and feedback. Will wait to hear from @dozyio. @Nkovaturient : Thank you so much for your persistence and continued efforts on this PR. Appreciate it. |
|
Implementation in opentelemetry metrics is missing, tests in prometheus metrics are missing. This may be a sign that platform-agnostic metrics like this should be pushed into a super class but that doesn't need to be done here. Also, consider splitting the |
… closes; add tests
|
@achingbrain : Noted and made relevant changes
I added |
| const label = `${stream.direction} ${stream.protocol}` | ||
|
|
||
| metrics.trackProtocolStream(stream) | ||
| stream.dispatchEvent(new Event('close')) |
There was a problem hiding this comment.
| stream.dispatchEvent(new Event('close')) | |
| stream.dispatchEvent(new StreamCloseEvent()) |
import from @libp2p/interface and replace all the other new Event('close')
| expect(scraped).to.include(`libp2p_protocol_streams_opened_total{${label}} 1`) | ||
| expect(scraped).to.include(`libp2p_protocol_streams_close_errors_total{${label}} 1`) |
There was a problem hiding this comment.
lets also check that closed it 0
expect(scraped).not.to.include(libp2p_protocol_streams_closed_total{${label}})
| label: 'protocol', | ||
| help: 'Total number of protocol streams closed, by direction and protocol' | ||
| }) | ||
| this.streamsCloseErrors = this.registerCounterGroup('libp2p_protocol_streams_close_errors_total', { |
There was a problem hiding this comment.
I think a better name would be libp2p_protocol_streams_reset_total
@achingbrain thoughts?
Description
Adds two new CounterGroup metrics alongside the existing
libp2p_protocol_streams_totalgauge:libp2p_protocol_streams_opened_total— incremented when a stream is openedlibp2p_protocol_streams_closed_total— incremented when a stream closes (via close, abort, or reset)Both counters use the same direction protocol label format as the existing gauge (e.g. inbound /identify/1.0.0). This enables:
rate(libp2p_protocol_streams_opened_total[30s])— stream open rate per protocolrate(libp2p_protocol_streams_closed_total[30s])— stream close rate per protocolopened_total - closed_total— current open streams, derivable from counters aloneThe counters are registered once in the constructor of both
PrometheusMetricsandSimpleMetricsand incremented insidetrackProtocolStream.A single
{ once: true }close listener handles all termination paths — graceful EOF, local .abort(), and remote reset — as defined by MessageStreamEvents.Fixes #3262.
Notes & open questions
metrics-opentelemetryis not changed in this PR — OpenTelemetry's Counter instrument is append-only by design (no decrement), which matches the counter semantics here, but the OpenTelemetry package currently implements all metrics via asingle Gauge type(see metric.ts).Change checklist
packages/libp2p/test/connection/index.spec.ts
packages/metrics-simple/test/track-protocol-stream.spec.ts