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

export v8 trace API #42462

Closed
wants to merge 1 commit into from
Closed

Conversation

theanarkh
Copy link
Contributor

export v8 trace API, so user can use it to product custom trace events and collect it by inspector or trace_events.createTracing

  • 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 nodejs-github-bot added needs-ci PRs that need a full CI run. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Mar 25, 2022
@theanarkh theanarkh force-pushed the export_v8_trace_api branch 2 times, most recently from 67aa73a to 45c6c04 Compare March 25, 2022 05:01
doc/api/tracing.md Outdated Show resolved Hide resolved
doc/api/tracing.md Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the export_v8_trace_api branch 3 times, most recently from 4e7a546 to 53c8fb2 Compare March 25, 2022 17:16
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Not sure we should do this. I mean, I generally prefer this, but when we added that trace API it was with the agreement with the v8 team that we would not expose a public API for it since they might still want to make changes to the underlying mechanism and exposing the public apis would make that more difficult.

@jasnell jasnell added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 25, 2022
@theanarkh
Copy link
Contributor Author

Not sure we should do this. I mean, I generally prefer this, but when we added that trace API it was with the agreement with the v8 team that we would not expose a public API for it since they might still want to make changes to the underlying mechanism and exposing the public apis would make that more difficult.

Thanks for the reply.

export v8 trace api, so user can use it to trace custom events
and collect the data by using inspector or the
trace_events.createTracing method
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I agree with @jasnell. Maybe @nodejs/v8 might chime in as well.

@jasnell
Copy link
Member

jasnell commented Apr 14, 2022

We spoke about this issue yesterday on the TSC call and landed on not wanting to expose this API right now, for more reasons then just the agreement with v8 that we wouldn't. The trace event mechanism in node is not super well maintained and has a good number of issues that will need to be addressed. Before we do more with it, and especially before we introduce apis that have to be supported long term, we should ensure the internal implementation is improved and made more robust.

@camillobruni
Copy link
Contributor

Could you point me to the C++ part of the tracing API? I wasn't aware that we made that available in the V8 API.

Generally though, Chrome's tracing system has a fairly stable and simple API: (category, name, payload1...)
Among all the questionable APIs we have, this doesn't sound too bad.

@jasnell
Copy link
Member

jasnell commented Apr 14, 2022

Here's the view from our vendored in v8 history for this: https://github.com/nodejs/node/commits/master/deps/v8/src/builtins/builtins-trace.cc

Ultimately, I'd really like to see the trace event system in Node.js get replaced with Perfetto with a fully supported API we can expose to users. If v8 doesn't mind us exposing an API here, then awesome, but we still need someone to dig in on the trace event subsystem and get it stable and out of experimental.

@theanarkh
Copy link
Contributor Author

Thanks for the reply, do i need to close this PR ?

@jasnell
Copy link
Member

jasnell commented Apr 14, 2022

@theanarkh ... for now, yeah, this PR can be closed. But, if trace events are something that are of interest to you, I'd strongly encourage you to consider helping get the internals updated and in a better shape! In the meantime, we can work with the v8 folks to come up with a strategy around the api that we can be sure they have no problem supporting long term :-)

@camillobruni
Copy link
Contributor

Ok I didn't know this was exposed as a Builtin in V8, which is far from ideal.

What speaks against implementing this purely on node-side and use perfetto there directly?

@jasnell
Copy link
Member

jasnell commented Apr 14, 2022

The reason we originally went with the builtin approach was purely performance motivated. The builtin was a whole lot faster at the time it turned out. That said, I think we could absolutely go with a node+perfetto based solution here... we just need someone to do the work to get perfetto integrated. I had started work in that direction a while back but couldn't seem to get folks interested in advancing it forward: #35900

@theanarkh
Copy link
Contributor Author

Thanks, i will try my best to do that.

@theanarkh theanarkh closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants