Skip to content

Commit

Permalink
fix(tracing): make activation robust against exotic Array.find override
Browse files Browse the repository at this point in the history 
- Make instrumentation robust against montagejs/collections#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.
  • Loading branch information
@basti1302
basti1302 committed Apr 24, 2020
1 parent a60746d commit 145f0af
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 27 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
28 changes: 26 additions & 2 deletions packages/collector/test/tracing/common/app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down
12 changes: 0 additions & 12 deletions packages/collector/test/tracing/common/controls.js
View file

This file was deleted.

98 changes: 88 additions & 10 deletions packages/collector/test/tracing/common/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
});

Expand Down Expand Up @@ -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)
)
Expand All @@ -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');
})
);
}
Expand All @@ -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'
}
Expand All @@ -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'
Expand All @@ -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);
});

Expand Down Expand Up @@ -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'] } : {};
Expand Down
8 changes: 5 additions & 3 deletions packages/core/src/tracing/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down

0 comments on commit 145f0af

Please sign in to comment.