feat: Add metrics for common events #69
Conversation
|
NOTE: I've taken the exact pattern mentioned in the TCP listener. We may want to expand certain metrics in future for WebRTC as there are some complex error conditions not captured in this initial draft. |
|
In the TCP version, metrics are added to the multiaddress connection. Do we want that here as well? |
|
@ddimaria good question about the multiaddr ... I read through that file in the webrtc implementation and it wasn't entirely clear if we need metrics there except in the case of close https://github.com/libp2p/js-libp2p-webrtc/blob/main/src/maconn.ts (see close) wdyt? |
|
We may want to include the |
|
There are events related to the |
|
@ddimaria I like that suggestion... The muxer is more related to the number of inbound/outbound connections which I like. I'll add that next. |
lightsofapollo
force-pushed
the
feat/64/export-metrics
branch
from
bd205b8
to
f85c6d9
Compare
lightsofapollo
force-pushed
the
feat/64/export-metrics
branch
from
f85c6d9
to
8f4cec9
Compare
lightsofapollo
changed the title
| @@ -11,6 +11,7 @@ describe('Multiaddr Connection', () => { | |||
| const remoteAddr = multiaddr('/ip4/1.2.3.4/udp/1234/webrtc/certhash/uEiAUqV7kzvM1wI5DYDc1RbcekYVmXli_Qprlw3IkiEg6tQ') | |||
| const maConn = new WebRTCMultiaddrConnection({ | |||
| peerConnection: peerConnection, | |||
| metrics: null, | |||
There was a problem hiding this comment.
Would be nice if we explicitly tested metrics inteface?
There was a problem hiding this comment.
The sinon-ts module can be used to do this in a straightforward way.
| /** | ||
| * Optional metrics counter group for this connection | ||
| */ | ||
| metrics: CounterGroup | null |
There was a problem hiding this comment.
I don't think we should accept null as a value, better to mark this optional
| metrics: CounterGroup | null | |
| metrics?: CounterGroup |
| @@ -11,6 +11,7 @@ describe('Multiaddr Connection', () => { | |||
| const remoteAddr = multiaddr('/ip4/1.2.3.4/udp/1234/webrtc/certhash/uEiAUqV7kzvM1wI5DYDc1RbcekYVmXli_Qprlw3IkiEg6tQ') | |||
| const maConn = new WebRTCMultiaddrConnection({ | |||
| peerConnection: peerConnection, | |||
| metrics: null, | |||
There was a problem hiding this comment.
The sinon-ts module can be used to do this in a straightforward way.
|
@achingbrain ❤️ Thanks! I'll have the next iteration up shortly which will be ready to review :) |
Adds metrics for common error cases (timeoout, dtls handshake failure) and few success cases (stream open / stream close)
Open questions:
Closes: #64