feat: add WebSockets metrics #2649

SgtPooki · 2024-08-05T21:05:39Z

Title

feat: add WebSockets metrics

Description

Creates two new metric counter groups for WebSockets: libp2p_websockets_dialer_events_total and libp2p_websockets_listener_events_total.

The following new metrics should be added:

> rg '.increment|.update' packages/transport-websockets/src
packages/transport-websockets/src/index.ts
157:      this.metrics?.dialerEvents.increment({ error: true })
166:        this.metrics?.dialerEvents.increment({ abort: true })
168:        this.metrics?.dialerEvents.increment({ error: true })
179:    this.metrics?.dialerEvents.increment({ connect: true })

packages/transport-websockets/src/socket-to-conn.ts
64:        metrics?.increment({ [`${metricPrefix}close_abort`]: true })
73:        metrics?.increment({ [`${metricPrefix}close_success`]: true })
76:        metrics?.increment({ [`${metricPrefix}close_error`]: true })
95:    metrics?.increment({ [`${metricPrefix}close`]: true })

packages/transport-websockets/src/listener.ts
94:              this.metrics?.errors.increment({ [`${this.addr} inbound_upgrade`]: true })
104:            this.metrics?.errors.increment({ [`${this.addr} inbound_closing_failed`]: true })
140:        this.metrics?.status.update({
147:      this.metrics?.errors.increment({ [`${this.addr} listen_error`]: true })
153:      this.metrics?.status.update({

This aligns pretty closely to TCP metrics now:

> rg '.increment|.update' packages/transport-tcp/src
packages/transport-tcp/src/index.ts
224:        this.metrics?.dialerEvents.increment({ error: true })
231:        this.metrics?.dialerEvents.increment({ timeout: true })
240:        this.metrics?.dialerEvents.increment({ connect: true })
246:        this.metrics?.dialerEvents.increment({ abort: true })

packages/transport-tcp/src/socket-to-conn.ts
64:    metrics?.increment({ [`${metricPrefix}timeout`]: true })
79:    metrics?.increment({ [`${metricPrefix}close`]: true })
93:    metrics?.increment({ [`${metricPrefix}end`]: true })

packages/transport-tcp/src/listener.ts
154:          this.metrics?.status.update({
162:        this.metrics?.errors.increment({ [`${this.addr} listen_error`]: true })
166:        this.metrics?.status.update({
187:      this.metrics?.events.increment({ [`${this.addr} error`]: true })
202:      this.metrics?.errors.increment({ [`${this.addr} inbound_to_connection`]: true })
251:          this.metrics?.errors.increment({ [`${this.addr} inbound_upgrade`]: true })
268:          this.metrics?.errors.increment({ [`${this.addr} inbound_closing_failed`]: true })

Fixes #1915

Notes & open questions

Callouts

index.ts
- There is no timeout metric because we don't have a socket timeout event already.
socket-to-conn.ts
- There is no timeout metric for websockets because we don't have a socket timeout event already.
- No end metric
- Some metrics are prefixed with close_ because we don't have general abort/close/end handlers already.
listener.ts
- No inbound_to_connection metric for websockets because socketToMaConn doesn't throw.
- No events.increment({ [${this.addr} error] for websockets because there is no existing socket.on('error' handler.

TODO based on comments:

follow the pattern established by @libp2p/tcp: the metric names have a prefix which is the listening address for the listener or omitted (e.g. defaults to an empty string) for the dialer. - done with 24341d1 (#2649)

Some questions:

Do we want less events? These seem like a lot but I wanted to throw something together.
Do we want more listener events in the server.ts instead of in listener.ts ?
Is the socket_close_success in socket-to-conn.ts called even after an abort event? we may want to move it if so.
Do we want an error event for WS listener error events? i.e. in this.server.on('error', (err: Error) => {
For the listener, a metricPrefix is added which is currently set to ${listeningAddrDetails?.transport}_${listeningAddrDetails?.host}:${listeningAddrDetails?.port} because WebSocket listener may be listening on /tls/ws or /wss or /ws -- Do we need .transport ?
Is PR comments clarifying metrics and usage enough or is there another place I need to change? https://github.com/libp2p/js-libp2p/blob/main/doc/METRICS.md doesn't list out metrics and explanations, so I figured this PR would be a good placeholder at the least.
Do we need to add any tests? I didn't see any in TCP: https://github.com/search?q=repo%3Alibp2p%2Fjs-libp2p+path%3A%2F%5Epackages%5C%2Ftransport-tcp%5C%2Ftest%5C%2F%2F+metric&type=code

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

SgtPooki · 2024-08-06T01:18:44Z

seems like webrtc firefox test is failing: https://github.com/libp2p/js-libp2p/actions/runs/10256017997/job/28381208851?pr=2649#step:5:2369, which shouldn't be related to these changes at all

achingbrain · 2024-08-06T10:05:39Z

The Firefox thing is documented here - #2642 - unrelated to this PR, as you say.

achingbrain · 2024-08-09T11:29:11Z

I think the various "start" events might be unnecessary? That is, we care about successes vs error/abort, the sum of which should be equal to the starts, so we can derive that metric if we need it but it doesn't seem particularly useful on its own?

For the maconn metrics, perhaps we should follow the pattern established by @libp2p/tcp - the metric names have a prefix which is the listening address for the listener or omitted (e.g. defaults to an empty string) for the dialer.

A good starting point apart from that, I think.

achingbrain

Comments above

SgtPooki · 2024-08-09T16:42:29Z

I think the various "start" events might be unnecessary? That is, we care about successes vs error/abort, the sum of which should be equal to the starts, so we can derive that metric if we need it but it doesn't seem particularly useful on its own?

The main thing I was thinking here was that with https://github.com/libp2p/js-libp2p-amino-dht-bootstrapper, I ran into some issues where dials were completely failing and being closed early and I couldn't find them in the logs or events. If we don't have an event for first-touch on the receiving end, we don't have an easy way to confirm that the error occurred at the networking/docker/etc layer above js-libp2p.

On the other hand, if we do have a start event and nothing else (totally possible if OS drops that socket for whatever reason), we can investigate there further.. e.g. A lot of starts and no other events for the transport could indicate instability at the host layer.

If we have ZERO starts, but we know we tried to dial it.. we know libp2p never got the connection.

These may not be enough of a reason to emit all these metrics all the time, but that's what was in my mind while creating this PR.

I can remove the various "starts" if you feel like they're still unnecessary. let me know!

I will add a metricsPrefix and other related metrics changes to socketToMaConn to more closely match https://github.com/libp2p/js-libp2p/blob/0edbfe7af1ccf4bd23dd78b2bcc29ecf54ea02eb/packages/transport-tcp/src/socket-to-conn.ts

achingbrain · 2024-08-09T18:02:25Z

The main thing I was thinking here was that with libp2p/js-libp2p-amino-dht-bootstrapper, I ran into some issues where dials were completely failing and being closed early and I couldn't find them in the logs or events. If we don't have an event for first-touch on the receiving end, we don't have an easy way to confirm that the error occurred at the networking/docker/etc layer above js-libp2p.

This sounds a bit weird. If our handler isn't being invoked but the connection is accepted by the server we should see the number of active handles increase. If the connection succeeds we should see the "success" metric increase, if it errors or times out we should see the "error" or "aborted" metrics increase. There shouldn't be any other scenarios?

If connections are not making it to the app it's probably a misconfiguration of ports in the docker file?

I'd say take the "start"s out for now, we can always add them later if they become necessary.

remove: * upgrade_start * socket_open_start * maconn_open_start * maconn_close_start add: * socket_open_error

SgtPooki · 2024-08-13T19:05:46Z

@achingbrain starts removed in 05bb4dd (#2649).

NOTE: added socket_open_error. updating PR description now

added metricPrefix to `socket-to-conn.ts` and set in `listener.ts` remove: * maconn_abort - would have resulted in duplicate of `maconn_close_abort` * maconn_open_success - No logic actually happens during maconn creation.. should be build/compile/run error and not a metric. add: N/A change: * socket_close_success -> maconn_socket_close_success

SgtPooki

self review

packages/transport-websockets/src/index.ts

packages/transport-websockets/src/listener.ts

SgtPooki · 2024-08-13T19:53:51Z

packages/transport-websockets/src/listener.ts


              await maConn.close().catch(err => {
                this.log.error('inbound connection failed to close after upgrade failed', err)
              })
            })
+          metrics?.increment({ socket_open_success: true })


this could fire after the .catch handler. should remove

SgtPooki · 2024-08-13T19:55:26Z

packages/transport-websockets/src/socket-to-conn.ts

@@ -91,6 +100,7 @@ export function socketToMaConn (stream: DuplexWebSocket, remoteAddr: Multiaddr,
    if (maConn.timeline.close == null) {
      maConn.timeline.close = Date.now()
    }
+    metrics?.increment({ [`${metricPrefix}maconn_socket_close_success`]: true })


we may not want this one? but based on the comment on lines 97-99, it seems like it could be useful

SgtPooki · 2024-08-13T19:57:13Z

packages/transport-websockets/src/socket-to-conn.ts

@@ -57,6 +63,7 @@ export function socketToMaConn (stream: DuplexWebSocket, remoteAddr: Multiaddr,
        const { host, port } = maConn.remoteAddr.toOptions()
        log('timeout closing stream to %s:%s after %dms, destroying it manually',
          host, port, Date.now() - start)
+        metrics?.increment({ [`${metricPrefix}maconn_close_abort`]: true })


I don't think the this.abort(err), that calls stream.destroy will trigger this.. i'm trying to avoid duplicate abort and error events but might have missed something

packages/transport-websockets/src/listener.ts

SgtPooki · 2024-08-13T20:00:24Z

packages/transport-websockets/src/index.ts

    const conn = await options.upgrader.upgradeOutbound(maConn, options)
+    this.metrics?.dialerEvents.increment({ upgrade_success: true })


do we need an upgrade_error somewhere?

Sure, makes sense.

SgtPooki · 2024-08-14T19:11:02Z

i'm currently in the process of updating this PR to much more closely resemble TCP metrics.. including adding a status metric

SgtPooki · 2024-08-14T20:02:40Z

FYI, take a look at the callouts in PR description and let me know if you want me to add any event handlers. it looks like this transport hasn't been changed much since the migration to typescript, so maybe it could use an overhaul, but I wanted to keep things limited to only adding metrics

SgtPooki

self review. single callout comment about the WebSocketListenerStatusCode enum

SgtPooki · 2024-08-14T20:04:13Z

packages/transport-websockets/src/listener.ts

+  events: CounterGroup
+}
+
+enum WebSocketListenerStatusCode {


Note: I removed the paused status that the TCP transport had because it didn't seem to make sense here.

Listener status was added to the TCP transport as it can shut itself down when under attack and restart later on. This feature isn't in the WebSocket transport so I think we don't need this right now.

We may add it later I guess but let's do that when we do that.

achingbrain · 2024-08-15T08:18:10Z

Do we want more listener events in the server.ts instead of in listener.ts ?

I think if we follow TCP as a model then no. We can always add more later.

Is the socket_close_success in socket-to-conn.ts called even after an abort event? we may want to move it if so.

We also listen for the close event emitted by the socket and record a metric, so socket_close_success seems redundant.

Is PR comments clarifying metrics and usage enough or is there another place I need to change? main/doc/METRICS.md doesn't list out metrics and explanations, so I figured this PR would be a good placeholder at the least.

Seems fine. METRICS.md purposefully doesn't list out every single metric as it would almost certainly get out of date really quickly. Maybe it could list some core ones to get people started but I shudder when I think of the maintenance.

Do we need to add any tests?

For completeness probably, and they may be useful in answering questions like "Is the socket_close_success in socket-to-conn.ts called even after an abort event?" but I would be fine without them.

I've pushed a couple of commits, I'd like to get this in.

I removed the socket status as per #2649 (comment)

I also remove the close_abort and close_error events in favour of listening for the close event (error is a bit more complicated as the WebSocket implementation from ws doesn't function the same as node Duplex streams).

The idea is, we listen for events on the socket and log metrics when they occur (same as the TCP transport).

This is a bit different to the in initial PR, which logged metrics after calling methods on the socket. That is, the socket is closed when the close event is emitted, not when we call methods that cause it to close.

Without only relying solely on the events for logging metrics we could end up masking problems caused if we misuse the stream API for whatever reason.

feat: add WebSockets metrics

421a696

SgtPooki requested a review from a team as a code owner August 5, 2024 21:05

SgtPooki self-assigned this Aug 5, 2024

achingbrain requested changes Aug 9, 2024

View reviewed changes

Merge branch 'main' into feat/ws-metrics

ca069a9

SgtPooki added 2 commits August 13, 2024 14:04

chore: remove unnecessary 'start' events

05bb4dd

remove: * upgrade_start * socket_open_start * maconn_open_start * maconn_close_start add: * socket_open_error

Merge branch 'main' into feat/ws-metrics

9536f81

SgtPooki commented Aug 13, 2024

View reviewed changes

SgtPooki requested a review from achingbrain August 13, 2024 20:06

Merge branch 'main' into feat/ws-metrics

aa6a9f4

SgtPooki mentioned this pull request Aug 14, 2024

epic: expose transport metrics #2664

Open

1 task

SgtPooki added 3 commits August 14, 2024 14:40

chore: remove unused property

ba44f7a

fix: align more closely to tcp metrics

b69a861

chore: remove sink metrics

5c8871a

SgtPooki commented Aug 14, 2024

View reviewed changes

achingbrain added 2 commits August 15, 2024 08:22

chore: revert status

fe808b0

chore: log on event

ec974b3

chore: do not double-track error

8524f80

achingbrain approved these changes Aug 15, 2024

View reviewed changes

achingbrain merged commit 1dfb74e into main Aug 15, 2024
24 checks passed

achingbrain deleted the feat/ws-metrics branch August 15, 2024 09:07

achingbrain mentioned this pull request Aug 15, 2024

chore: release main #2657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add WebSockets metrics #2649

feat: add WebSockets metrics #2649

SgtPooki commented Aug 5, 2024 •

edited

Loading

SgtPooki commented Aug 6, 2024

achingbrain commented Aug 6, 2024

achingbrain commented Aug 9, 2024

achingbrain left a comment

SgtPooki commented Aug 9, 2024

achingbrain commented Aug 9, 2024 •

edited

Loading

SgtPooki commented Aug 13, 2024

SgtPooki left a comment

SgtPooki Aug 13, 2024

SgtPooki Aug 13, 2024

SgtPooki Aug 13, 2024

SgtPooki Aug 13, 2024

achingbrain Aug 14, 2024

SgtPooki commented Aug 14, 2024

SgtPooki commented Aug 14, 2024

SgtPooki left a comment

SgtPooki Aug 14, 2024

achingbrain Aug 15, 2024

achingbrain commented Aug 15, 2024

		const conn = await options.upgrader.upgradeOutbound(maConn, options)
		this.metrics?.dialerEvents.increment({ upgrade_success: true })

feat: add WebSockets metrics #2649

feat: add WebSockets metrics #2649

Conversation

SgtPooki commented Aug 5, 2024 • edited Loading

Title

Description

Notes & open questions

Callouts

TODO based on comments:

Some questions:

Change checklist

SgtPooki commented Aug 6, 2024

achingbrain commented Aug 6, 2024

achingbrain commented Aug 9, 2024

achingbrain left a comment

Choose a reason for hiding this comment

SgtPooki commented Aug 9, 2024

achingbrain commented Aug 9, 2024 • edited Loading

SgtPooki commented Aug 13, 2024

SgtPooki left a comment

Choose a reason for hiding this comment

SgtPooki Aug 13, 2024

Choose a reason for hiding this comment

SgtPooki Aug 13, 2024

Choose a reason for hiding this comment

SgtPooki Aug 13, 2024

Choose a reason for hiding this comment

SgtPooki Aug 13, 2024

Choose a reason for hiding this comment

achingbrain Aug 14, 2024

Choose a reason for hiding this comment

SgtPooki commented Aug 14, 2024

SgtPooki commented Aug 14, 2024

SgtPooki left a comment

Choose a reason for hiding this comment

SgtPooki Aug 14, 2024

Choose a reason for hiding this comment

achingbrain Aug 15, 2024

Choose a reason for hiding this comment

achingbrain commented Aug 15, 2024

SgtPooki commented Aug 5, 2024 •

edited

Loading

achingbrain commented Aug 9, 2024 •

edited

Loading