fix(tracing): make activation robust against exotic Array.find override

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.
 

packages/collector/test/tracing/common/app.js

@@ -6,18 +6,42 @@ 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();
 
 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();
 });
 

packages/collector/test/tracing/common/test.js

@@ -7,22 +7,22 @@ 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() {
   if (!supportedVersion(process.versions.node)) {
     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'] } : {};

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();
         }