From 400914dbc268f90a20d6eb1265b0492f1690b270 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Thu, 5 Sep 2024 13:23:15 -0400 Subject: [PATCH 1/2] feat: Provided ability to disable instrumentation for a 3rd party package --- lib/config/build-instrumentation-config.js | 19 ++++ lib/config/default.js | 9 +- lib/shimmer.js | 6 +- test/lib/custom-assertions.js | 90 +++++++++++++++++++ .../build-instrumentation-config.test.js | 20 +++++ test/unit/config/config-defaults.test.js | 6 ++ test/unit/config/config-env.test.js | 14 +++ .../disabled-express.test.js | 56 ++++++++++++ .../disabled-ioredis.test.js | 78 ++++++++++++++++ .../disabled-instrumentation/newrelic.js | 24 +++++ .../disabled-instrumentation/package.json | 22 +++++ 11 files changed, 342 insertions(+), 2 deletions(-) create mode 100644 lib/config/build-instrumentation-config.js create mode 100644 test/unit/config/build-instrumentation-config.test.js create mode 100644 test/versioned/disabled-instrumentation/disabled-express.test.js create mode 100644 test/versioned/disabled-instrumentation/disabled-ioredis.test.js create mode 100644 test/versioned/disabled-instrumentation/newrelic.js create mode 100644 test/versioned/disabled-instrumentation/package.json diff --git a/lib/config/build-instrumentation-config.js b/lib/config/build-instrumentation-config.js new file mode 100644 index 0000000000..c4fdebfb55 --- /dev/null +++ b/lib/config/build-instrumentation-config.js @@ -0,0 +1,19 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' +const { boolean } = require('./formatters') +const instrumentedLibraries = require('../instrumentations')() +const pkgNames = Object.keys(instrumentedLibraries) + +/** + * Builds the stanza for config.instrumentation.* + * It defaults every library to true and assigns a boolean + * formatter for the environment variable conversion of the values + */ +module.exports = pkgNames.reduce((config, pkg) => { + config[pkg] = { enabled: { formatter: boolean, default: true } } + return config +}, {}) diff --git a/lib/config/default.js b/lib/config/default.js index dea31c3c2e..cfb59cae5a 100644 --- a/lib/config/default.js +++ b/lib/config/default.js @@ -7,6 +7,7 @@ const defaultConfig = module.exports const { array, int, float, boolean, object, objectList, allowList, regex } = require('./formatters') +const pkgInstrumentation = require('./build-instrumentation-config') /** * A function that returns the definition of the agent configuration @@ -1382,7 +1383,13 @@ defaultConfig.definition = () => ({ default: true } } - } + }, + /** + * Stanza that contains all keys to disable 3rd party package instrumentation(i.e. mongodb, pg, redis, etc) + * Note: Disabling a given 3rd party library may affect the instrumentation of 3rd party libraries used after + * the disabled library. + */ + instrumentation: pkgInstrumentation }) /** diff --git a/lib/shimmer.js b/lib/shimmer.js index deab878465..ac479ead09 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -286,7 +286,11 @@ const shimmer = (module.exports = { */ registerThirdPartyInstrumentation(agent) { for (const [moduleName, instrInfo] of Object.entries(INSTRUMENTATIONS)) { - if (instrInfo.module) { + if (agent.config.instrumentation?.[moduleName]?.enabled === false) { + logger.warn( + `Instrumentation for ${moduleName} has been disabled via 'config.instrumentation.${moduleName}.enabled. Not instrumenting package` + ) + } else if (instrInfo.module) { // Because external instrumentations can change independent of // the agent core, we don't want breakages in them to entirely // disable the agent. diff --git a/test/lib/custom-assertions.js b/test/lib/custom-assertions.js index edc0c94cee..1010722329 100644 --- a/test/lib/custom-assertions.js +++ b/test/lib/custom-assertions.js @@ -80,6 +80,95 @@ function compareSegments(parent, segments) { }) } +/** + * @param {TraceSegment} parent Parent segment + * @param {Array} expected Array of strings that represent segment names. + * If an item in the array is another array, it + * represents children of the previous item. + * @param {boolean} options.exact If true, then the expected segments must match + * exactly, including their position and children on all + * levels. When false, then only check that each child + * exists. + * @param {array} options.exclude Array of segment names that should be excluded from + * validation. This is useful, for example, when a + * segment may or may not be created by code that is not + * directly under test. Only used when `exact` is true. + */ +function assertSegments(parent, expected, options) { + let child + let childCount = 0 + + // rather default to what is more likely to fail than have a false test + let exact = true + if (options && options.exact === false) { + exact = options.exact + } else if (options === false) { + exact = false + } + + function getChildren(_parent) { + return _parent.children.filter(function (item) { + if (exact && options && options.exclude) { + return options.exclude.indexOf(item.name) === -1 + } + return true + }) + } + + const children = getChildren(parent) + if (exact) { + for (let i = 0; i < expected.length; ++i) { + const sequenceItem = expected[i] + + if (typeof sequenceItem === 'string') { + child = children[childCount++] + assert.equal( + child ? child.name : undefined, + sequenceItem, + 'segment "' + + parent.name + + '" should have child "' + + sequenceItem + + '" in position ' + + childCount + ) + + // If the next expected item is not array, then check that the current + // child has no children + if (!Array.isArray(expected[i + 1])) { + // var children = child.children + assert.ok( + getChildren(child).length === 0, + 'segment "' + child.name + '" should not have any children' + ) + } + } else if (typeof sequenceItem === 'object') { + assertSegments(child, sequenceItem, options) + } + } + + // check if correct number of children was found + assert.equal(children.length, childCount) + } else { + for (let i = 0; i < expected.length; i++) { + const sequenceItem = expected[i] + + if (typeof sequenceItem === 'string') { + // find corresponding child in parent + for (let j = 0; j < parent.children.length; j++) { + if (parent.children[j].name === sequenceItem) { + child = parent.children[j] + } + } + assert.ok(child, 'segment "' + parent.name + '" should have child "' + sequenceItem + '"') + if (typeof expected[i + 1] === 'object') { + assertSegments(child, expected[i + 1], exact) + } + } + } + } +} + /** * Like `tap.prototype.match`. Verifies that `actual` satisfies the shape * provided by `expected`. @@ -147,6 +236,7 @@ function match(actual, expected) { module.exports = { assertCLMAttrs, assertExactClmAttrs, + assertSegments, compareSegments, isNonWritable, match diff --git a/test/unit/config/build-instrumentation-config.test.js b/test/unit/config/build-instrumentation-config.test.js new file mode 100644 index 0000000000..70f0fbebf5 --- /dev/null +++ b/test/unit/config/build-instrumentation-config.test.js @@ -0,0 +1,20 @@ +/* + * Copyright 2022 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +const test = require('node:test') +const assert = require('node:assert') + +test('should default the instrumentation stanza', () => { + const { boolean } = require('../../../lib/config/formatters') + const pkgs = require('../../../lib/config/build-instrumentation-config') + const instrumentation = require('../../../lib/instrumentations')() + const pkgNames = Object.keys(instrumentation) + + pkgNames.forEach((pkg) => { + assert.deepEqual(pkgs[pkg], { enabled: { formatter: boolean, default: true } }) + }) +}) diff --git a/test/unit/config/config-defaults.test.js b/test/unit/config/config-defaults.test.js index a00f2bc162..acae9d6e2b 100644 --- a/test/unit/config/config-defaults.test.js +++ b/test/unit/config/config-defaults.test.js @@ -262,4 +262,10 @@ test('with default properties', async (t) => { assert.equal(configuration.ai_monitoring.enabled, false) assert.equal(configuration.ai_monitoring.streaming.enabled, true) }) + + await t.test('instrumentation defaults', () => { + assert.equal(configuration.instrumentation.express.enabled, true) + assert.equal(configuration.instrumentation['@prisma/client'].enabled, true) + assert.equal(configuration.instrumentation.npmlog.enabled, true) + }) }) diff --git a/test/unit/config/config-env.test.js b/test/unit/config/config-env.test.js index b1fd16b3c0..a526258b91 100644 --- a/test/unit/config/config-env.test.js +++ b/test/unit/config/config-env.test.js @@ -768,5 +768,19 @@ test('when overriding configuration values via environment variables', async (t) end() }) }) + + await t.test('should convert NEW_RELIC_INSTRUMENTATION* accordingly', (t, end) => { + const env = { + NEW_RELIC_INSTRUMENTATION_IOREDIS_ENABLED: 'false', + ['NEW_RELIC_INSTRUMENTATION_@GRPC/GRPC-JS_ENABLED']: 'false', + NEW_RELIC_INSTRUMENTATION_KNEX_ENABLED: 'false' + } + idempotentEnv(env, (config) => { + assert.equal(config.instrumentation.ioredis.enabled, false) + assert.equal(config.instrumentation['@grpc/grpc-js'].enabled, false) + assert.equal(config.instrumentation.knex.enabled, false) + end() + }) + }) } }) diff --git a/test/versioned/disabled-instrumentation/disabled-express.test.js b/test/versioned/disabled-instrumentation/disabled-express.test.js new file mode 100644 index 0000000000..83763685c4 --- /dev/null +++ b/test/versioned/disabled-instrumentation/disabled-express.test.js @@ -0,0 +1,56 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' +const assert = require('node:assert') +const test = require('node:test') +const helper = require('../../lib/agent_helper') +const http = require('http') +const params = require('../../lib/params') +const { assertSegments } = require('../../lib/custom-assertions') + +test('should still record child segments if express instrumentation is disabled', async (t) => { + const agent = helper.instrumentMockedAgent({ + instrumentation: { + express: { + enabled: false + } + } + }) + const express = require('express') + const app = express() + const Redis = require('ioredis') + const client = new Redis(params.redis_port, params.redis_host) + + app.get('/test-me', (_req, res) => { + client.get('foo', (err) => { + assert.equal(err, undefined) + res.end() + }) + }) + + const promise = new Promise((resolve) => { + agent.on('transactionFinished', (tx) => { + assert.equal(tx.name, 'WebTransaction/NormalizedUri/*', 'should not name transactions') + const rootSegment = tx.trace.root + const expectedSegments = ['WebTransaction/NormalizedUri/*', ['Datastore/operation/Redis/get']] + assertSegments(rootSegment, expectedSegments) + resolve() + }) + }) + + const server = app.listen(() => { + const { port } = server.address() + http.request({ port, path: '/test-me' }).end() + }) + + t.after(() => { + server.close() + client.disconnect() + helper.unloadAgent(agent) + }) + + await promise +}) diff --git a/test/versioned/disabled-instrumentation/disabled-ioredis.test.js b/test/versioned/disabled-instrumentation/disabled-ioredis.test.js new file mode 100644 index 0000000000..c433d08789 --- /dev/null +++ b/test/versioned/disabled-instrumentation/disabled-ioredis.test.js @@ -0,0 +1,78 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' +const assert = require('node:assert') +const test = require('node:test') +const helper = require('../../lib/agent_helper') +const params = require('../../lib/params') +const { assertSegments } = require('../../lib/custom-assertions') +const mongoCommon = require('../mongodb/common') + +test('Disabled PG scenarios', async (t) => { + t.beforeEach(async (ctx) => { + ctx.nr = {} + const agent = helper.instrumentMockedAgent({ + instrumentation: { + ioredis: { + enabled: false + } + } + }) + const Redis = require('ioredis') + const mongodb = require('mongodb') + const mongo = await mongoCommon.connect({ mongodb }) + const collection = mongo.db.collection('disabled-inst-test') + const redisClient = new Redis(params.redis_port, params.redis_host) + await redisClient.select(1) + ctx.nr.redisClient = redisClient + ctx.nr.agent = agent + ctx.nr.collection = collection + ctx.nr.db = mongo.db + ctx.nr.mongoClient = mongo.client + }) + + t.afterEach(async (ctx) => { + const { agent, redisClient, mongoClient, db } = ctx.nr + await mongoCommon.close(mongoClient, db) + redisClient.disconnect() + helper.unloadAgent(agent) + }) + + await t.test('should record child segments if pg is disabled and using promises', async (t) => { + const { agent, redisClient, collection } = t.nr + await helper.runInTransaction(agent, async (tx) => { + await redisClient.get('foo') + await collection.countDocuments() + await redisClient.get('bar') + tx.end() + assertSegments(tx.trace.root, [ + 'Datastore/statement/MongoDB/disabled-inst-test/aggregate', + 'Datastore/statement/MongoDB/disabled-inst-test/next' + ]) + }) + }) + + await t.test('should record child segments if pg is disabled and using callbacks', async (t) => { + const { agent, redisClient, collection } = t.nr + await helper.runInTransaction(agent, async (tx) => { + await new Promise((resolve) => { + redisClient.get('foo', async (err) => { + assert.equal(err, null) + await collection.countDocuments() + redisClient.get('bar', (innerErr) => { + tx.end() + assert.equal(innerErr, null) + assertSegments(tx.trace.root, [ + 'Datastore/statement/MongoDB/disabled-inst-test/aggregate', + 'Datastore/statement/MongoDB/disabled-inst-test/next' + ]) + resolve() + }) + }) + }) + }) + }) +}) diff --git a/test/versioned/disabled-instrumentation/newrelic.js b/test/versioned/disabled-instrumentation/newrelic.js new file mode 100644 index 0000000000..0caf680f7e --- /dev/null +++ b/test/versioned/disabled-instrumentation/newrelic.js @@ -0,0 +1,24 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +exports.config = { + app_name: ['My Application'], + license_key: 'license key here', + logging: { + level: 'trace' + }, + utilization: { + detect_aws: false, + detect_pcf: false, + detect_azure: false, + detect_gcp: false, + detect_docker: false + }, + transaction_tracer: { + enabled: true + } +} diff --git a/test/versioned/disabled-instrumentation/package.json b/test/versioned/disabled-instrumentation/package.json new file mode 100644 index 0000000000..251f84cf84 --- /dev/null +++ b/test/versioned/disabled-instrumentation/package.json @@ -0,0 +1,22 @@ +{ + "name": "disabled-instrumentation-tests", + "targets": [], + "version": "0.0.0", + "private": true, + "tests": [ + { + "engines": { + "node": ">=18" + }, + "dependencies": { + "express": "latest", + "ioredis": "latest", + "mongodb": "latest" + }, + "files": [ + "disabled-express.test.js", + "disabled-ioredis.test.js" + ] + } + ] +} From 6b7b9574a337793291b101d5f369c22d1c1a06b9 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Tue, 10 Sep 2024 14:47:31 -0400 Subject: [PATCH 2/2] chore: removed unnecessary commented out code --- test/lib/custom-assertions.js | 1 - test/lib/metrics_helper.js | 1 - 2 files changed, 2 deletions(-) diff --git a/test/lib/custom-assertions.js b/test/lib/custom-assertions.js index 1010722329..2b1b10515c 100644 --- a/test/lib/custom-assertions.js +++ b/test/lib/custom-assertions.js @@ -136,7 +136,6 @@ function assertSegments(parent, expected, options) { // If the next expected item is not array, then check that the current // child has no children if (!Array.isArray(expected[i + 1])) { - // var children = child.children assert.ok( getChildren(child).length === 0, 'segment "' + child.name + '" should not have any children' diff --git a/test/lib/metrics_helper.js b/test/lib/metrics_helper.js index 98fc078d89..b5e3cb1370 100644 --- a/test/lib/metrics_helper.js +++ b/test/lib/metrics_helper.js @@ -161,7 +161,6 @@ function assertSegments(parent, expected, options) { // If the next expected item is not array, then check that the current // child has no children if (!Array.isArray(expected[i + 1])) { - // var children = child.children this.ok( getChildren(child).length === 0, 'segment "' + child.name + '" should not have any children'