trace_events: add file pattern cli option #18480

AndreasMadsen · 2018-01-31T16:17:05Z

Allow the user to specify the filepath for the trace_events log file using a printf format string.

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

Affected core subsystem(s)

trace_events

/cc @ofrobots

AndreasMadsen · 2018-01-31T16:18:35Z

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

vsemozhetbyt · 2018-01-31T16:38:50Z

doc/api/tracing.md

@@ -20,6 +20,15 @@ Running Node.js with tracing enabled will produce log files that can be opened
 in the [`chrome://tracing`](https://www.chromium.org/developers/how-tos/trace-event-profiling-tool)
 tab of Chrome.

+The logging file is by default called `node_trace.1.log`, where `1` is an
+incrementing log-rotation id. You can specify the filepath pattern with
+`--trace-event-file-pattern` that accepts an `printf` style format string.


nit: an printf -> a printf

Please avoid using you in the docs :-)

richardlau · 2018-01-31T18:35:55Z

src/node.cc

@@ -3510,6 +3514,7 @@ static void CheckIfAllowedInEnv(const char* exe, bool is_env,
    "--no-force-async-hooks-checks",
    "--trace-events-enabled",
    "--trace-event-categories",
+    "--trace-event-file-pattern",


Please could you also update the list of whitelisted options in the NODE_OPTIONS section of the CLI documentation?

richardlau · 2018-01-31T19:13:00Z

src/tracing/node_trace_writer.cc

-  fd_ = uv_fs_open(tracing_loop_, &req, log_file.str().c_str(),
+
+  char log_file[1024];
+  snprintf(log_file, 1024, log_file_pattern_, file_num_, uv_os_getpid());


Since there's no validation of log_file_pattern things can go bad here.
e.g.,

-bash-4.2$ ./node --trace-events-enabled --trace-event-file-pattern 'tracelog.%s.log' -e 'setTimeout(() => { for (var i = 0; i < 100000; i++) { "test" + i } }, 1)' Warning: Trace event is an experimental feature and could change at any time. Segmentation fault (core dumped) -bash-4.2$

I know. But it is a CLI flag, we document what the user should use, and it is an advanced feature. Adding validation would add a lot of complexity here.

ofrobots · 2018-01-31T23:47:33Z

I am not opposed to this approach, but have you considered something simpler like:

Changing the default pattern to be ${prefix}-${pid}-${logRotationId}.log.
Users can only specify a prefix.

This gets rid of the need to do parsing and formatting, and is an easier option for users to remember and correctly specify?

AndreasMadsen · 2018-02-01T08:35:08Z

@ofrobots That only works if the trace_event file should be in the cwd.

For https://github.com/nearform/node-clinic-doctor we collect data from multiple sources and store each of them in a file. To not make it too noisy we put all of them in a directory called ${pid}.clinic-doctor. The trace_events file will then be called ${pid}.clinic-doctor/${pid}.clinic-doctor-trace-events (we don't support log rotation yet).

Right now this is done by an fs.rename op (https://github.com/nearform/node-clinic-doctor/blob/master/index.js#L82) but in cases where something went wrong that fs.rename isn't called. However, we are still interested in the partial data and would like for it to be in the ${pid}.clinic-doctor directory.

ofrobots · 2018-02-01T23:59:24Z

@AndreasMadsen I share the concerns from @richardlau here. Advanced feature or not, propensity for segfaulting isn't very nice. Could we deem that the prefix could include a directory + filename components, and go with the simpler approach as I proposed? Something like:

node --trace-events-enabled --trace-event-file-prefix /path/to/trace-file

AndreasMadsen · 2018-02-02T06:49:47Z

@ofrobots That doesn't really satisfy my usecase for having the pid in the data directory.

AndreasMadsen · 2018-02-02T06:54:20Z

I suppose we could allow a syntax like ${pid}-${logRotationId}.log and use string.find and string.replace to build a printf format string.

AndreasMadsen · 2018-02-02T08:20:45Z

All comments should be addressed now. PTAL

ofrobots

LGTM

ofrobots · 2018-02-03T01:40:58Z

src/node.cc

@@ -3385,6 +3386,9 @@ static void PrintHelp() {
         "  --trace-events-enabled     track trace events\n"
         "  --trace-event-categories   comma separated list of trace event\n"
         "                             categories to record\n"
+         "  --trace-event-file-pattern printf format string specifying the\n"
+         "                             logging path. %%1$u is the\n"


nit: this bit needs to be updated with the new template strings.

bnoordhuis · 2018-02-03T10:19:47Z

logRotationId is a bit unwieldy to type. Perhaps use a shorter name.

AndreasMadsen · 2018-02-04T11:03:14Z

I called it ${rotation} now.

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

AndreasMadsen · 2018-02-04T11:45:32Z

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

AndreasMadsen · 2018-02-05T10:20:58Z

Apparently, we just changed the way tempdirs are created, thus the test failed after the CI rebased.

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

AndreasMadsen · 2018-02-09T09:28:01Z

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

AndreasMadsen · 2018-02-09T10:40:49Z

There appear to be some failures. Unfortunately, I don't have easy access to any of those OS's. It could also be a compile-chain issue. Any ideas?

ofrobots · 2018-02-09T22:05:04Z

There were infra issues on at least some of the machines, here's a relaunch: https://ci.nodejs.org/job/node-test-pull-request/13072/

jasnell

LGTM once CI is figured out. There's nothing obvious that stands out as being problematic.

apapirovski · 2018-03-04T11:08:29Z

Landed in 85212bb

Allow the user to specify the filepath for the trace_events log file using a template string. PR-URL: #18480 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Allow the user to specify the filepath for the trace_events log file using a template string. Backport-PR-URL: #19144 PR-URL: #18480 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) #17690 * lib: - v8_prof_processor works again 🎉 (Anna Henningsen) #19059 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) #18949 * src: - handle exceptions in env-\>SetImmediates (James M Snell) #18297 - make process.dlopen() load well-known symbol (Ben Noordhuis) #18934 * trace_events: - add file pattern cli option (Andreas Madsen) #18480 PR-URL: #19181

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) #17690 * http2: - Fixed issues with aborted connections in the HTTP/2 implementation (Anna Henningsen) #18987 #19002 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) #18949 * src: - make process.dlopen() load well-known symbol (Ben Noordhuis) #18934 * trace_events: - add file pattern cli option (Andreas Madsen) #18480 * Added new collaborators: - Chen Gang (MoonBall) https://github.com/MoonBall PR-URL: #19181

Allow the user to specify the filepath for the trace_events log file using a template string. PR-URL: nodejs#18480 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * crypto: - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) nodejs#17690 * http2: - Fixed issues with aborted connections in the HTTP/2 implementation (Anna Henningsen) nodejs#18987 nodejs#19002 * loader: - --inspect-brk now works properly for esmodules (Gus Caplan) nodejs#18949 * src: - make process.dlopen() load well-known symbol (Ben Noordhuis) nodejs#18934 * trace_events: - add file pattern cli option (Andreas Madsen) nodejs#18480 * Added new collaborators: - Chen Gang (MoonBall) https://github.com/MoonBall PR-URL: nodejs#19181

Allow the user to specify the filepath for the trace_events log file using a template string. Backport-PR-URL: #19145 PR-URL: #18480 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

AndreasMadsen added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v4.x trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Jan 31, 2018

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. labels Jan 31, 2018

vsemozhetbyt reviewed Jan 31, 2018

View reviewed changes

richardlau reviewed Jan 31, 2018

View reviewed changes

ofrobots approved these changes Feb 3, 2018

View reviewed changes

richardlau approved these changes Feb 3, 2018

View reviewed changes

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

AndreasMadsen mentioned this pull request Feb 8, 2018

Clinic v0.6.2 / Node v8.9.4: Error: Invalid JSON (Unexpected "p" at position 24584 in state STOP) clinicjs/node-clinic#9

Closed

AndreasMadsen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2018

AndreasMadsen mentioned this pull request Feb 9, 2018

JSON parsing issue clinicjs/node-clinic#7

Closed

jasnell approved these changes Feb 10, 2018

View reviewed changes

BridgeAR added the wip Issues and PRs that are still a work in progress. label Feb 10, 2018

AndreasMadsen mentioned this pull request Feb 20, 2018

make trace-events write to configurable sink nodejs/diagnostics#152

Closed

jasnell approved these changes Mar 1, 2018

View reviewed changes

BridgeAR approved these changes Mar 2, 2018

View reviewed changes

apapirovski closed this Mar 4, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trace_events: add file pattern cli option #18480

trace_events: add file pattern cli option #18480

AndreasMadsen commented Jan 31, 2018

AndreasMadsen commented Jan 31, 2018

vsemozhetbyt Jan 31, 2018

jasnell Jan 31, 2018

richardlau Jan 31, 2018

richardlau Jan 31, 2018 •

edited

Loading

AndreasMadsen Jan 31, 2018

ofrobots commented Jan 31, 2018

AndreasMadsen commented Feb 1, 2018 •

edited

Loading

ofrobots commented Feb 1, 2018

AndreasMadsen commented Feb 2, 2018

AndreasMadsen commented Feb 2, 2018

AndreasMadsen commented Feb 2, 2018

ofrobots left a comment

ofrobots Feb 3, 2018

bnoordhuis commented Feb 3, 2018

AndreasMadsen commented Feb 4, 2018

AndreasMadsen commented Feb 4, 2018

AndreasMadsen commented Feb 5, 2018

AndreasMadsen commented Feb 9, 2018

AndreasMadsen commented Feb 9, 2018

ofrobots commented Feb 9, 2018

jasnell left a comment

apapirovski commented Mar 4, 2018

trace_events: add file pattern cli option #18480

trace_events: add file pattern cli option #18480

Conversation

AndreasMadsen commented Jan 31, 2018

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jan 31, 2018

vsemozhetbyt Jan 31, 2018

Choose a reason for hiding this comment

jasnell Jan 31, 2018

Choose a reason for hiding this comment

richardlau Jan 31, 2018

Choose a reason for hiding this comment

richardlau Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

AndreasMadsen Jan 31, 2018

Choose a reason for hiding this comment

ofrobots commented Jan 31, 2018

AndreasMadsen commented Feb 1, 2018 • edited Loading

ofrobots commented Feb 1, 2018

AndreasMadsen commented Feb 2, 2018

AndreasMadsen commented Feb 2, 2018

AndreasMadsen commented Feb 2, 2018

ofrobots left a comment

Choose a reason for hiding this comment

ofrobots Feb 3, 2018

Choose a reason for hiding this comment

bnoordhuis commented Feb 3, 2018

AndreasMadsen commented Feb 4, 2018

AndreasMadsen commented Feb 4, 2018

AndreasMadsen commented Feb 5, 2018

AndreasMadsen commented Feb 9, 2018

AndreasMadsen commented Feb 9, 2018

ofrobots commented Feb 9, 2018

jasnell left a comment

Choose a reason for hiding this comment

apapirovski commented Mar 4, 2018

richardlau Jan 31, 2018 •

edited

Loading

AndreasMadsen commented Feb 1, 2018 •

edited

Loading