Skip to content

feat: add opened/closed counters to protocol stream metrics#3407

Open
Nkovaturient wants to merge 7 commits intolibp2p:mainfrom
Nkovaturient:feat/track-protocol-stream-open-close-counters
Open

feat: add opened/closed counters to protocol stream metrics#3407
Nkovaturient wants to merge 7 commits intolibp2p:mainfrom
Nkovaturient:feat/track-protocol-stream-open-close-counters

Conversation

@Nkovaturient
Copy link
Copy Markdown

@Nkovaturient Nkovaturient commented Mar 14, 2026

Description

Adds two new CounterGroup metrics alongside the existing libp2p_protocol_streams_total gauge:

  • libp2p_protocol_streams_opened_total — incremented when a stream is opened
  • libp2p_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 protocol
  • rate(libp2p_protocol_streams_closed_total[30s]) — stream close rate per protocol
  • opened_total - closed_total — current open streams, derivable from counters alone

The counters are registered once in the constructor of both PrometheusMetrics and SimpleMetrics and incremented inside trackProtocolStream.
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-opentelemetry is 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 a single Gauge type (see metric.ts).

A follow-up issue or PR scoped to that package alone would be cleaner than mixing it into this one.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works
packages/libp2p/test/connection/index.spec.ts
npm run test:node -- --files dist/test/connection/index.spec.js 2>&1

> libp2p@3.1.6 test:node
> aegir test -t node --cov --files dist/test/connection/index.spec.js

build

> libp2p@3.1.6 build
> aegir build

[17:42:18] tsc [started]
[17:42:19] tsc [completed]
[17:42:19] esbuild [started]
[17:42:20] esbuild [completed]
test node.js


  connection
     should not require local or remote addrs
     should append remote peer id to address if not already present
     should not append remote peer id to address if present
     should have properties set
     should get the metadata of an open connection
     should return an empty array of streams
     should be able to create a new stream
     should be able to close the connection after being created
     should be able to close the connection after opening a stream
     should remove streams that close
     should remove streams that error
     should fail to create a new stream if the connection is closing
     should fail to create a new stream if the connection is closed
     should limit the number of incoming streams that can be opened using a protocol (254ms)
     should limit the number of outgoing streams that can be opened using a protocol
     should allow overriding the number of outgoing streams that can be opened using a protocol without a handler
     should support outgoing stream middleware
     should support incoming stream middleware (102ms)
     should not call outbound middleware if previous middleware errors
     should not call inbound middleware if previous middleware errors (102ms)
     should call trackProtocolStream when a new outbound stream is opened
     should call trackProtocolStream when an inbound stream is opened (102ms)


  22 passing (601ms)
packages/metrics-simple/test/track-protocol-stream.spec.ts
npm run test:node -- --files dist/test/track-protocol-stream.spec.js 2>&1

> @libp2p/simple-metrics@2.0.10 test:node
> aegir test -t node --cov --files dist/test/track-protocol-stream.spec.js

build

> @libp2p/simple-metrics@2.0.10 build
> aegir build

[17:38:26] tsc [started]
[17:38:27] tsc [completed]
[17:38:27] esbuild [started]
[17:38:27] esbuild [completed]
test node.js


  SimpleMetrics - protocol stream counters
     increments opened counter when trackProtocolStream is called
     increments closed counter on graceful stream close
     increments closed counter exactly once because listener is registered with once:true
     accumulates counts correctly across multiple streams
     does not track a stream with no protocol


  5 passing (6ms)

@Nkovaturient Nkovaturient requested a review from a team as a code owner March 14, 2026 12:43
@Faolain
Copy link
Copy Markdown

Faolain commented Mar 14, 2026

How does this compare to #3374 ?

@Nkovaturient
Copy link
Copy Markdown
Author

Nkovaturient commented Mar 15, 2026

@Faolain : sure, let me highlight how I approached this issue:-

As per my understanding, that's the fundamental problem: metric registration should happen once, not per-stream.

  • This PR doesn't touch connection.ts at all. The two existing trackProtocolStream(muxedStream) call sites there are already correct.
    Instead, the counters are registered once in the constructor of PrometheusMetrics and SimpleMetrics — the same pattern as every other metric in those packages — and incremented inside trackProtocolStream, which is the single place that already owns all stream lifecycle metric logic (byte tracking, the existing gauge).

All tests pass: 22 in the connection spec and 5 in the new metrics-simple spec covering the counter behaviour directly.

Copy link
Copy Markdown
Collaborator

@tabcat tabcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good. just some linting errors and a couple nit picks.

@tabcat tabcat requested a review from dozyio March 19, 2026 09:21
Nkovaturient and others added 3 commits March 19, 2026 15:47
- 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
@tabcat
Copy link
Copy Markdown
Collaborator

tabcat commented Mar 29, 2026

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.

@Nkovaturient
Copy link
Copy Markdown
Author

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.

@seetadev seetadev self-requested a review March 31, 2026 14:29
@seetadev
Copy link
Copy Markdown

@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.

@achingbrain
Copy link
Copy Markdown
Member

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 close metric into close (e.g. clean exit) and error (e.g. failure). Total stream results can be computed from summing both exit states.

@Nkovaturient
Copy link
Copy Markdown
Author

@achingbrain : Noted and made relevant changes

  • I kept libp2p_protocol_streams_opened_total and split how stream ends into libp2p_protocol_streams_closed_total (clean close, no error on StreamCloseEvent) and libp2p_protocol_streams_close_errors_total (close with error set, e.g. abort/reset).
  • wired the same close listener branch (evt.error != null) in metrics-simple, metrics-prometheus, and metrics-opentelemetry trackProtocolStream, and I registered the new counter group in the OTEL constructor so OTEL matches the other backends.
  • Further, extending metrics-simple track-protocol-stream.spec.ts for the error path (StreamAbortEvent) and the new metric.
  • Also, added metrics-prometheus/test/track-protocol-stream.spec.ts that scrapes prom-client for opened / clean close / abort.
  • henceforth, adding the metrics-opentelemetry/test/track-protocol-stream.spec.ts with an in-memory OTEL exporter;
    • here, I fixed the Stream typing with as unknown as Stream on the minimal test double, and then fixed flaky second-test failures by using one MeterProvider per describe, exporter.reset() in beforeEach, and a single after() shutdown instead of recreating the global provider every test.

I added @opentelemetry/sdk-metrics as a devDependency on @libp2p/opentelemetry-metrics so those tests can drive a real SDK reader/exporter instead of sinon-ts (cuz sinon-ts broke the event wiring; the double cast was enough for types.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track protocol stream open/close count in metrics

5 participants