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

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

Merged
merged 20 commits into from
Mar 6, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Feb 25, 2023

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

@nodejs-github-bot 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
@debadree25
Copy link
Member Author

cc @nodejs/workers

@debadree25
Copy link
Member Author

Screenshot 2023-02-25 at 8 32 51 PM

It looks something like this on VSCode

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 25, 2023
src/node.h Outdated
@@ -677,7 +677,8 @@ NODE_EXTERN Environment* CreateEnvironment(
NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
Environment* parent_env,
ThreadId child_thread_id,
const char* child_url);
const char* child_url,
const char* title_prefix);
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, could you check

@RaisinTen
Copy link
Contributor

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

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

@debadree25
Copy link
Member Author

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

@debadree25
Copy link
Member Author

Added tests and doc, opening it for review!

@debadree25 debadree25 marked this pull request as ready for review February 26, 2023 08:12
@addaleax 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
src/api/environment.cc Show resolved Hide resolved
src/api/environment.cc Show resolved Hide resolved
@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@bnoordhuis
Copy link
Member

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
Copy link
Member Author

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

@debadree25
Copy link
Member Author

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

session.connect();
session.on('NodeWorker.attachedToWorker', ({ params: { workerInfo } }) => {
const id = workerInfo.id;
const expectedTitle = `${titlePrefix}Worker ${id}`;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@debadree25 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
@nodejs-github-bot
Copy link
Collaborator

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

@debadree25 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 6, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 6, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 2d9bf91 into nodejs:main Mar 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2d9bf91

targos pushed a commit that referenced this pull request Mar 13, 2023
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>
targos pushed a commit that referenced this pull request Mar 14, 2023
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>
targos added a commit that referenced this pull request Mar 14, 2023
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
targos added a commit that referenced this pull request Mar 14, 2023
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
targos added a commit that referenced this pull request Mar 14, 2023
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
targos added a commit that referenced this pull request Mar 14, 2023
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
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
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>
danielleadams added a commit that referenced this pull request Apr 11, 2023
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
danielleadams added a commit that referenced this pull request Apr 11, 2023
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
danielleadams added a commit that referenced this pull request Apr 11, 2023
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
danielleadams added a commit that referenced this pull request Apr 12, 2023
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
danielleadams added a commit that referenced this pull request Apr 13, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give a name to a Worker (for debugging purpose)
8 participants