feat: Add stats of client and pool to be expored through agent #4157
Conversation
| @@ -64,5 +64,6 @@ module.exports = { | |||
| kMaxConcurrentStreams: Symbol('max concurrent streams'), | |||
| kNoProxyAgent: Symbol('no proxy agent'), | |||
| kHttpProxyAgent: Symbol('http proxy agent'), | |||
| kHttpsProxyAgent: Symbol('https proxy agent') | |||
| kHttpsProxyAgent: Symbol('https proxy agent'), | |||
| kStats: Symbol('stats') | |||
There was a problem hiding this comment.
This is used now by Pool and Client. Hence it should be shared in my opinion.
| @@ -110,6 +110,17 @@ class Agent extends DispatcherBase { | |||
|
|
|||
| await Promise.all(destroyPromises) | |||
| } | |||
|
|
|||
| get stats () { | |||
There was a problem hiding this comment.
Getter to loop over Clients which can be Pool or Client themselves to assemble and expose stats.
Data structure can be changed. Right now it's [string, [PoolStats, ClientStats]]. It could also be Record<string, PoolStats | ClientStats>.
There was a problem hiding this comment.
The current shape seems like a good first iteration, especially for Agent as the number of origins called can vary; caller can reconstruct it into a different data structure if needed
There was a problem hiding this comment.
Thanks. Same thinking on my end.
lib/dispatcher/client-stats.js
Outdated
| @@ -0,0 +1,28 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
Follows implementation of PoolStats. Clients however expose a subset of stats. Hence the separate class.
There was a problem hiding this comment.
I'll put this class in utils or so, as the files within the dispatcher/ dir are mostly mean for classes that shares dispatcher interface.
There was a problem hiding this comment.
Sounds good. Same for the PoolStats then which prior to this change lived here too, right?
| @@ -249,6 +251,8 @@ class Client extends DispatcherBase { | |||
|
|
|||
| this[kResume] = (sync) => resume(this, sync) | |||
| this[kOnError] = (err) => onError(this, err) | |||
|
|
|||
| this[kStats] = new ClientStats(this) | |||
metcoder95
changed the title
| @@ -110,6 +110,17 @@ class Agent extends DispatcherBase { | |||
|
|
|||
| await Promise.all(destroyPromises) | |||
| } | |||
|
|
|||
| get stats () { | |||
There was a problem hiding this comment.
The current shape seems like a good first iteration, especially for Agent as the number of origins called can vary; caller can reconstruct it into a different data structure if needed
lib/dispatcher/client-stats.js
Outdated
| @@ -0,0 +1,28 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
I'll put this class in utils or so, as the files within the dispatcher/ dir are mostly mean for classes that shares dispatcher interface.
Yip. I followed the Pool Stats addition PR in terms of depth of testing on only existing through tsd. |
|
Hi @metcoder95! In ab942e4 I've moved the pool stats both of pool and client into a Should we also add docs and maybe an example? |
This relates to:
This relates to the issue #4149
Rationale
Integrating existing and exposed Pool stats into a telemetry system is a bit cumbersome. Right now we can hook into the
factoryfunction and observe the created pool there. It would be beneficial if a Pool's and Client's stats would be exposed by the Agent owning the Pool or Client respectively.Changes
Exposing stats through the Agent makes it easier to integrate telemetry systems as we do not have to "hook" into the
factoryto receive the Pool or Client used by an Agent.Features
Exposes a new getter called
statson an Agent. The method returns an Array of Arrays with stats by origin.Additionally, the Client now also exposes available stats similar to a Pool. The initial implementation of Pool stats happened in this PR while the implementation for a Client here follows the Pool's implementation pattenr.
Bug Fixes
Breaking Changes and Deprecations
Status