Skip to content

doc: deprecate lttng#18975

Closed
GlenTiki wants to merge 1 commit intonodejs:masterfrom
GlenTiki:deprecate-lttng
Closed

doc: deprecate lttng#18975
GlenTiki wants to merge 1 commit intonodejs:masterfrom
GlenTiki:deprecate-lttng

Conversation

@GlenTiki
Copy link
Contributor

@GlenTiki GlenTiki commented Feb 24, 2018

Building with-lttng has been broken for 2 years. We should
deprecate and remove this dead code.

Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 24, 2018
@GlenTiki GlenTiki mentioned this pull request Feb 24, 2018
Closed
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Feb 24, 2018 •
edited
Loading

End-of-Life means that the probes are actually removed. However, this doesn't actually remove them. This is barely a documentation deprecation.

edit: Ordinarily, one would documentation deprecate a feature first. But given that this feature hasn't worked for two years and no one have complained, we can properly remove it immediately. But I think that requires TSC approval.

@GlenTiki
Copy link
Contributor Author

GlenTiki commented Feb 24, 2018

@AndreasMadsen I've changed it to Documentation only.

@GlenTiki
Copy link
Contributor Author

GlenTiki commented Feb 24, 2018

If TSC approved, I would be happy to open another PR straight up tearing it out.

@AndreasMadsen AndreasMadsen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 24, 2018
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Feb 24, 2018 •
edited
Loading

Should we remove LTTng integration or only document deprecate LTTng integration?

LTTng integration have been broken for two years, no one is apparently using it. If we are going to support static tracing points on linux, the vanilla linux tracing points should be used instead. LTTng supports those too, so very little will be lost and it allows for other tracing implementations too. The only advantages of LTTng is that is slightly more performant.

/cc @nodejs/tsc

@ChALkeR
Copy link
Member

ChALkeR commented Feb 24, 2018 •
edited
Loading

Some refs:

@GlenTiki
Copy link
Contributor Author

GlenTiki commented Feb 24, 2018

@ChALkeR And the commit that broke it: f1d2792

@ChALkeR
Copy link
Member

ChALkeR commented Feb 24, 2018

Previous fix attempt (stalled, closed): #9404.

mcollina
mcollina approved these changes Feb 24, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM.

I’m aso +1 in removing this completely.

TimothyGu
TimothyGu approved these changes Feb 24, 2018
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

+1 on removing all lttng code right now.

@GlenTiki GlenTiki mentioned this pull request Feb 24, 2018
Closed
2 tasks
cjihrig
cjihrig approved these changes Feb 25, 2018
View reviewed changes
doc/api/deprecations.md Outdated
Copy link
Contributor

@cjihrig cjihrig Feb 25, 2018

Choose a reason for hiding this comment

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

Shouldn't the 0101 here and below be XXXX until the PR lands?

Copy link
Member

@jasnell jasnell Feb 25, 2018

Choose a reason for hiding this comment

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

Yep. DEP00XX until it lands. The person landing should assign the code

doc/api/deprecations.md Outdated
Copy link
Contributor

@cjihrig cjihrig Feb 25, 2018

Choose a reason for hiding this comment

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

I think it would be better to just say the with-lttng compile time option is deprecated. If there is a recommended alternative, that could be listed here as well.

Copy link
Member

@jasnell jasnell Feb 25, 2018

Choose a reason for hiding this comment

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

Agreed

jasnell
jasnell approved these changes Feb 25, 2018
View reviewed changes
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.

Lgtm with the comments addressed

@GlenTiki
doc: deprecate lttng
c84a31d 
Building with-lttng has been broken for 2 years. We should
deprecate and remove this dead code.
GlenTiki added a commit to GlenTiki/node that referenced this pull request 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
@rvagg
Copy link
Member

rvagg commented Feb 26, 2018

Fine by me, I think nearForm were the last ppl I saw using/promoting it and they're in here with a +1 so I'm onboard with just ripping it out.

gibfahn
gibfahn approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

+1 on directly removing given the reasons and consensus in this thread.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

I generally prefer deprecating but given that it's been broken, I'm good with pulling it out.

@GlenTiki
Copy link
Contributor Author

GlenTiki commented Feb 26, 2018

Should this be closed in favour of #18982? Should that PR receive a deprecation PR too?

@mcollina
Copy link
Member

mcollina commented Feb 26, 2018

Yes, but the deprecation should be ported there too (I think).

GlenTiki added a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+4 more reviewers

@rvagg rvagg rvagg approved these changes

@evanlucas evanlucas evanlucas approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.