doc: deprecate lttng #18975

GlenTiki · 2018-02-24T12:05:17Z

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

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

AndreasMadsen · 2018-02-24T12:26:27Z

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 · 2018-02-24T12:35:39Z

@AndreasMadsen I've changed it to Documentation only.

GlenTiki · 2018-02-24T12:36:39Z

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

AndreasMadsen · 2018-02-24T13:51:22Z

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 · 2018-02-24T14:56:16Z

Some refs:

Deprecating lttng tracepoints. (and add alternative for linux if wanted) #18971 is the issue for this PR.
Introduced in Tracing: Add lttng support for tracing on Linux. #702.
src: fix build with --with-lttng #18945 fixes build with lttng and was filed 2 days ago.

GlenTiki · 2018-02-24T15:03:42Z

@ChALkeR And the commit that broke it: f1d2792

ChALkeR · 2018-02-24T15:34:45Z

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

mcollina

LGTM.

I’m aso +1 in removing this completely.

TimothyGu

+1 on removing all lttng code right now.

cjihrig · 2018-02-25T14:48:19Z

doc/api/deprecations.md

@@ -914,6 +914,14 @@ Type: Runtime

 This was never a documented feature.

+<a id="DEP0101"></a>


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

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

cjihrig · 2018-02-25T14:52:01Z

doc/api/deprecations.md

+
+Type: Documentation-only
+
+`with-lttng` has been an opt-in compile time option that was broken when trying to compile


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.

jasnell

Lgtm with the comments addressed

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

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 · 2018-02-26T00:22:00Z

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

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

jasnell · 2018-02-26T13:31:11Z

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

GlenTiki · 2018-02-26T15:17:51Z

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

mcollina · 2018-02-26T16:53:52Z

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

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #18982 Refs: #18971 Refs: #18975 Refs: #18945 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#18982 Refs: nodejs#18971 Refs: nodejs#18975 Refs: nodejs#18945 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 24, 2018

GlenTiki mentioned this pull request Feb 24, 2018

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

Closed

GlenTiki force-pushed the deprecate-lttng branch from bf74d8f to df5df4b Compare February 24, 2018 12:35

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

mcollina approved these changes Feb 24, 2018

View reviewed changes

TimothyGu approved these changes Feb 24, 2018

View reviewed changes

GlenTiki mentioned this pull request Feb 24, 2018

src: Remove lttng support. #18982

Closed

2 tasks

cjihrig approved these changes Feb 25, 2018

View reviewed changes

jasnell approved these changes Feb 25, 2018

View reviewed changes

doc: deprecate lttng

c84a31d

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

GlenTiki force-pushed the deprecate-lttng branch from df5df4b to c84a31d Compare February 25, 2018 20:24

rvagg approved these changes Feb 26, 2018

View reviewed changes

gibfahn approved these changes Feb 26, 2018

View reviewed changes

evanlucas approved these changes Feb 26, 2018

View reviewed changes

AndreasMadsen closed this Feb 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: deprecate lttng #18975

doc: deprecate lttng #18975

GlenTiki commented Feb 24, 2018

AndreasMadsen commented Feb 24, 2018 •

edited

Loading

GlenTiki commented Feb 24, 2018

GlenTiki commented Feb 24, 2018

AndreasMadsen commented Feb 24, 2018 •

edited

Loading

ChALkeR commented Feb 24, 2018 •

edited

Loading

GlenTiki commented Feb 24, 2018

ChALkeR commented Feb 24, 2018

mcollina left a comment

TimothyGu left a comment

cjihrig Feb 25, 2018

jasnell Feb 25, 2018

cjihrig Feb 25, 2018

jasnell Feb 25, 2018

jasnell left a comment

rvagg commented Feb 26, 2018

gibfahn left a comment

jasnell commented Feb 26, 2018

GlenTiki commented Feb 26, 2018

mcollina commented Feb 26, 2018

		@@ -914,6 +914,14 @@ Type: Runtime

		This was never a documented feature.

		<a id="DEP0101"></a>


		Type: Documentation-only

		`with-lttng` has been an opt-in compile time option that was broken when trying to compile

doc: deprecate lttng #18975

doc: deprecate lttng #18975

Conversation

GlenTiki commented Feb 24, 2018

Checklist

Affected core subsystem(s)

AndreasMadsen commented Feb 24, 2018 • edited Loading

GlenTiki commented Feb 24, 2018

GlenTiki commented Feb 24, 2018

AndreasMadsen commented Feb 24, 2018 • edited Loading

ChALkeR commented Feb 24, 2018 • edited Loading

GlenTiki commented Feb 24, 2018

ChALkeR commented Feb 24, 2018

mcollina left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

cjihrig Feb 25, 2018

Choose a reason for hiding this comment

jasnell Feb 25, 2018

Choose a reason for hiding this comment

cjihrig Feb 25, 2018

Choose a reason for hiding this comment

jasnell Feb 25, 2018

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

rvagg commented Feb 26, 2018

gibfahn left a comment

Choose a reason for hiding this comment

jasnell commented Feb 26, 2018

GlenTiki commented Feb 26, 2018

mcollina commented Feb 26, 2018

AndreasMadsen commented Feb 24, 2018 •

edited

Loading

AndreasMadsen commented Feb 24, 2018 •

edited

Loading

ChALkeR commented Feb 24, 2018 •

edited

Loading