src: add --title command line argument, trace event by jasnell · Pull Request #21477 · nodejs/node

jasnell · 2018-06-22T23:27:16Z

Adds a simple utility --title command line argument to set the process title on startup. Also, emit the process_name metadata to the trace event log.

$ node --title=foo -pe "process.title"
foo

When emit trace events, the process name appears within the trace event viewer UI:

$ node --title=foo --trace-event-categories node -pe "process.title"
foo

Changes to process.title also appear within the trace_event log:

/cc @ofrobots @eugeneo

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-06-22T23:27:17Z

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/128/pipeline

jasnell · 2018-06-22T23:27:52Z

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

jasnell · 2018-06-23T00:30:02Z

some relevant failures in CI.

jasnell · 2018-06-23T01:10:53Z

@ofrobots ... when you get a moment, can you take a look at this? It would appear that TRACE_STR_COPY() may not be actually be copying for metadata events, but it's difficult to verify.

vsemozhetbyt · 2018-06-23T04:38:38Z

doc/node.1

Nit: missing period.

Nit: remove the

Trott · 2018-06-23T05:03:18Z

doc/api/cli.md

Nit: remove the

jasnell · 2018-06-24T22:15:21Z

Still exploring why the trace event bit is failing on some of the CI machines.. unable to recreate locally... https://ci.nodejs.org/job/node-test-pull-request/15587/

addaleax · 2018-06-25T13:26:54Z

src/node.cc

Are you sure this is safe? Setting the process title (which on a lot of OS works by overwriting argv) while iterating over argv?

Possibly not ;) still verifying and chasing down a few issues that are likely being caused by exactly this problem. I have a change staged locally that moves the actual set_process_title call to after the args are processed but haven't pushed it up to test yet

Is the addition of --title tied to the trace event things, or is it a convenience thing? It seems like it’s pretty easy already to set the process title from inside the JS code, so it might be okay to leave this out…?

They are not tied together but the use case is closely related for me. The introduction of --title is specifically for cases of profiling other users code that I don't exactly want to have to modify just to set the title... using a preload module is a bit overkill for that.

jasnell · 2018-06-25T18:32:17Z

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

jasnell · 2018-06-25T20:55:18Z

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

This is failing on SmartOS only at this point.

Simple utility command line argument for setting the process title on process startup.

jasnell · 2018-06-25T22:18:00Z

Ok, should be good to go now: https://ci.nodejs.org/job/node-test-pull-request/15615/

jasnell · 2018-06-25T23:53:01Z

CI is good. One unrelated flaky failure in windows.

Trott · 2018-06-26T05:33:14Z

CI is good. One unrelated flaky failure in windows.

Then it's not quite good yet. :-D

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

jasnell · 2018-06-29T21:10:26Z

Let's give it one more shot since the unrelated windows failure happened again https://ci.nodejs.org/job/node-test-pull-request/15676/

PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Simple utility command line argument for setting the process title on process startup. PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2018-07-11T00:34:03Z

Landed in 6705356 and 9d71619

PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Simple utility command line argument for setting the process title on process startup. PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477)

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * inspector: * Expose the original console API in `require('inspector').console`. (#21659) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 22, 2018

bmeck approved these changes Jun 22, 2018

View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Jun 23, 2018

jasnell force-pushed the trace-event-data branch from 8b9d470 to 51111a8 Compare June 23, 2018 01:11

vsemozhetbyt reviewed Jun 23, 2018

View reviewed changes

Trott reviewed Jun 23, 2018

View reviewed changes

doc/api/cli.md Outdated

Copy link

Member

Trott Jun 23, 2018

Choose a reason for hiding this comment

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

Nit: remove the

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 23, 2018

addaleax reviewed Jun 25, 2018

View reviewed changes

jasnell added 2 commits June 25, 2018 15:14

trace_events: add process_name metadata

62efea9

src: add --title command line argument

550335e

Simple utility command line argument for setting the process title on process startup.

jasnell force-pushed the trace-event-data branch from b8c686b to 550335e Compare June 25, 2018 22:14

addaleax approved these changes Jun 25, 2018

View reviewed changes

addaleax removed the wip Issues and PRs that are still a work in progress. label Jun 25, 2018

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 30, 2018

jasnell added a commit that referenced this pull request Jul 11, 2018

trace_events: add process_name metadata

6705356

PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell closed this Jul 11, 2018

targos pushed a commit that referenced this pull request Jul 12, 2018

trace_events: add process_name metadata

c4d7413

PR-URL: #21477 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

Uh oh!

Conversation

jasnell commented Jun 22, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 22, 2018

Uh oh!

jasnell commented Jun 22, 2018

Uh oh!

jasnell commented Jun 23, 2018

Uh oh!

jasnell commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 24, 2018

Uh oh!

addaleax Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 25, 2018

Uh oh!

jasnell commented Jun 25, 2018

Uh oh!

jasnell commented Jun 25, 2018

Uh oh!

jasnell commented Jun 25, 2018

Uh oh!

Trott commented Jun 26, 2018

Uh oh!

jasnell commented Jun 29, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasnell commented Jun 23, 2018 •

edited

Loading

jasnell Jun 25, 2018 •

edited

Loading