diff --git a/CHANGELOG.md b/CHANGELOG.md index 1431c5a519..452a10c763 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased +- Make instrumentation robust against https://github.com/montagejs/collections/issues/178. +- Make check for disabled instrumentation stricter (the keys provided in config.tracing.disabledTracers/INSTANA_DISABLED_TRACERS need to be an exact, case-insensitive match of the instrumentation file now). If you have used this configuration option and relied on the fact that this was a string.contains check previously, you might need to update your config. + ## 1.95.2 - [AWS Lambda] Fix: Add a connection timeout in addition to the read/idle timeout. diff --git a/packages/collector/test/tracing/common/app.js b/packages/collector/test/tracing/common/app.js index 3f83bb0d74..9a5f5ceae5 100644 --- a/packages/collector/test/tracing/common/app.js +++ b/packages/collector/test/tracing/common/app.js @@ -6,11 +6,32 @@ if (process.env.SERVICE_CONFIG) { config.serviceName = process.env.SERVICE_CONFIG; } -require('../../../')(config); - const logPrefix = `Server (${process.pid}):\t`; +if (process.env.SCREW_AROUND_WITH_UP_ARRAY_FIND) { + log('!Breaking Array.find on purpose!'); + // Yoinked from https://github.com/montagejs/collections/blob/v1.0.0/shim-array.js + // eslint-disable-next-line no-extend-native + Object.defineProperty(Array.prototype, 'find', { + value: function(value, equals) { + equals = equals || this.contentEquals || Object.equals; + for (var index = 0; index < this.length; index++) { + if (index in this && equals(this[index], value)) { + return index; + } + } + return -1; + }, + writable: true, + configurable: true, + enumerable: false + }); +} + +require('../../../')(config); + const http = require('http'); +const pino = require('pino')(); const port = process.env.APP_PORT || 3000; const app = new http.Server(); @@ -18,6 +39,9 @@ app.on('request', (req, res) => { if (process.env.WITH_STDOUT) { log(`${req.method} ${req.url}`); } + if (req.url.indexOf('with-log') >= 0) { + pino.error('This error message should be traced, unless the pino instrumentation is disabled.'); + } res.end(); }); diff --git a/packages/collector/test/tracing/common/controls.js b/packages/collector/test/tracing/common/controls.js deleted file mode 100644 index 97e81be66e..0000000000 --- a/packages/collector/test/tracing/common/controls.js +++ /dev/null @@ -1,12 +0,0 @@ -'use strict'; - -const path = require('path'); - -const AbstractControls = require('../AbstractControls'); - -const Controls = (module.exports = function Controls(opts) { - opts.appPath = path.join(__dirname, 'app.js'); - AbstractControls.call(this, opts); -}); - -Controls.prototype = Object.create(AbstractControls.prototype); diff --git a/packages/collector/test/tracing/common/test.js b/packages/collector/test/tracing/common/test.js index da8e3f44af..2b7751753c 100644 --- a/packages/collector/test/tracing/common/test.js +++ b/packages/collector/test/tracing/common/test.js @@ -7,8 +7,8 @@ const supportedVersion = require('@instana/core').tracing.supportedVersion; const config = require('../../../../core/test/config'); const delay = require('../../../../core/test/test_util/delay'); const testUtils = require('../../../../core/test/test_util'); +const ProcessControls = require('../ProcessControls'); -let Controls; const extendedTimeout = Math.max(config.getTestTimeout(), 10000); describe('tracing/common', function() { @@ -16,13 +16,13 @@ describe('tracing/common', function() { return; } - Controls = require('./controls'); describe('delay', function() { describe('with minimal delay', function() { this.timeout(extendedTimeout); const agentControls = setupAgentControls(); - const controls = new Controls({ + const controls = new ProcessControls({ agentControls, + dirname: __dirname, minimalDelay: 6000 }); registerDelayTest.call(this, agentControls, controls, true); @@ -31,7 +31,10 @@ describe('tracing/common', function() { describe('without minimal delay', function() { this.timeout(config.getTestTimeout()); const agentControls = setupAgentControls(); - const controls = new Controls({ agentControls }); + const controls = new ProcessControls({ + agentControls, + dirname: __dirname + }); registerDelayTest.call(this, agentControls, controls, false); }); @@ -76,7 +79,7 @@ describe('tracing/common', function() { expect(span.data.http.method).to.equal('GET'); expect(span.data.http.url).to.equal('/'); expect(span.data.http.status).to.equal(200); - expect(span.data.http.host).to.equal('127.0.0.1:3215'); + expect(span.data.http.host).to.equal('127.0.0.1:3216'); }), Math.max(extendedTimeout / 2, 10000) ) @@ -101,7 +104,7 @@ describe('tracing/common', function() { expect(span.data.http.method).to.equal('GET'); expect(span.data.http.url).to.equal('/'); expect(span.data.http.status).to.equal(200); - expect(span.data.http.host).to.equal('127.0.0.1:3215'); + expect(span.data.http.host).to.equal('127.0.0.1:3216'); }) ); } @@ -110,8 +113,9 @@ describe('tracing/common', function() { describe('service name', function() { describe('with env var', function() { const agentControls = setupAgentControls(); - const controls = new Controls({ + const controls = new ProcessControls({ agentControls, + dirname: __dirname, env: { INSTANA_SERVICE_NAME: 'much-custom-very-wow service' } @@ -121,8 +125,9 @@ describe('tracing/common', function() { describe('with config', function() { const agentControls = setupAgentControls(); - const controls = new Controls({ + const controls = new ProcessControls({ agentControls, + dirname: __dirname, env: { // this makes the app set the serviceName per config object SERVICE_CONFIG: 'much-custom-very-wow service' @@ -133,13 +138,19 @@ describe('tracing/common', function() { describe('with header when agent is configured to capture the header', function() { const agentControls = setupAgentControls(true); - const controls = new Controls({ agentControls }); + const controls = new ProcessControls({ + agentControls, + dirname: __dirname + }); registerServiceNameTest.call(this, agentControls, controls, 'X-Instana-Service header', true); }); describe('with header when agent is _not_ configured to capture the header', function() { const agentControls = setupAgentControls(false); - const controls = new Controls({ agentControls }); + const controls = new ProcessControls({ + agentControls, + dirname: __dirname + }); registerServiceNameTest.call(this, agentControls, controls, 'X-Instana-Service header', false); }); @@ -177,6 +188,73 @@ describe('tracing/common', function() { } }); + describe('disable individual tracers', function() { + this.timeout(config.getTestTimeout()); + + describe('disable an individual tracer', () => { + const agentControls = setupAgentControls(); + const controls = new ProcessControls({ + agentControls, + dirname: __dirname, + env: { + INSTANA_DISABLED_TRACERS: 'pino' + } + }); + controls.registerTestHooks(); + + it('can disable a single instrumentation', () => + controls + .sendRequest({ + path: '/with-log' + }) + .then(() => { + return testUtils.retry(() => + agentControls.getSpans().then(spans => { + expect(spans.length).to.equal(1); + expect(spans[0].n).to.equal('node.http.server'); + }) + ); + })); + }); + + describe('robustness against overriding Array.find', () => { + const agentControls = setupAgentControls(); + const controls = new ProcessControls({ + agentControls, + dirname: __dirname, + env: { + SCREW_AROUND_WITH_UP_ARRAY_FIND: 'sure why not?' + } + }); + controls.registerTestHooks(); + + // Story time: There is a package out there that overrides Array.find with different behaviour that, once upon a + // time, as a random side effect, disabled our auto tracing. This is a regression test to make sure we are now + // robust against that particular breakage. + // + // It is precisely this issue that broke the activation of individual instrumentations: + // https://github.com/montagejs/collections/issues/178 + // + // In particular, the monkey patched version of Array.find returns -1 if nothing is found, while the standard + // Array.find returns undefined. When checking if an array contains something with find, this reverses the logic + // of the check because -1 is truthy and undefined is falsy. + + it('messing up Array.find must not break tracing', () => + controls + .sendRequest({ + path: '/' + }) + .then(() => { + return testUtils.retry(() => + agentControls.getSpans().then(spans => { + expect(spans.length).to.equal(1); + expect(spans[0].n).to.equal('node.http.server'); + }) + ); + })); + }); + }); + function setupAgentControls(captureXInstanaServiceHeader) { const agentControls = require('../../apps/agentStubControls'); const agentConfig = captureXInstanaServiceHeader ? { extraHeaders: ['x-iNsTanA-sErViCe'] } : {}; diff --git a/packages/core/src/tracing/index.js b/packages/core/src/tracing/index.js index 8e26462341..798a93eca4 100644 --- a/packages/core/src/tracing/index.js +++ b/packages/core/src/tracing/index.js @@ -108,9 +108,11 @@ exports.activate = function() { if (automaticTracingEnabled) { instrumentations.forEach(function(instrumentationKey) { - var isDisabled = !!config.tracing.disabledTracers.find(function(disabledKey) { - return instrumentationKey.toLowerCase().indexOf(disabledKey) >= 0; - }); + var instrumentationName = /.\/instrumentation\/[^/]*\/(.*)/.exec(instrumentationKey)[1]; + var isDisabled = + config.tracing.disabledTracers.findIndex(function(disabledKey) { + return instrumentationName.toLowerCase() === disabledKey; + }) !== -1; if (!isDisabled) { instrumentationModules[instrumentationKey].activate(); }