fix(dependency): updated tar to 6.x in shared-metrics #415

kirrg001 · 2021-11-05T08:52:20Z

refs 75234

Tar 6 has already dropped Node support < 10
See https://github.com/npm/node-tar/blob/065e3850dfcfa439fd7d4bce0407ba616f85f576/package.json#L48

We currently receive security updates in tar 5.x, but that can stop quickly, because Node 8 support has stopped in 2020 already.

We should just update tar to 6.x in shared-metrics because:

Node 8 (and lower) is officially not supported for two more packages:

semver 7.x
[email protected]

(and there are probably tons of more, which just don't make use of the engines keyword)

If you set npm config set engine-strict true, you will see that you cannot install collector
with Node 8 (without the tar update). That menas, it is already a problem.

tar is used in nativeModuleRetry#copyPrecompiled (

nodejs/packages/shared-metrics/src/util/nativeModuleRetry.js

Line 75 in dc2160b

copyPrecompiled(opts, loaderEmitter, success => {

), but this piece of code is not covered in our tests.

Where is nativeModuleRetry script used? Only in shared-metrics.

in gc.js
in lubuv.js

Options:

~~Add tests for copyPrecompiled to ensure that Node 8 still works~~ There is a test!
Disallow copyPrecompiled for Node < 10
Do nothing, because the execution of tar.x is catched in the codebase and the user won't experience anything bad. But (!) if anything goes wrong with requiring tar in Node 8 or before tar.x is executed and it's not catched, we can bring down the customers application. We'd need to try/catch the require statement + tar.x

tar is not a binary installation, that's why I'd not add it as optional dependency, because it is not possible to fail except the customer uses engine-strict.

Other usages of tar:

in tests
in dummy apps
in docker examples
in subdependencies of dev dependencies
- these should be updated in a separate and general version update

TODO

ci passes
~~[ ] consider adding a test!~~ there is a test!
Option 2

kirrg001 · 2021-11-05T08:59:19Z

@basti1302 Could you please give this a pre review? Let me know if you agree or disagree :) Thanks!

basti1302

The general direction looks good in my opinion. I would maybe lean towards disabling the copy-precompiled feature outright in Node.js < 10, but that's totally up for discussion.

I'll will follow up with a more detailed answer to the questions and aspects you brought up here.

packages/shared-metrics/src/util/nativeModuleRetry.js

basti1302 · 2021-11-05T13:42:11Z

We should just update tar to 6.x in shared-metrics because:

Node 8 (and lower) is officially not supported for two more packages:

semver 7.x

[email protected]

I would leave these other two packages out of the picture for this PR. We use semver much more widely, so I think we cannot update this to a newer version right now.

By the way, when we update node-tar, we might want to remove it from https://github.com/instana/nodejs/blob/main/packages/shared-metrics/.ncurc.json as well.

If you set npm config set engine-strict true, you will see that you cannot install collector with Node 8 (without the tar update). That menas, it is already a problem.

Interesting, I didn't know that. But you say it already fails to install even before updating to tar@6? That's good, so that update does not create a new problem then. Besides, people running on EOL Node.js versions can then just not make use of the engine-strict option, that seems fair enough.

tar is used in nativeModuleRetry#copyPrecompiled (

nodejs/packages/shared-metrics/src/util/nativeModuleRetry.js

Line 75 in dc2160b

copyPrecompiled(opts, loaderEmitter, success => {

), but this piece of code is not covered in our tests.

I beg to differ :-)

Unfortunately, the tests covering this are not straight forward to find. They are in the collector package (https://github.com/instana/nodejs/tree/main/packages/collector/test/nativeModuleRetry) because they require the whole integration test shebang with the agent stub etc. When I wrote those tests, that test infrastructure was only available in the collector package :-/. Actually, I think, we should be able to move those tests to shared-metrics now, because by now there is a precedent for having an integration test with the agent stub in the shared-metrics package -> https://github.com/instana/nodejs/blob/main/packages/shared-metrics/test/dependencies/test.js (this did not exist when the native retry feature and tests were added).

Where is nativeModuleRetry script used? Only in shared-metrics.

in gc.js

in lubuv.js

That's correct, these are the only two usages.

Add tests for copyPrecompiled to ensure that Node 8 still works

Those exist. 👍

Disallow copyPrecompiled for Node < 10

That's what I had in mind originally, I think it is a bit more straightforward then (3.).

Do nothing, because the execution of tar.x is catched in the codebase and the user won't experience anything bad. But (!) if anything goes wrong with requiring tar in Node 8 or before tar.x is executed and it's not catched, we can bring down the customers application. We'd need to try/catch the require statement + tar.x
I'd go with 3) and just add more catches

Also possible, assuming the catch will also catch outright "syntax errors" (because tar@6 uses syntax constructs that
are not available in Node.js 8/10) when requiring it.

tar is not a binary installation, that's why I'd not add it as optional dependency, because it is not possible to fail except the customer uses engine-strict.

I agree with that, tar should not be an optional dependency. 👍

Other usages of tar:

in tests

in dummy apps

in docker examples

in subdependencies of dev dependencies

these should be updated in a separate and general version update

Do we explicitly require the tar package in any of our tests? If so, we might need to provide a different version of node-tar in our dev-dependencies, maybe in the root package, in case we want to keep running the tests that depend on it in older Node.js versions. To be honest, I am not sure about those other usages right now.

basti1302 · 2021-11-05T13:45:41Z

ci passes

Hu, that's interesting. I would have expected https://github.com/instana/nodejs/blob/main/packages/collector/test/nativeModuleRetry/test.js to fail for Node.js 8 then, but it didn't? 🤔 🤔

kirrg001 · 2021-11-05T14:11:04Z

ci passes

Hu, that's interesting. I would have expected https://github.com/instana/nodejs/blob/main/packages/collector/test/nativeModuleRetry/test.js to fail for Node.js 8 then, but it didn't? 🤔 🤔

~~I ran the tests and the fn copyPrecompiled was not called in any of the tests. Or maybe I have missed something 🤔~~ see next comments

kirrg001 · 2021-11-05T14:17:47Z

That's what I had in mind originally, I think it is a bit more straightforward then (3.).

I have no strong opinion if 2 or 3. We can go with 3 :) Its defo an easier check than the try catch statements. My thinking was to try our best to still offer the feature to as many people we can.

kirrg001 · 2021-11-05T16:28:36Z

Node 8 (and lower) is officially not supported for two more packages:
semver 7.x
[email protected]
I would leave these other two packages out of the picture for this PR. We use semver much more widely, so I think we cannot update this to a newer version right now.

Definitely :) I was summing up that these two packages will make the installation of the collector fail if engine-strict is enabled. And tar will bring it to fail too!

I would have expected https://github.com/instana/nodejs/blob/main/packages/collector/test/nativeModuleRetry/test.js to fail for Node.js 8 then, but it didn't

Oh. I haven't seen this test. So yeah there is a test and copyPrecompiled is covered. That is good! Thanks for the hint! 🔥
I have tested Node 8 with tar 6 in a script in the beginning and it was working fine. That's why I thought option 2) is the best approach.

Do we explicitly require the tar package in any of our tests?

No. I'd update our dev dependencies in a separate PR as part of the stability roadmap. Ok for you?

kirrg001 · 2021-11-08T09:25:46Z

Discussed: We go with option 2!

basti1302 · 2021-11-08T09:27:47Z

Do we explicitly require the tar package in any of our tests?

No. I'd update our dev dependencies in a separate PR as part of the stability roadmap. Ok for you?

Sure, absolutely 👍 I was just curious.

kirrg001 · 2021-11-10T10:29:15Z

packages/shared-metrics/src/util/nativeModuleRetry.js

@@ -11,8 +11,8 @@ const EventEmitter = require('events');
 const copy = require('recursive-copy');
 const fs = require('fs');
 const os = require('os');
+const semver = require('semver');


FYI

semver@7 does not support Node < 10 anymore.
But I ignored it for this PR because we already require semver@7 in different places (production code).
I'll check if we should protect the code against semver@7 and [email protected] in a different PR.

Good find! The situation seems to be a bit weird with semver.

AFAICT, the node-semver changelog did not call out explicitly which Node.js versions they dropped with 7.0.0, it just said "Drop support for very old node versions, use const/let, => functions, and classes." -- all of which are supported in Node.js 6, but not in Node.js 4. Then there is this discussion: npm/node-semver@d61f828#r36469180, around setting the minimum version to 10 in package.json/engines. :-/

Looks like the current code is working on >= Node.js 6 for now, but we should definitely do something about it and not rely on that.

But let's discuss this offline, outside of this PR. 👍

kirrg001 · 2021-11-10T10:33:05Z

packages/shared-metrics/src/libuv.js

@@ -47,5 +47,7 @@ function sense() {
    stats.statsSupported = true;
    return stats;
  }
-  return {};
+  return {
+    statsSupported: false


FYI I have added consistency changes for libuv and gc to have the statsSupported key always available in the metrics result. Before these optimisations the key did not exist for some cases.

Especially in this line, I tried to read the actual exports.currentPayload.statsSupported value, which is set on failed event, but it is impossible to access it without refactoring the code.

The decision to not transmit statsSupported = false might have been deliberate back in the days (to save a few bytes on the wire). But I agree, it is probably not worth it and having the value always present as a boolean makes sense.

basti1302

LGTM

basti1302 · 2021-11-10T12:52:13Z

packages/shared-metrics/src/util/nativeModuleRetry.js

@@ -11,8 +11,8 @@ const EventEmitter = require('events');
 const copy = require('recursive-copy');
 const fs = require('fs');
 const os = require('os');
+const semver = require('semver');


Good find! The situation seems to be a bit weird with semver.

AFAICT, the node-semver changelog did not call out explicitly which Node.js versions they dropped with 7.0.0, it just said "Drop support for very old node versions, use const/let, => functions, and classes." -- all of which are supported in Node.js 6, but not in Node.js 4. Then there is this discussion: npm/node-semver@d61f828#r36469180, around setting the minimum version to 10 in package.json/engines. :-/

Looks like the current code is working on >= Node.js 6 for now, but we should definitely do something about it and not rely on that.

But let's discuss this offline, outside of this PR. 👍

basti1302 · 2021-11-10T13:11:14Z

packages/shared-metrics/src/libuv.js

@@ -47,5 +47,7 @@ function sense() {
    stats.statsSupported = true;
    return stats;
  }
-  return {};
+  return {
+    statsSupported: false


The decision to not transmit statsSupported = false might have been deliberate back in the days (to save a few bytes on the wire). But I agree, it is probably not worth it and having the value always present as a boolean makes sense.

refs 75234 Tar 6 has already dropped Node support < 10 See https://github.com/npm/node-tar/blob/065e3850dfcfa439fd7d4bce0407ba616f85f576/package.json#L48 We currently receive security updates in tar 5.x, but that can stop quickly, because Node 8 support has stopped in 2020 already. We should just update tar to 6.x in shared-metrics because: Node 8 (and lower) is officially not supported for two more packages: - semver 7.x - [email protected] (and there are probably tons of more, which just don't make use of the engines keyword) If you set `npm config set engine-strict true`, you will see that you cannot install collector with Node 8 (without the tar update). That menas, it is already a problem. `tar` is used in nativeModuleRetry#copyPrecompiled (https://github.com/instana/nodejs/blob/dc2160bddab5a25d0b6bab25c5d10bee8b5f1bd5/packages/shared-metrics/src/util/nativeModuleRetry.js#L75), but this piece of code is not covered in our tests. Where is nativeModuleRetry script used? Only in shared-metrics. - in gc.js - in lubuv.js Options: 1) ~Add tests for copyPrecompiled to ensure that Node 8 still works~ There is a test! 2) Disallow copyPrecompiled for Node < 10 3) Do nothing, because the execution of `tar.x` is catched in the codebase and the user won't experience anything bad. But (!) if anything goes wrong with requiring tar in Node 8 or before `tar.x` is executed and it's not catched, we can bring down the customers application. We'd need to try/catch the require statement + tar.x ---- Other usages of `tar`: - in tests - in dummy apps - in docker examples - in subdependencies of dev dependencies - these should be updated in a separate and general version update We decided to go with 2).

refs 75234 Tar 6 has already dropped Node support < 10 See https://github.com/npm/node-tar/blob/065e3850dfcfa439fd7d4bce0407ba616f85f576/package.json, line 48. We currently receive security updates in tar 5.x, but that can stop quickly, because Node 8 support has stopped in 2020 already. We should just update tar to 6.x in shared-metrics because: Node 8 (and lower) is officially not supported for two more packages: - semver 7.x - [email protected] (and there are probably tons of more, which just don't make use of the engines keyword) If you set `npm config set engine-strict true`, you will see that you cannot install collector with Node 8 (without the tar update). That menas, it is already a problem. `tar` is used in nativeModuleRetry#copyPrecompiled (https://github.com/instana/nodejs/blob/dc2160bddab5a25d0b6bab25c5d10bee8b5f1bd5/packages/shared-metrics/src/util/nativeModuleRetry.js, line 75), but this piece of code is not covered in our tests. Where is nativeModuleRetry script used? Only in shared-metrics. - in gc.js - in lubuv.js Options: 1) ~Add tests for copyPrecompiled to ensure that Node 8 still works~ There is a test! 2) Disallow copyPrecompiled for Node < 10 3) Do nothing, because the execution of `tar.x` is catched in the codebase and the user won't experience anything bad. But (!) if anything goes wrong with requiring tar in Node 8 or before `tar.x` is executed and it's not catched, we can bring down the customers application. We'd need to try/catch the require statement + tar.x ---- Other usages of `tar`: - in tests - in dummy apps - in docker examples - in subdependencies of dev dependencies - these should be updated in a separate and general version update We decided to go with 2). Co-authored-by: kirrg001 <[email protected]>

kirrg001 added the WIP label Nov 5, 2021

basti1302 reviewed Nov 5, 2021

View reviewed changes

packages/shared-metrics/src/util/nativeModuleRetry.js Outdated Show resolved Hide resolved

packages/shared-metrics/src/util/nativeModuleRetry.js Outdated Show resolved Hide resolved

packages/shared-metrics/src/util/nativeModuleRetry.js Outdated Show resolved Hide resolved

kirrg001 force-pushed the update/tar6 branch 7 times, most recently from 9a9e6c8 to adcaeeb Compare November 10, 2021 10:26

kirrg001 commented Nov 10, 2021

View reviewed changes

kirrg001 changed the title ~~[WIP] fix(dependency): updated tar to 6.x in shared-metrics~~ fix(dependency): updated tar to 6.x in shared-metrics Nov 10, 2021

kirrg001 commented Nov 10, 2021

View reviewed changes

kirrg001 requested a review from basti1302 November 10, 2021 12:12

kirrg001 added review and removed WIP labels Nov 10, 2021

basti1302 approved these changes Nov 10, 2021

View reviewed changes

kirrg001 force-pushed the update/tar6 branch from adcaeeb to 9d88701 Compare November 10, 2021 13:33

kirrg001 merged commit c4c8367 into main Nov 10, 2021

kirrg001 deleted the update/tar6 branch November 10, 2021 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dependency): updated tar to 6.x in shared-metrics #415

fix(dependency): updated tar to 6.x in shared-metrics #415

kirrg001 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 5, 2021

basti1302 left a comment

basti1302 commented Nov 5, 2021 •

edited

Loading

basti1302 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 5, 2021

kirrg001 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 8, 2021

basti1302 commented Nov 8, 2021

kirrg001 Nov 10, 2021

basti1302 Nov 10, 2021

kirrg001 Nov 10, 2021

basti1302 Nov 10, 2021

basti1302 left a comment

basti1302 Nov 10, 2021

basti1302 Nov 10, 2021

fix(dependency): updated tar to 6.x in shared-metrics #415

fix(dependency): updated tar to 6.x in shared-metrics #415

Conversation

kirrg001 commented Nov 5, 2021 • edited Loading

TODO

kirrg001 commented Nov 5, 2021

basti1302 left a comment

Choose a reason for hiding this comment

basti1302 commented Nov 5, 2021 • edited Loading

basti1302 commented Nov 5, 2021 • edited Loading

kirrg001 commented Nov 5, 2021 • edited Loading

kirrg001 commented Nov 5, 2021

kirrg001 commented Nov 5, 2021 • edited Loading

kirrg001 commented Nov 8, 2021

basti1302 commented Nov 8, 2021

kirrg001 Nov 10, 2021

Choose a reason for hiding this comment

basti1302 Nov 10, 2021

Choose a reason for hiding this comment

kirrg001 Nov 10, 2021

Choose a reason for hiding this comment

basti1302 Nov 10, 2021

Choose a reason for hiding this comment

basti1302 left a comment

Choose a reason for hiding this comment

basti1302 Nov 10, 2021

Choose a reason for hiding this comment

basti1302 Nov 10, 2021

Choose a reason for hiding this comment

kirrg001 commented Nov 5, 2021 •

edited

Loading

basti1302 commented Nov 5, 2021 •

edited

Loading

basti1302 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 5, 2021 •

edited

Loading

kirrg001 commented Nov 5, 2021 •

edited

Loading