-
-
Notifications
You must be signed in to change notification settings - Fork 34k
src: add locksCounters() to monitor Web Locks usage #60502
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
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60502 +/- ##
==========================================
+ Coverage 88.05% 88.57% +0.51%
==========================================
Files 704 704
Lines 207857 207941 +84
Branches 39964 40054 +90
==========================================
+ Hits 183030 184182 +1152
+ Misses 16806 15803 -1003
+ Partials 8021 7956 -65
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we generally avoid adding more stuff to the process namespace. Probably better as another export in node:worker_threads given that's where the locks themselves are. 🤔
|
@Qard I put it on process because Locks are shared across the entire process and It follows the same pattern as |
|
I think if we want to add a usage counter to process, it should ideally be generic, e.g. something like Also, it would be nicer if the usage counter can be cleared/started/stopped at some given time instead of being recorded throughout the lifetime of the application. |
|
@joyeecheung I looked at the debug counters pattern, but I think it's better suited for internal testing rather than public API. I'm more with moving this to
IMO I don't think they're a good fit for lock counters. These are lifetime counters, and adding Also, |
| Use [`process.locksCounters()`][] to monitor lock acquisitions, contention, and | ||
| queue sizes across the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be either all singular or all plural no ?
| Use [`process.locksCounters()`][] to monitor lock acquisitions, contention, and | |
| queue sizes across the process. | |
| Use [`process.locksCounters()`][] to monitor lock acquisitions, contentions, and | |
| queue sizes across the process. |
| threadCpuUsage: _threadCpuUsage, | ||
| memoryUsage: _memoryUsage, | ||
| rss, | ||
| locksCounters: _locksCounters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have cpuUsage, threadCpuUsage, memoryUsage, resourceUsage... maybe we should call it lockUsage instead ?
|
@Qard @joyeecheung I would like to continue on this. Any thought how we can make a decision? |
I meant that it would be nice to have individual counters that can be independently started/paused/stopped, so that e.g. you can use it in tests, as opposed to only having one global counter. For example, if one library A does diagnostics and another library B uses locks in some way, A can start its own counter to monitor code that depends on B, B won't be able to clear that counter unless it gets the handle of it somehow. |
Description
This PR introduces
process.locksCounters()to provide real time visibility into Web Locks API usage and contention metrics across the Node.js process.Motivation
Lock contention can be difficult to diagnose in concurrent applications. Other platforms already make this easier,.NET has lock performance counters, Java has JMX for checking thread locks, and Go has mutex profiling.
Node.js added the Web Locks API in v24, but there’s currently no built-in way to see how locks are behaving. The new
process.locksCounters()provide metrics to help developers monitor and debug lock related performance problems.