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

src: trace_event: secondary storage for metadata #20900

Closed
wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented May 22, 2018

Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

/cc @nodejs/trace-events

CI:

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

@ofrobots ofrobots requested a review from jasnell May 22, 2018 22:48
@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. labels May 22, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 7, 2018
@ofrobots ofrobots added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jun 26, 2018
@ofrobots
Copy link
Contributor Author

I am unable to build on AIX to reproduce the timeout that only happens on AIX.

~/tmp/ofrobots/aix61-ppc64 $ gmake
gmake -C out BUILDTYPE=Release V=1
gmake[1]: Entering directory `/home/iojs/tmp/ofrobots/aix61-ppc64/out'
node.target.mk:13: warning: overriding recipe for target `/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/node.exp'
cctest.target.mk:13: warning: ignoring old recipe for target `/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/node.exp'
  touch 4874ac81486ce32779d2dbfe562b8f1b6ec63989.intermediate
  LD_LIBRARY_PATH=/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/lib.host:/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/src/node/inspector/protocol; python deps/v8/third_party/inspector_protocol/CodeGenerator.py --jinja_dir deps/v8/third_party/inspector_protocol/.. --output_base "/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/src/" --config "/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/node_protocol_config.json"
  touch 48324d268abcc345f409e144bccee591e70bdbe8.intermediate
  LD_LIBRARY_PATH=/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/lib.host:/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/gypfiles; mkdir -p /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/src/inspector/protocol /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/include/inspector; python ../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../third_party --output_base "/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj/gen/src/inspector" --config ../src/inspector/inspector_protocol_config.json
  g++ -pthread -Wl,-bbigtoc -maix64  -o /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/genccode /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj.host/genccode/deps/icu-small/source/tools/genccode/genccode.o /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj.host/genccode/tools/icu/no-op.o /home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/obj.host/tools/icu/libicutools.a
ld: 0711-317 ERROR: Undefined symbol: .std::type_info::operator==(std::type_info const&) const
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
collect2: error: ld returned 8 exit status
gmake[1]: *** [/home/iojs/tmp/ofrobots/aix61-ppc64/out/Release/genccode] Error 1
rm 4874ac81486ce32779d2dbfe562b8f1b6ec63989.intermediate 48324d268abcc345f409e144bccee591e70bdbe8.intermediate
gmake[1]: Leaving directory `/home/iojs/tmp/ofrobots/aix61-ppc64/out'
gmake: *** [node] Error 2

This is after I setup the environment as per select-compiler.sh.

I'm stuck at this point. @mhdawson @richardlau is there a way to get help with this?

The test that times out has a large number of FS/IO operations in it. It is quite possible that the AIX machines genuinely not able able to finish this work in 120s. Either way, I cannot build on the machine, so I am unable to debug.

@richardlau
Copy link
Member

cc @nodejs/platform-aix @jBarz

@ofrobots
Copy link
Contributor Author

@refack pointed me to the job config on jenkins which fills in more blanks for me. I can build now.

@richardlau
Copy link
Member

Out of interest, what was the missing piece?

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 28, 2018 via email

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 28, 2018

The aix timeout should be fixed by #21335. On AIX the race manifests as an infinite loop on this particular test on erroneous shutdown.

EDIT: minimal repro: ./node --trace-events-enabled -e 'throw 42'.

@ofrobots ofrobots added the blocked PRs that are blocked by other issues or PRs. label Jun 28, 2018
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Ping @ofrobots

@ofrobots
Copy link
Contributor Author

Thanks for the ping. I plan to come back to this later this week.

@ofrobots ofrobots removed the blocked PRs that are blocked by other issues or PRs. label Aug 13, 2018
@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

Ping again :-)

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018
@ofrobots
Copy link
Contributor Author

I am planning to come back to this once #22865 is addressed, i.e. after libuv/libuv#2003, #23110, and #22938 land.

@ofrobots ofrobots removed the stalled Issues and PRs that are stalled. label Oct 11, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 12, 2018

This is updated now given the cascade of blockers are now resolved. New CI: https://ci.nodejs.org/job/node-test-pull-request/17751/

Linter re-run: https://ci.nodejs.org/job/node-test-linter/22720/

Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.
@ofrobots
Copy link
Contributor Author

CI is green. @jasnell assuming your LGTM from 5 months ago (!) still holds, I am going to land this later today. The rebase did not change this PR in substance.

@ofrobots ofrobots added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018
@addaleax
Copy link
Member

Landed in f5986a4

@addaleax addaleax closed this Oct 12, 2018
addaleax pushed a commit that referenced this pull request Oct 12, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

PR-URL: #20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@ofrobots
Copy link
Contributor Author

backport on #23700

jasnell pushed a commit that referenced this pull request Oct 17, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

PR-URL: #20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ofrobots added a commit to ofrobots/node that referenced this pull request Nov 8, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

PR-URL: nodejs#20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: nodejs#23700
MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

Backport-PR-URL: #23700
PR-URL: #20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: #23700
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

Backport-PR-URL: #23700
PR-URL: #20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: #23700
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Metadata trace-events should be held in secondary storage so that they
can be periodically reemitted. This change establishes the secondary
storage and ensures that events are reemitted on each flush.

Backport-PR-URL: #23700
PR-URL: #20900
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: #23700
@codebytere codebytere mentioned this pull request Nov 29, 2018
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++. lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants