Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Todai88
force-pushed
the
feat/add-client-lifecycle-metrics
branch
4 times, most recently
from
76b75d9 to
649200c
Compare
| openHandler(websocket, localClientOps, identifyingMetadata, metricsClient), | ||
| ); | ||
| websocket.on('open', () => { | ||
| openHandler(websocket, localClientOps, identifyingMetadata, metricsClient); |
There was a problem hiding this comment.
Is it intentional that the open event is not considered a lifecycle event?
There was a problem hiding this comment.
We should standardise on lifecycleEvent, but the openHandler already emits other connection-related metrics.
|
|
||
| websocket.on('close', () => { | ||
| closeHandler(websocket, localClientOps, identifyingMetadata, metricsClient); | ||
| reportWebSocketClosureEvent(websocket, localClientOps, identifyingMetadata, metricsClient, 'connection_lost'); |
There was a problem hiding this comment.
How did we get to these human readable names? Are we introducing a layer of confusion by not sticking with the events the websocket client uses? They already seem pretty human readable to me...
There was a problem hiding this comment.
Unfortunately, some of the events can be slightly misleading.
Like closing -- one could be excused if one believed it was emitted as a "client is intending to close", but it's actually an event emitted by the server ahead of actually closing the connection.
The purpose of this change is to improve our overall telemetry surface. The new metric has a human-readable event as a label, rather than the "raw" websocket event. This hopefully makes it clearer for our downstream users what's specifically going on. Compare "closing" to "server_requested_close". Also: - renames the closeHandler to reportWebSocketClosureEvent - this is much closer to what we actually are doing. The code isn't handling any events. - implements the reportWebSocketClosureEvent function to emit new telemetry - adds tests to ensure existing and current functionality continues working
Todai88
force-pushed
the
feat/add-client-lifecycle-metrics
branch
from
649200c to
2e285df
Compare
2 participants
The purpose of this change is to improve our overall telemetry surface.
The new metric has a human-readable event as a label, rather than the "raw" websocket event. This hopefully makes it clearer for our downstream users what's specifically going on.
Compare "closing" to "server_requested_close".
Also: