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: support aggregating metrics across workers in a Node.js worker_… #403

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rafixer
Copy link

@rafixer rafixer commented Oct 15, 2020

…threads

this PR is connected with this issue: #401 (comment)

@zbjornson zbjornson linked an issue Jan 23, 2021 that may be closed by this pull request
Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Thanks for opening this and sorry it took forever to get reviewed. A few comments below, plus:

  • Looks like Node.js 10 breaks - drop support
  • Needs a changelog update
  • Please add tests

lib/cluster.js Outdated Show resolved Hide resolved
lib/cluster.js Outdated Show resolved Hide resolved
@bgalek
Copy link

bgalek commented May 5, 2021

@zbjornson node 10 met its EOL https://nodejs.org/en/about/releases/ maybe we should not test against it anymore? worker features are available from node 11.0.0

@SimenB
Copy link
Collaborator

SimenB commented May 12, 2021

Dropping node 10 must be done in a major release, but we can probably do one of those?

@alec-benson
Copy link

any chance we could push this along? I am using worker threads on a project and this would help a lot.

@bgalek
Copy link

bgalek commented Jul 23, 2021

@rafixer @zbjornson? :)

@zbjornson
Copy link
Collaborator

This looks good.

We have two other PRs staged for a semver-major release. Let's drop v10 support in that release and land this PR with it.

(I can add the changelog and fix the merge conflict if @rafixer doesn't get to it.)

@bgalek
Copy link

bgalek commented Feb 18, 2022

@zbjornson any chances for new major? :) deprecation fix for node 16+ is also "ready to release" - it would be great if new release comes some time soon ;)

@bgalek
Copy link

bgalek commented Feb 28, 2022

@zbjornson? :)

@Zagorodnyi
Copy link

Zagorodnyi commented Jun 2, 2022

@zbjornson Any news about this feature?

@rafixer
Copy link
Author

rafixer commented Jun 2, 2022

Sorry for no action about this PR. I will back to this on next week!

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

sorry about the slow response! can you update the documentation?

lib/workerThreads.js Outdated Show resolved Hide resolved
lib/workerThreads.js Outdated Show resolved Hide resolved
@SimenB SimenB changed the base branch from master to next January 17, 2023 13:58
@rafixer
Copy link
Author

rafixer commented Jan 17, 2023

sorry about the slow response! can you update the documentation?

also, I will check why currently tests cases are failing

@SimenB
Copy link
Collaborator

SimenB commented Jan 17, 2023

there's a syntax error. Miiight be the merge? ee05476 (#403). I didn't get any merge conflicts, tho

@SimenB
Copy link
Collaborator

SimenB commented Jan 17, 2023

Fixing the syntax error gives a bunch of lint errors, so I dunno 😅

@rafixer
Copy link
Author

rafixer commented Jan 17, 2023

as I remember, I tried to refactor the implementation - I will bring back the previous one

@rafixer
Copy link
Author

rafixer commented Jan 17, 2023

but still, I'm worried that implementation might not be efficient - because it depends on communication between parent < - > child, especially when using a nested tree of workers/processes. Maybe the better idea will be to create a separate process and try to communicate with him via IPC

@SimenB
Copy link
Collaborator

SimenB commented Jan 17, 2023

thread communication shouldn't be any slower than separate process, should it?

@SimenB SimenB force-pushed the next branch 6 times, most recently from 3f25c32 to 47441a0 Compare March 9, 2023 11:52
@SimenB
Copy link
Collaborator

SimenB commented Mar 13, 2023

@rafixer could you target master with this (and resolve conflicts)?

@rafixer rafixer changed the base branch from next to master March 13, 2023 14:45
@rafixer
Copy link
Author

rafixer commented Mar 13, 2023

@rafixer could you target master with this (and resolve conflicts)?

Target changed , resolving conflicts in progress!

@infloop
Copy link

infloop commented Jun 26, 2023

@SimenB @zbjornson Are there any updates yet? Does this need any help?

@iamyamakin
Copy link

@SimenB @zbjornson are you waiting for tests and changes for readme.md? How can we help you?

@matthieu-beteille
Copy link

@SimenB @zbjornson Any update on this? Anything we can do to push this through?

@nrathi
Copy link

nrathi commented Jul 25, 2024

+1, this would really help me as well!

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

is it possible to write tests for this? Probably by spinning up some process which emulates this and making requests against it?

I can't speak to the feature itself as I've never used the cluster mode

@@ -170,6 +182,7 @@ function addListeners() {
cluster().on('message', (worker, message) => {
if (message.type === GET_METRICS_RES) {
const request = requests.get(message.requestId);
request.pending += message.workerRequests || 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
request.pending += message.workerRequests || 0;
if (message.workerRequests) {
request.pending += message.workerRequests;
}

@@ -196,12 +209,38 @@ function addListeners() {
// Respond to master's requests for worker's local metrics.
process.on('message', message => {
if (message.type === GET_METRICS_REQ) {
let workerRequests = 0;
workersQueue.forEach(worker => {
if (worker && worker instanceof Worker) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (worker && worker instanceof Worker) {
if (worker instanceof Worker) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we check this when instantiating/pushing into workersQueue rather than when receiving messages? We should probably throw

@@ -196,12 +209,38 @@ function addListeners() {
// Respond to master's requests for worker's local metrics.
process.on('message', message => {
if (message.type === GET_METRICS_REQ) {
let workerRequests = 0;
workersQueue.forEach(worker => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use for-of instead of forEach

parentPort.on('message', ({ type, requestId, port } = {}) => {
if (type === GET_METRICS_REQ) {
Promise.all(registries.map(r => r.getMetricsAsJSON()))
.then(metrics => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use async-await rather than .then and .catch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for worker_threads module
10 participants