From af5bcf633b876339c0126ed84454450387f93db6 Mon Sep 17 00:00:00 2001 From: Sumit Suthar Date: Fri, 20 Sep 2024 16:30:31 +0530 Subject: [PATCH] Revert "feat: Provided ability to disable instrumentation for a 3rd party package (#2551)" This reverts commit 7e467b8482331f607ee6a785bc7198846626c6ab. --- lib/config/build-instrumentation-config.js | 19 ---- lib/config/default.js | 9 +- lib/shimmer.js | 6 +- test/lib/custom-assertions.js | 89 ------------------- test/lib/metrics_helper.js | 1 + .../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 ----- 12 files changed, 3 insertions(+), 341 deletions(-) delete mode 100644 lib/config/build-instrumentation-config.js delete mode 100644 test/unit/config/build-instrumentation-config.test.js delete mode 100644 test/versioned/disabled-instrumentation/disabled-express.test.js delete mode 100644 test/versioned/disabled-instrumentation/disabled-ioredis.test.js delete mode 100644 test/versioned/disabled-instrumentation/newrelic.js delete mode 100644 test/versioned/disabled-instrumentation/package.json diff --git a/lib/config/build-instrumentation-config.js b/lib/config/build-instrumentation-config.js deleted file mode 100644 index c4fdebfb55..0000000000 --- a/lib/config/build-instrumentation-config.js +++ /dev/null @@ -1,19 +0,0 @@ -/* - * 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 cfb59cae5a..dea31c3c2e 100644 --- a/lib/config/default.js +++ b/lib/config/default.js @@ -7,7 +7,6 @@ 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 @@ -1383,13 +1382,7 @@ 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 ac479ead09..deab878465 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -286,11 +286,7 @@ const shimmer = (module.exports = { */ registerThirdPartyInstrumentation(agent) { for (const [moduleName, instrInfo] of Object.entries(INSTRUMENTATIONS)) { - 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) { + 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 2b1b10515c..edc0c94cee 100644 --- a/test/lib/custom-assertions.js +++ b/test/lib/custom-assertions.js @@ -80,94 +80,6 @@ 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])) { - 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`. @@ -235,7 +147,6 @@ function match(actual, expected) { module.exports = { assertCLMAttrs, assertExactClmAttrs, - assertSegments, compareSegments, isNonWritable, match diff --git a/test/lib/metrics_helper.js b/test/lib/metrics_helper.js index b5e3cb1370..98fc078d89 100644 --- a/test/lib/metrics_helper.js +++ b/test/lib/metrics_helper.js @@ -161,6 +161,7 @@ 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' diff --git a/test/unit/config/build-instrumentation-config.test.js b/test/unit/config/build-instrumentation-config.test.js deleted file mode 100644 index 70f0fbebf5..0000000000 --- a/test/unit/config/build-instrumentation-config.test.js +++ /dev/null @@ -1,20 +0,0 @@ -/* - * 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 acae9d6e2b..a00f2bc162 100644 --- a/test/unit/config/config-defaults.test.js +++ b/test/unit/config/config-defaults.test.js @@ -262,10 +262,4 @@ 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 a526258b91..b1fd16b3c0 100644 --- a/test/unit/config/config-env.test.js +++ b/test/unit/config/config-env.test.js @@ -768,19 +768,5 @@ 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 deleted file mode 100644 index 83763685c4..0000000000 --- a/test/versioned/disabled-instrumentation/disabled-express.test.js +++ /dev/null @@ -1,56 +0,0 @@ -/* - * 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 deleted file mode 100644 index c433d08789..0000000000 --- a/test/versioned/disabled-instrumentation/disabled-ioredis.test.js +++ /dev/null @@ -1,78 +0,0 @@ -/* - * 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 deleted file mode 100644 index 0caf680f7e..0000000000 --- a/test/versioned/disabled-instrumentation/newrelic.js +++ /dev/null @@ -1,24 +0,0 @@ -/* - * 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 deleted file mode 100644 index 251f84cf84..0000000000 --- a/test/versioned/disabled-instrumentation/package.json +++ /dev/null @@ -1,22 +0,0 @@ -{ - "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" - ] - } - ] -}