worker: add support for worker name in inspector and trace_events

[debadree25]

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]

cc @nodejs/workers

[debadree25]

It looks something like this on VSCode

[addaleax]

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)

[debadree25]

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

[debadree25]

Updated, could you check

[RaisinTen]

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]

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

[debadree25]

Added tests and doc, opening it for review!

[bnoordhuis]

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]

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

[debadree25]

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

[jasnell]

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.

[debadree25]

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

[nodejs-github-bot]

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]

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

[nodejs-github-bot]

Landed in 2d9bf91