Skip to content

Add stats of client and pool to be accessible through agent #4157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
metcoder95 merged 17 commits into from
May 9, 2025
Merged

Add stats of client and pool to be accessible through agent #4157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2661544
Add shared stats symbol
tdeekens Apr 12, 2025
28ec198
Add client stats class
tdeekens Apr 12, 2025
df86d9b
Use client stats in client
tdeekens Apr 12, 2025
4370e3d
Adjust pool and expose stats from agent
tdeekens Apr 12, 2025
2947cba
Add types for changes
tdeekens Apr 12, 2025
39c7617
Add test for client
tdeekens Apr 12, 2025
651eb86
Fix formatting changes
tdeekens Apr 12, 2025
0c22ba5
Add types for agent stats getter
tdeekens Apr 12, 2025
8855ecb
fixup! Fix formatting changes
tdeekens Apr 12, 2025
ab942e4
Move pool and client stats into util/stats
tdeekens Apr 13, 2025
f2452c4
Add tests and fix implementation
tdeekens Apr 13, 2025
15bc63b
Adds documentation and fixes
tdeekens Apr 15, 2025
5a93529
Fix to remove mise
tdeekens Apr 22, 2025
435acc8
Restore docs list star
tdeekens Apr 22, 2025
373736e
Fix boolean type
tdeekens Apr 22, 2025
52e7d94
Refactor to return object over list of list
tdeekens Apr 26, 2025
7ac13ff
Refactor to not use getters but object
tdeekens Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Refactor to not use getters but object
  • Loading branch information
@tdeekens
tdeekens committed Apr 29, 2025
commit 7ac13ffd0274bc28ab5967f82db43ca8500b6023
3 changes: 1 addition & 2 deletions lib/core/symbols.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,5 @@ module.exports = {
kMaxConcurrentStreams: Symbol('max concurrent streams'),
kNoProxyAgent: Symbol('no proxy agent'),
kHttpProxyAgent: Symbol('http proxy agent'),
kHttpsProxyAgent: Symbol('https proxy agent'),
kStats: Symbol('stats')
Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore. Not assigned as property any longer.

kHttpsProxyAgent: Symbol('https proxy agent')
}
7 changes: 2 additions & 5 deletions lib/dispatcher/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ const {
kOnError,
kHTTPContext,
kMaxConcurrentStreams,
kResume,
kStats
kResume
} = require('../core/symbols.js')
const connectH1 = require('./client-h1.js')
const connectH2 = require('./client-h2.js')
Expand Down Expand Up @@ -251,8 +250,6 @@ class Client extends DispatcherBase {

this[kResume] = (sync) => resume(this, sync)
this[kOnError] = (err) => onError(this, err)

this[kStats] = new ClientStats(this)
}

get pipelining () {
Expand All @@ -265,7 +262,7 @@ class Client extends DispatcherBase {
}

get stats () {
return this[kStats]
return new ClientStats(this)
Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New class every time accessed.

}

get [kPending] () {
Expand Down
6 changes: 2 additions & 4 deletions lib/dispatcher/pool-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const { PoolStats } = require('../util/stats.js')
const DispatcherBase = require('./dispatcher-base')
const FixedQueue = require('./fixed-queue')
const { kConnected, kSize, kRunning, kPending, kQueued, kBusy, kFree, kUrl, kClose, kDestroy, kDispatch, kStats } = require('../core/symbols')
const { kConnected, kSize, kRunning, kPending, kQueued, kBusy, kFree, kUrl, kClose, kDestroy, kDispatch } = require('../core/symbols')

const kClients = Symbol('clients')
const kNeedDrain = Symbol('needDrain')
Expand Down Expand Up @@ -66,8 +66,6 @@ class PoolBase extends DispatcherBase {
this[kOnConnectionError] = (origin, targets, err) => {
pool.emit('connectionError', origin, [pool, ...targets], err)
}

this[kStats] = new PoolStats(this)
}

get [kBusy] () {
Expand Down Expand Up @@ -107,7 +105,7 @@ class PoolBase extends DispatcherBase {
}

get stats () {
return this[kStats]
return new PoolStats(this)
}

async [kClose] () {
Expand Down
55 changes: 10 additions & 45 deletions lib/util/stats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,58 +9,23 @@ const {
kQueued
} = require('../core/symbols')

const kPool = Symbol('pool')
const kClient = Symbol('client')

class ClientStats {
constructor (client) {
this[kClient] = client
}

get connected () {
Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not getters just instance properties.

return this[kClient][kConnected]
}

get pending () {
return this[kClient][kPending]
}

get running () {
return this[kClient][kRunning]
}

get size () {
return this[kClient][kSize]
this.connected = client[kConnected]
this.pending = client[kPending]
this.running = client[kRunning]
this.size = client[kSize]
}
}

class PoolStats {
constructor (pool) {
this[kPool] = pool
}

get connected () {
return this[kPool][kConnected]
}

get free () {
return this[kPool][kFree]
}

get pending () {
return this[kPool][kPending]
}

get queued () {
return this[kPool][kQueued]
}

get running () {
return this[kPool][kRunning]
}

get size () {
return this[kPool][kSize]
this.connected = pool[kConnected]
this.free = pool[kFree]
this.pending = pool[kPending]
this.queued = pool[kQueued]
this.running = pool[kRunning]
this.size = pool[kSize]
}
}
Copy link
Copy Markdown
Member

@mcollina mcollina Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this design. Instead of using getters, I would allocate new objects. This ensures that data is stable in time without having to copy them.

Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair I also didn't but this it the current implementation and I am following it assuming it has reasoning. In that this code is just moved from elsewhere as requested by @metcoder95.

Do you wish to have this changed here or as a follow up?

Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏼 What's the implicit agreement here? Refactor as a follow-up, not at all or here? I understand the issue that the using getters can cause inconsistent as a whole with single values. I assume we would return an object of a "stats bag" in the pool and agent. Just not sure when/where to implement it.

Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metcoder95 what's missing on this PR to land this? Anything expected from my side?

Copy link
Copy Markdown
Member

@metcoder95 metcoder95 Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just applying the suggested refactor from getters to a plain object (it can also come from a class, but avoiding the getters/setters).

Copy link
Copy Markdown
Contributor Author

@tdeekens tdeekens Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. From what I understand 7ac13ff is what you had in mind?


Expand Down
Loading