trace_events: add traced_value.cc/traced_value.h by jasnell · Pull Request #21475 · nodejs/node

jasnell · 2018-06-22T19:59:48Z

Port of the V8 internal v8::tracing::TracedValue that allows
structured data to be included in the trace event. The v8 class
is not exported in the public API so we cannot use it directly.

This is a simplified and slightly modified port. This commit only
adds the class, it does not add uses of it. Those will come in
separate PRs/commits.

This will be used to include more complex data structures within
various trace events.

/cc @ofrobots @eugeneo @nodejs/diagnostics @nodejs/trace-events

Checklist

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

nodejs-github-bot · 2018-06-22T19:59:50Z

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

jasnell · 2018-06-22T20:01:17Z

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

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

ping @nodejs/diagnostics

mike-kaufman · 2018-06-29T21:18:02Z

not following how this is being used, and how its value is correct after a call sequence like BeginArray, BeginDictionary, EndDictionary.

Look within the WriteComma function for use (https://github.com/nodejs/node/pull/21475/files#diff-7e8d44b3c53ea475e1b34c59578ed6e8R169) ...
The original implementation in V8 has DCHECK statements that are only enabled in debug builds that check for proper state. I pulled those to keep things simple but could add them back in as regular CHECKs

addaleax

Code-wise LGTM, but it’s hard to give feedback about the API itself without knowing how this is going to end up being used ;)

addaleax · 2018-06-29T21:21:41Z

nit: It would be nicer to return a std::string instead of making result an in/out parameter, tbh…

I agree. I adopted this directly from the v8 impl. If it's ok to diverge then I can change

addaleax · 2018-06-29T21:26:03Z

Does return std::to_string(v); do the trick here too, for the default case?

I'll double check

Ah, right, the challenge with std::to_string(v) is that it ignores any reasonable precision on the double and always prints six decimals, whereas the version used here will print with actual scientific notation (e.g. std::to_string(v) when v = 1.23e7, will print 12300000.000000 whereas the version used here will print 1.23e+07 as one would more reasonably expect.

addaleax · 2018-06-29T21:28:55Z

Same here, seems like it might be nicer to make out a return value rather than an in/out parameter

This is part of the v8 trace event API and really isn't under our control, unfortunately.

mcollina

Can you please add a test that verifies that it escape strings correctly? JSON a few special chars, as well as UTF-8.

Port of the V8 internal v8::tracing::TracedValue that allows structured data to be included in the trace event. The v8 class is not exported in the public API so we cannot use it directly. This is a simplified and slightly modified port. This commit only adds the class, it does not add uses of it. Those will come in separate PRs/commits.

jasnell · 2018-07-11T20:28:29Z

@mcollina ... Neither this nor the internal V8 version of TracedValue support proper escaping of UTF8 characters yet. I can update so that it does properly escape but that would mean further variance from the internal V8 version (which really shouldn't be a problem).

jasnell · 2018-07-11T23:42:20Z

@mcollina @addaleax ... PTAL, I've updated the escaping to properly handle UTF8 characters, added the requested escaping test, and made a couple other minor improvements.

jasnell · 2018-07-11T23:42:52Z

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

jasnell · 2018-07-11T23:46:11Z

@addaleax ... to give you a better idea about how this is going to be used... here's an example

auto traced_value=TracedValue::Create();
traced_value->SetString("abc", "xyz");
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("cat", "name", 1, "data", traced_value);

Within the trace event log, the traced_value will be serialized as additional JSON payload as the value of the data argument. What this allows us to do is provide complex structured and nested data within the trace event as opposed to simple primitives. One example of this will be trace events for http and http2, which will optionally include detailed information about the session, the headers, etc.

jasnell · 2018-07-12T04:44:37Z

Related failures in Windows and a couple others... trying again: https://ci.nodejs.org/job/node-test-pull-request/15819/

New run on Windows failed early for some reason... trying again on that platform: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19232/

jasnell · 2018-07-12T05:27:05Z

@nodejs/build ... Windows CI is failing with an odd one...


[git-rebase] $ /bin/sh -xe /tmp/jenkins6120436250089634014.sh
+ git checkout -B jenkins-node-test-commit-windows-fanned-19232
Switched to a new branch 'jenkins-node-test-commit-windows-fanned-19232'
+ grep -q ^147.75.70.237 /home/iojs/.ssh/known_hosts
+ git push binary_tmp@147.75.70.237:binary_tmp.git +jenkins-node-test-commit-windows-fanned-19232
ssh: connect to host 147.75.70.237 port 22: Connection timed out
fatal: Could not read from remote repository.

Trott · 2018-07-12T06:06:58Z

Unfortunately, for windows-fanned jobs, "Resume Build" does not work. (I don't know if this is something Build WG can fix or not.) So for that, you need to use Rebuild on the windows-fanned job itself instead.

~~Regardless, it looks like you've pushed two commits since the last full CI run (if GitHub's interface isn't misleading me somehow) so a full CI run is in order.~~ EDIT: Indeed I am mistaken.

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

Trott · 2018-07-12T06:09:09Z

Hmm...lots of stuff failing very fast. Something odd is up...

Trott · 2018-07-12T06:10:20Z

Ah, the problem might be test-softlayer-ubuntu1604-x64-1 which builds for a few different tasks. I'll see if someone is around on #node-build who might know how to investigate/fix.

jasnell · 2018-07-12T12:45:16Z

@Trott ... I was not using "Resume Build". When adding new commits, I always start a full CI run.

jasnell · 2018-07-12T12:46:07Z

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

jasnell · 2018-07-12T13:59:52Z

woo... green CI.

mcollina

LGTM

Port of the V8 internal v8::tracing::TracedValue that allows structured data to be included in the trace event. The v8 class is not exported in the public API so we cannot use it directly. This is a simplified and slightly modified port. This commit only adds the class, it does not add uses of it. Those will come in separate PRs/commits. PR-URL: #21475 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2018-07-12T17:36:13Z

Landed in d85449d

Port of the V8 internal v8::tracing::TracedValue that allows structured data to be included in the trace event. The v8 class is not exported in the public API so we cannot use it directly. This is a simplified and slightly modified port. This commit only adds the class, it does not add uses of it. Those will come in separate PRs/commits. PR-URL: #21475 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 Jun 22, 2018

jasnell requested review from addaleax and ofrobots June 29, 2018 21:12

mike-kaufman reviewed Jun 29, 2018
View reviewed changes

addaleax approved these changes Jun 29, 2018
View reviewed changes

mcollina reviewed Jun 30, 2018
View reviewed changes

jasnell force-pushed the add-traced-value branch from 98665ce to 509a715Compare July 11, 2018 18:44

[Squash] add test, better escaping[Squash] add test, better escaping
629f936

jasnell added 2 commits July 11, 2018 16:58

[Squash] fix lint
239e870

[Squash] hopefully avoid failure on windows
8f25cc1

[Squash] fix without-intl[Squash] fix without-intl
c62c810

mcollina approved these changes Jul 12, 2018
View reviewed changes

jasnell closed this Jul 12, 2018

targos mentioned this pull request Jul 17, 2018
v10.7.0 proposal #21851
Merged

Uh oh!

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 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

jasnell commented Jul 11, 2018

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Jul 12, 2018

Uh oh!

Trott commented Jul 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 12, 2018

Uh oh!

Trott commented Jul 12, 2018

Uh oh!

jasnell commented Jul 12, 2018

Uh oh!

jasnell commented Jul 12, 2018

Uh oh!

jasnell commented Jul 12, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasnell commented Jul 12, 2018•
edited
Loading

Trott commented Jul 12, 2018•
edited
Loading