Skip to content
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.

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

feat: expose pool aggregate statistics #1255

Merged
merged 1 commit into from Feb 28, 2022
Merged

Conversation

mnutt
Copy link
Contributor

@mnutt mnutt commented Feb 26, 2022

This relates to...

In achieving parity with the node.js agent, I'd like to be able to get aggregate statistics about requests and connections in undici. This is possible today, but requires using private symbols.

Addresses #1254, relates to #693 -- I just now noticed that one, so will review it to see if there's any feedback that can be applied here.

Rationale

I currently use the node.js agent and send its stats to prometheus; I'd like to send the same ones for undici to be able to instrument the switchover and ensure everything is working properly.

Changes

  • Expose a stats getter on pools.

Features

  • Access aggregate statistics about undici Pools via a stats object.

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

KEY: S = Skipped, x = complete

  • I have read and agreed to the Developer's Certificate of Origin
  • Tested
  • [S] Benchmarked (optional)
    • I didn't benchmark it because the new methods aren't in the normal request path and are only intended to be called periodically; eyeballing it I believe it should scale roughly O(n) with number of active requests.
  • Documented
  • [] Review ready
  • In review
  • Merge ready

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2022

Codecov Report

Merging #1255 (48d99d8) into main (662fd82) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
+ Coverage   94.08%   94.10%   +0.02%     
==========================================
  Files          43       44       +1     
  Lines        4071     4086      +15     
==========================================
+ Hits         3830     3845      +15     
  Misses        241      241              
Impacted Files Coverage Δ
lib/core/symbols.js 100.00% <ø> (ø)
lib/pool-base.js 97.74% <100.00%> (+0.08%) ⬆️
lib/pool-stats.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 662fd82...48d99d8. Read the comment docs.

@ronag
Copy link
Member

ronag commented Feb 26, 2022

Could we provide more info through diagnostic channels so that these stats can be built in user space?

@mnutt
Copy link
Contributor Author

mnutt commented Feb 26, 2022

That's an interesting idea, I guess in metrics terms these are more like gauges, while diagnostics channels are counters. I think it's possible that one could use diagnostic channels counters to emulate some of these by doing some bookkeeping, though I think it might require quite a few more diagnostic channels since some of these are overlapping?

@ronag
Copy link
Member

ronag commented Feb 26, 2022

few more diagnostic channels since some of these are overlapping?

I'm happy to add more channels!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I still think for best performance and reactive API adding more diagnostic channels and implementing this on top of that is a better idea.

@ronag ronag merged commit 8a17699 into nodejs:main Feb 28, 2022
@ronag
Copy link
Member

ronag commented Feb 28, 2022

@mnutt Do you think you could add something similar to Client and maybe Agent?

@mnutt mnutt deleted the expose-statistics branch March 1, 2022 19:21
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Mar 1, 2022
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants