Skip to content

Commit

Permalink
fix(dependency): updated tar to 6.x in shared-metrics (#415)
Browse files Browse the repository at this point in the history
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).

Co-authored-by: kirrg001 <[email protected]>
  • Loading branch information
kirrg001 and kirrg001 authored Nov 10, 2021
1 parent e29e268 commit c4c8367
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 47 deletions.
31 changes: 21 additions & 10 deletions packages/collector/test/nativeModuleRetry/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const async = require('async');
const { expect } = require('chai');
const semver = require('semver');
const fs = require('fs');
const os = require('os');
const path = require('path');
Expand Down Expand Up @@ -38,7 +39,7 @@ describe('retry loading native addons', function () {
let foundAtLeastOneUnsupported;
for (let i = 0; i < allMetrics.length; i++) {
if (allMetrics[i].data.libuv) {
expect(allMetrics[i].data.libuv.statsSupported).to.not.exist;
expect(allMetrics[i].data.libuv.statsSupported).to.be.false;
foundAtLeastOneUnsupported = true;
break;
}
Expand All @@ -50,11 +51,16 @@ describe('retry loading native addons', function () {
const libuv = aggregated.libuv;
expect(libuv).to.exist;
expect(libuv).to.be.an('object');
expect(libuv.statsSupported).to.be.true;
expect(libuv.min).to.be.a('number');
expect(libuv.max).to.be.a('number');
expect(libuv.sum).to.be.a('number');
expect(libuv.lag).to.be.a('number');

if (semver.lt(process.version, '10.0.0')) {
expect(libuv.statsSupported).to.be.false;
} else {
expect(libuv.statsSupported).to.be.true;
expect(libuv.min).to.be.a('number');
expect(libuv.max).to.be.a('number');
expect(libuv.sum).to.be.a('number');
expect(libuv.lag).to.be.a('number');
}
}
},
{
Expand All @@ -67,7 +73,7 @@ describe('retry loading native addons', function () {
let foundAtLeastOneUnsupported;
for (let i = 0; i < allMetrics.length; i++) {
if (allMetrics[i].data.libuv) {
expect(allMetrics[i].data.gc.statsSupported).to.not.exist;
expect(allMetrics[i].data.gc.statsSupported).to.be.false;
foundAtLeastOneUnsupported = true;
break;
}
Expand All @@ -79,9 +85,14 @@ describe('retry loading native addons', function () {
const gc = aggregated.gc;
expect(gc).to.exist;
expect(gc).to.be.an('object');
expect(gc.statsSupported).to.be.true;
expect(gc.minorGcs).to.exist;
expect(gc.majorGcs).to.exist;

if (semver.lt(process.version, '10.0.0')) {
expect(gc.statsSupported).to.be.false;
} else {
expect(gc.statsSupported).to.be.true;
expect(gc.minorGcs).to.exist;
expect(gc.majorGcs).to.exist;
}
}
}
];
Expand Down
60 changes: 31 additions & 29 deletions packages/shared-metrics/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/shared-metrics/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@
"detect-libc": "^1.0.3",
"event-loop-lag": "^1.4.0",
"recursive-copy": "^2.0.13",
"tar": "^5.0.11"
"semver": "^7.3.5",
"tar": "^6.1.11"
},
"devDependencies": {
"@types/tar": "^4.0.5",
Expand Down
3 changes: 2 additions & 1 deletion packages/shared-metrics/src/gc.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ exports.currentPayload = {
majorGcs: 0,
incrementalMarkings: 0,
weakCallbackProcessing: 0,
gcPause: 0
gcPause: 0,
statsSupported: false
};

exports.activate = function activate() {
Expand Down
4 changes: 3 additions & 1 deletion packages/shared-metrics/src/libuv.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,7 @@ function sense() {
stats.statsSupported = true;
return stats;
}
return {};
return {
statsSupported: false
};
}
24 changes: 19 additions & 5 deletions packages/shared-metrics/src/util/nativeModuleRetry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
const path = require('path');
const tar = require('tar');
const detectLibc = require('detect-libc');

/**
Expand Down Expand Up @@ -171,6 +171,20 @@ function copyPrecompiled(opts, loaderEmitter, callback) {

logger.info(`Found a precompiled version for ${opts.nativeModuleName} ${label}, unpacking.`);

/**
* tar@6 has dropped support for Node < 10
* It might work to require tar@6 or to execute commands with tar@6 and Node < 10,
* but we don't want to risk that a customers application fails - especially if tar@6 adds
* breaking changes. We decided to disallow this feature.
*/
if (semver.lt(process.version, '10.0.0')) {
logger.info(`Skipped copying precompiled version for ${opts.nativeModuleName} ${label} with Node < 10.`);
callback(false);
return;
}

const tar = require('tar');

tar
.x({
cwd: os.tmpdir(),
Expand Down Expand Up @@ -203,10 +217,10 @@ function copyPrecompiled(opts, loaderEmitter, callback) {
// is, node_modules/${opts.nativeModuleName}). Node.js' module loading infrastructure
// (lib/internal/modules/cjs/loader.js and lib/internal/modules/package_json_reader.js) have built-in
// caching on multiple levels (for example, package.json locations and package.json contents). If Node.js
// has tried unsuccessfully to load a module or read a package.json from a particular path, it will remember
// and not try to load anything from that path again (a `false` will be put into the cache for that cache
// key). Instead, we force a new path, by adding precompiled to the module path and use the absolute path to
// the module to load it.
// has tried unsuccessfully to load a module or read a package.json from a particular path,
// it will remember and not try to load anything from that path again (a `false` will be
// put into the cache for that cache key). Instead, we force a new path, by adding precompiled
// to the module path and use the absolute path to the module to load it.
opts.loadFrom = targetDir;
callback(true);
}
Expand Down

0 comments on commit c4c8367

Please sign in to comment.