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

debadree25 · 2023-02-25T13:06:32Z

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

debadree25 · 2023-02-25T13:09:25Z

cc @nodejs/workers

debadree25 · 2023-02-25T15:03:10Z

It looks something like this on VSCode

addaleax · 2023-02-25T21:09:51Z

src/node.h

 ThreadId child_thread_id,
-constchar* child_url);
+constchar* child_url,
+constchar* title_prefix);


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

RaisinTen · 2023-02-26T06:52:01Z

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`);

?

debadree25 · 2023-02-26T07:44:08Z

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

debadree25 · 2023-02-26T08:12:33Z

Added tests and doc, opening it for review!

src/api/environment.cc

bnoordhuis · 2023-02-27T11:15:41Z

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.

debadree25 · 2023-02-27T11:48:19Z

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

debadree25 · 2023-02-27T13:19:00Z

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

jasnell · 2023-02-28T01:51:50Z

test/parallel/test-worker-title-prefix.js

+session.connect();
+session.on('NodeWorker.attachedToWorker',({params: { workerInfo }})=>{
+constid=workerInfo.id;
+constexpectedTitle=`${titlePrefix}Worker ${id}`;


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

nodejs-github-bot · 2023-03-06T11:33:47Z

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

nodejs-github-bot · 2023-03-06T13:00:37Z

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

nodejs-github-bot · 2023-03-06T16:34:12Z

Landed in 2d9bf91

Fixes: #41589 PR-URL: #46832 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

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: #47086

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

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: TBD

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

src/api/environment.ccShow resolvedHide resolved
src/api/environment.ccShow resolvedHide resolved

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

github-actionsbot 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-actionsbot 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-actionsbot 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_eventsFeb 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-actionsbot 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:mainMar 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

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 #46832

Uh oh!

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!

addaleaxFeb 25, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Feb 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!

jasnellFeb 28, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25Feb 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

debadree25 commented Feb 25, 2023•
edited
Loading