worker: add support for worker name in inspector and trace_events by debadree25 · Pull Request #46832 · nodejs/node · GitHub

This is an attempt at adding a title prefix for the names of workers, while the change works well on VSCode I have no idea how to write a unit test for the same, so any help on that aspect would be helpful, also my cpp knowledge is shaky at best have tried my best to trace the code and add the variable here :-)

EDIT: Added tests and doc

Fixes: #41589

cc @nodejs/workers

It looks something like this on VSCode

Labeled this as semver-major because adding a parameter here is breaking the ABI (and, since there is no default value for the parameter, also the API); you can avoid that by adding an overload with this signature instead of changing the signature.

(For example: c566a04 introduced a new parameter to a C++ API function in an ABI-compatible way, e8bddac followed up as a semver-major change to simplify the implementation again)

Ah understood! I think overloading would be better, updating!

Updated, could you check

how to write a unit test

Maybe you can create a worker with a prefix and then assert on the title property on the workerInfo object like in here

node/test/parallel/test-inspector-workers-flat-list.js

Line 25 in 80a7843

console.log(`Worker ${workerInfo.title} was reported`);

?

Perfect just what I was looking for thank you so much @RaisinTen ❤️!!

Added tests and doc, opening it for review!

Purely cosmetic observation: options.titlePrefix feels kinda clunky and foo Worker 42 looks kinda ugly.

Suggestion: rename it to options.title or options.name (I like that last one better), change the default to [worker 42] and append the name so it turns into [worker 42] foo.

Besides aesthetics, it has the benefit that you can still sort workers by id.

This makes sense implementing these changes @bnoordhuis, also I think we can make the trace name same as this #41589 (comment) for consistency

Will changing the name formation of workers be a semver-major change?

Hello ThreadWorker just feels a bit odd (specifically, not having a space between the prefix and Worker). I would almost prefer a model like Worker {id} [label] ... e.g. Worker 1 [Hello Thread]. Consider this non-blocking tho.

Yes i am updating the name formation as per this #46832 (comment)

Fixes: nodejs#41589

Commit Queue failed

- Loading data for nodejs/node/pull/46832
✔  Done loading data for nodejs/node/pull/46832
----------------------------------- PR info ------------------------------------
Title      worker: add support for worker name in inspector and trace_events (#46832)
Author     Debadree Chatterjee  (@debadree25)
Branch     debadree25:ft/worker-prefix -> nodejs:main
Labels     c++, semver-minor, lib / src, author ready, worker, needs-ci, commit-queue-squash
Commits    20
 - worker: add support for worker name in inspector and trace_events
 - fixup! remove breaking change
 - fixup! add a test
 - fixup! add doc
 - fixup! lint
 - fixup! lint
 - fixup! null check
 - fixup! simplify function
 - fixup! update name formation
 - fixup! refactor option name
 - fixup! update worker trace name
 - fixup! lint
 - fixup! lint
 - fixup! lint again
 - fixup! surround with mustcall
 - fixup!
 - fixup! move the session to worker thread
 - fixup! keep worker alive
 - fixup! move worker code to a fixture
 - fixup! typo
Committers 1
 - Debadree Chatterjee 
PR-URL: https://github.com/nodejs/node/pull/46832
Fixes: https://github.com/nodejs/node/issues/41589
Reviewed-By: Ben Noordhuis 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Anna Henningsen 
Reviewed-By: Darshan Sen 
Reviewed-By: Gireesh Punathil 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46832
Fixes: https://github.com/nodejs/node/issues/41589
Reviewed-By: Ben Noordhuis 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Anna Henningsen 
Reviewed-By: Darshan Sen 
Reviewed-By: Gireesh Punathil 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 25 Feb 2023 13:06:32 GMT
   ✔  Approvals: 5
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/46832#pullrequestreview-1317249969
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46832#pullrequestreview-1318020184
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/46832#pullrequestreview-1318030016
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/46832#pullrequestreview-1325986351
   ✔  - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/46832#pullrequestreview-1324899481
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-03-02T18:36:53Z: https://ci.nodejs.org/job/node-test-pull-request/50160/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - fixup! typo
- Querying data for job/node-test-pull-request/50160/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4342938214

CI: https://ci.nodejs.org/job/node-test-pull-request/50226/

Landed in 2d9bf91

Fixes: #41589 PR-URL: #46832 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087

Fixes: #41589 PR-URL: #46832 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: Add initial support for single executable applications Compile a JavaScript file into a single executable application: ```console $ echo 'console.log(`Hello, ${process.argv[2]}!`);' > hello.js $ cp $(command -v node) hello $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 \ --macho-segment-name NODE_JS $ ./hello world Hello, world! ``` Contributed by Darshan Sen in #45038 Replace url parser with Ada Node.js gets a new URL parser called Ada that is compliant with the WHATWG URL Specification and provides more than 100% performance improvement to the existing implementation. Contributed by Yagiz Nizipli in #46410 Other notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 25, 2023

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 25, 2023

addaleax reviewed Feb 25, 2023

View reviewed changes

debadree25 marked this pull request as ready for review February 26, 2023 08:12

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 26, 2023

addaleax reviewed Feb 26, 2023

View reviewed changes

Comment thread src/api/environment.cc Outdated

Comment thread src/api/environment.cc Outdated

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Feb 27, 2023

CI Reliability 2023-02-27 nodejs/reliability#508

Open

13 tasks

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Feb 28, 2023

CI Reliability 2023-02-28 nodejs/reliability#509

Open

16 tasks

jasnell reviewed Feb 28, 2023

View reviewed changes

debadree25 changed the title worker: add support for worker title prefix worker: add support for worker name in inspector and trace_events Feb 28, 2023

debadree25 added 4 commits February 28, 2023 13:42

worker: add support for worker name in inspector and trace_events

c977783

Fixes: nodejs#41589

fixup! remove breaking change

35811f5

fixup! add a test

2680a3e

fixup! add doc

b6738e1

debadree25 added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 6, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 6, 2023

This comment was marked as outdated.

Sign in to view

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 6, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 6, 2023

nodejs-github-bot merged commit 2d9bf91 into nodejs:main Mar 6, 2023

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

worker: add support for worker name in inspector and trace_events by debadree25 · Pull Request #46832 · nodejs/node · GitHub

Uh oh!

Uh oh!

worker: add support for worker name in inspector and trace_events - #46832

worker: add support for worker name in inspector and trace_events#46832nodejs-github-bot merged 20 commits intonodejs:mainnodejs/node:mainfrom debadree25:ft/worker-prefixdebadree25/node:ft/worker-prefixCopy head branch name to clipboard

Conversation

debadree25 commented Feb 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25 commented Feb 25, 2023

Uh oh!

debadree25 commented Feb 25, 2023

Uh oh!

addaleax Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented Feb 26, 2023

Uh oh!

debadree25 commented Feb 26, 2023

Uh oh!

debadree25 commented Feb 26, 2023

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

bnoordhuis commented Feb 27, 2023

Uh oh!

debadree25 commented Feb 27, 2023

Uh oh!

debadree25 commented Feb 27, 2023

Uh oh!

jasnell Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 6, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Mar 6, 2023

Uh oh!

nodejs-github-bot commented Mar 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

worker: add support for worker name in inspector and trace_events#46832
nodejs-github-bot merged 20 commits into
nodejs:mainfrom
debadree25:ft/worker-prefix

debadree25 commented Feb 25, 2023 •
edited
Loading