WIP: Improve streaming metrics #24886
Conversation
* Adds labels to the 'connected_clients' metric to allow analysis by type of http vs websocket connection. * Adds node.js performance metrics, * Corrects measurement error in 'connected_channels' metric to only report number of subscriptions to redis, not the number of handlers subscribed * Adds 'streaming_latency' metric which indicates the lag between when an event was published to redis and the streaming server consumed it
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| res.set('Content-Type', metrics.register.contentType); | ||
| res.end(await metrics.register.metrics()); | ||
| } catch (ex) { | ||
| res.status(500).end(ex); |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace Medium
There was a problem hiding this comment.
We should probably fix this, but the /metrics endpoint should never be exposed to the public internet anyway.
streaming/index.js
Outdated
| @@ -621,6 +692,8 @@ const startServer = async () => { | |||
| const delta = now - queued_at; | |||
| const encodedPayload = typeof payload === 'object' ? JSON.stringify(payload) : payload; | |||
|
|
|||
| streamingLatency.observe(delta); | |||
There was a problem hiding this comment.
Apparently this is often undefined
There was a problem hiding this comment.
Looks like a lot of the events don't actually carry the queued_at property, and as such lag cannot be calculated.
| // FIXME: most stream events don't actually carry a queued_at timestamp | ||
| // const now = new Date().getTime(); | ||
| // const delta = now - queued_at; | ||
| // streamingLatency.observe(delta); |
There was a problem hiding this comment.
This code didn't actually work as expected, as it was usually doing delta = now - undefined because most events didn't carry the queued_at timestamp.
|
This pull request has merge conflicts that must be resolved before it can be merged. |
ThisIsMissEm
changed the title
| }); | ||
|
|
||
| const streamingLatency = new metrics.Histogram({ | ||
| name: 'streaming_latency', |
There was a problem hiding this comment.
This is disabled at present, as not all redis.publish payloads carry the queued_at value.
@renchap @Gargron to solve this, we'd be best creating a Streaming class/module which takes care of serialising all the events, instead of having that code and the publish calls distributed across the codebase.
|
This pull request has been superseded by #26299 |
This branches off from #24702 (as the package.json has been split); Only the last two commits are new.
This pull request: