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.

Sign up for GitHub

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

Jump to bottom

Deprecating lttng tracepoints. (and add alternative for linux if wanted) #18971

Closed
GlenTiki opened this issue Feb 24, 2018 · 5 comments
Closed

Deprecating lttng tracepoints. (and add alternative for linux if wanted) #18971

GlenTiki opened this issue Feb 24, 2018 · 5 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks.

Comments

@GlenTiki
Copy link
Contributor

GlenTiki commented Feb 24, 2018 •
edited
Loading

I am proposing that we deprecate lttng tracepoints in favour of vanilla linux tracepoint support. I've started working on a POC for this personally.

I added lttng support way back in the day: #702

However, a PR has been opened recently that reveals building with lttng support has broken for over 2 years: #18945

And this is the commit that broke it: f1d2792 (June 28, 2016)

Very few people seem to use it or need it based on the above pr. The benefit of lttng is that when node is built with support for it, you can activate tracing your program from lttng without much overhead affecting it. It has been designed to be low enough overhead that you can run it in production to get useful information from the environment, if needed.

However, to build and use node with lttng support enabled, you need to be on linux with lttng installed on your system. There is not much value added to the general linux userbase because there is barrier of entry to using this that requires specific lttng knowledge.

If we added support for vanilla linux tracepoints, as @AndreasMadsen suggests here, lttng users can still consume these. By adding vanilla linux tracepoints, other alternative tracing tools should be able to use these new tracepoints too. Additionally we could consider beginning to compile node with support for these tracepoints enabled by default on linux.
@GlenTiki GlenTiki added c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. labels Feb 24, 2018
@richardlau
Copy link
Member

Cc @nodejs/diagnostics

@AndreasMadsen
Copy link
Member

@thekemkid Do you know if the vanilla linux tracepoints support dynamic tracing names? With our new tracing point implementation we would like to support dynamic tracing points, such users can emit them too. For DTrace libusdt exists.

@GlenTiki
Copy link
Contributor Author

@AndreasMadsen I think these are static tracepoints, but I may be wrong. I only started exploring them last night.

@GlenTiki
Copy link
Contributor Author

Relevant PR: #18975

@ChALkeR ChALkeR mentioned this issue Feb 24, 2018
Closed
2 tasks
@GlenTiki GlenTiki mentioned this issue Feb 24, 2018
Closed
2 tasks
GlenTiki added a commit to GlenTiki/node that referenced this issue Feb 25, 2018
@GlenTiki
src: Remove lttng support.
a170f7b 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945
GlenTiki added a commit that referenced this issue Mar 1, 2018
@GlenTiki
src: Remove lttng support.
30fd3d2 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: #18971
Refs: #18975
Refs: #18945

PR-URL: #18982
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
GlenTiki added a commit that referenced this issue Mar 1, 2018
@GlenTiki
doc: add a deprecation message for removing lttng
740c426 
PR-URL: #18982
Refs: #18971
Refs: #18975
Refs: #18945
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@GlenTiki @MayaLekova
src: Remove lttng support.
fd24991 
This cleans up and removes lttng support completely. Recent discussion
on a PR to deprecate lttng suggested that we remove it completely
pending feedback from the TSC.

This should be considered a non breaking change, as a recent PR reveals
that compiling with this system has been broken for nearly two years.

Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945

PR-URL: nodejs#18982
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@GlenTiki @MayaLekova
doc: add a deprecation message for removing lttng
98660ee 
PR-URL: nodejs#18982
Refs: nodejs#18971
Refs: nodejs#18975
Refs: nodejs#18945
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 26, 2018

Am I wrong to think this can be closed since #18982 landed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@AndreasMadsen @Trott @richardlau @GlenTiki