From 9cd1c8bfc315ec9ec8ecc7be1e1b867b0466f7a7 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Mon, 23 May 2022 15:56:14 +0200 Subject: [PATCH] Add options for disabling default instrumentation (#664) * Add options for disabling default instrumentation We previously had the undocumented configuration option `ignoreInstrumentation` that didn't follow our previously set out naming and behavior conventions for configuration options. Split up this configuration option in separate configuration options for the different instrumentations. Closes #519 * Merge the instrumentHttp and https options These two options basically do the same thing, instrument the http and https modules, but there will most likely not be a scenario where one would instrument http requests but not https requests or the other way around. Merge both options into the `instrumentHttp` option so one option disables both instrumentations. * Fix config reading issue for instrument options The client loaded the instrument options too early, before the config was all read properly. This mean the `APPSIGNAL_INSTRUMENT_*` env vars were ignored. * Fix instrument options always returning true The `|| true` fallback for the default value would always set the options to true for the `initCorePlugins` function. Allow the value to be `undefined`, which is necessary because of the `Partial` argument to the client constructor. We only check if the `instrumentHttp` option value is `false` and not if it's true (the default). If a user explicitly sets it to `false` the instrumentation will not be enabled. --- ...s-for-disabling-default-instrumentation.md | 23 +++++++++++++++++++ .../nodejs/src/__tests__/bootstrap.test.ts | 18 +++++++++++++-- packages/nodejs/src/__tests__/config.test.ts | 3 +++ packages/nodejs/src/bootstrap.ts | 10 ++++---- packages/nodejs/src/client.ts | 12 +++++++--- packages/nodejs/src/config.ts | 3 +++ packages/nodejs/src/config/configmap.ts | 6 +++++ packages/nodejs/src/interfaces/options.ts | 3 +++ test/integration/diagnose | 2 +- 9 files changed, 70 insertions(+), 10 deletions(-) create mode 100644 packages/nodejs/.changesets/add-config-options-for-disabling-default-instrumentation.md diff --git a/packages/nodejs/.changesets/add-config-options-for-disabling-default-instrumentation.md b/packages/nodejs/.changesets/add-config-options-for-disabling-default-instrumentation.md new file mode 100644 index 00000000..148937c0 --- /dev/null +++ b/packages/nodejs/.changesets/add-config-options-for-disabling-default-instrumentation.md @@ -0,0 +1,23 @@ +--- +bump: "patch" +type: "add" +--- + +Add config options for disabling default instrumentation like HTTP, HTTPS, PostgreSQL (pg package) and Redis (node-redis package). + +The following configuration options have been added: + +- `instrumentHttp` +- `instrumentPg` +- `instrumentRedis` + +By default these configuration options are set to `true`, which means the instrumentation is active by default. If you want to disable one of these instrumentations, configure it by setting the configuration option to `false`. + +```js +// appsignal.js +// Brief example, see our docs for a full example + +const appsignal = new Appsignal({ + instrumentRedis: false // Disables the node-redis package instrumentation +}); +``` diff --git a/packages/nodejs/src/__tests__/bootstrap.test.ts b/packages/nodejs/src/__tests__/bootstrap.test.ts index 6294d27f..3325c13c 100644 --- a/packages/nodejs/src/__tests__/bootstrap.test.ts +++ b/packages/nodejs/src/__tests__/bootstrap.test.ts @@ -9,12 +9,26 @@ describe("Bootstrap", () => { } as unknown) as Instrumentation it("bootstraps the core instumentation plugins", () => { - initCorePlugins(mock, { ignoreInstrumentation: undefined }) + initCorePlugins(mock, { + instrumentationConfig: { + http: true, + https: true, + pg: true, + redis: true + } + }) expect(mock.load).toHaveBeenCalledTimes(4) }) it("can ignore a core instumentation plugin", () => { - initCorePlugins(mock, { ignoreInstrumentation: ["http"] }) + initCorePlugins(mock, { + instrumentationConfig: { + http: false, + https: true, + pg: undefined, + redis: true + } + }) expect(mock.load).toHaveBeenCalledTimes(3) expect((mock.load as any).mock.calls.map((m: any) => m[0])).not.toContain( diff --git a/packages/nodejs/src/__tests__/config.test.ts b/packages/nodejs/src/__tests__/config.test.ts index 9ae8be34..d82d3708 100644 --- a/packages/nodejs/src/__tests__/config.test.ts +++ b/packages/nodejs/src/__tests__/config.test.ts @@ -27,6 +27,9 @@ describe("Configuration", () => { ignoreActions: [], ignoreErrors: [], ignoreNamespaces: [], + instrumentHttp: true, + instrumentPg: true, + instrumentRedis: true, log: "file", requestHeaders: [ "accept", diff --git a/packages/nodejs/src/bootstrap.ts b/packages/nodejs/src/bootstrap.ts index b014bead..3b882d53 100644 --- a/packages/nodejs/src/bootstrap.ts +++ b/packages/nodejs/src/bootstrap.ts @@ -15,14 +15,16 @@ import * as gcProbe from "./probes/v8" */ export function initCorePlugins( instrumentation: Instrumentation, - { ignoreInstrumentation }: { ignoreInstrumentation?: string[] } + { + instrumentationConfig + }: { instrumentationConfig?: { [key: string]: boolean | undefined } } ) { let plugins: any[] = [httpPlugin, httpsPlugin, redisPlugin, pgPlugin] - // cull ignored plugins - if (ignoreInstrumentation && Array.isArray(ignoreInstrumentation)) { + // Do not load disabled instrumentation plugins + if (instrumentationConfig) { plugins = plugins.filter( - p => !ignoreInstrumentation.includes(p.PLUGIN_NAME) + plugin => instrumentationConfig[plugin.PLUGIN_NAME] !== false ) } diff --git a/packages/nodejs/src/client.ts b/packages/nodejs/src/client.ts index 7045c8c2..4a4be672 100644 --- a/packages/nodejs/src/client.ts +++ b/packages/nodejs/src/client.ts @@ -55,8 +55,6 @@ export class BaseClient implements Client { * Creates a new instance of the `Appsignal` object */ constructor(options: Partial = {}) { - const { ignoreInstrumentation } = options - this.config = new Configuration(options) this.extension = new Extension() this.logger = this.setUpLogger() @@ -73,7 +71,15 @@ export class BaseClient implements Client { this.instrumentation = new Instrumentation(this.tracer(), this.metrics()) - initCorePlugins(this.instrumentation, { ignoreInstrumentation }) + const { instrumentRedis, instrumentHttp, instrumentPg } = this.config.data + initCorePlugins(this.instrumentation, { + instrumentationConfig: { + http: instrumentHttp, + https: instrumentHttp, + pg: instrumentPg, + redis: instrumentRedis + } + }) initCoreProbes(this.metrics()) } diff --git a/packages/nodejs/src/config.ts b/packages/nodejs/src/config.ts index 7b8526c5..57759dee 100644 --- a/packages/nodejs/src/config.ts +++ b/packages/nodejs/src/config.ts @@ -127,6 +127,9 @@ export class Configuration { ignoreActions: [], ignoreErrors: [], ignoreNamespaces: [], + instrumentHttp: true, + instrumentPg: true, + instrumentRedis: true, log: "file", requestHeaders: [ "accept", diff --git a/packages/nodejs/src/config/configmap.ts b/packages/nodejs/src/config/configmap.ts index 61bc209e..69c6d978 100644 --- a/packages/nodejs/src/config/configmap.ts +++ b/packages/nodejs/src/config/configmap.ts @@ -17,6 +17,9 @@ export const ENV_TO_KEY_MAPPING: { [key: string]: string } = { APPSIGNAL_IGNORE_ACTIONS: "ignoreActions", APPSIGNAL_IGNORE_ERRORS: "ignoreErrors", APPSIGNAL_IGNORE_NAMESPACES: "ignoreNamespaces", + APPSIGNAL_INSTRUMENT_HTTP: "instrumentHttp", + APPSIGNAL_INSTRUMENT_PG: "instrumentPg", + APPSIGNAL_INSTRUMENT_REDIS: "instrumentRedis", APPSIGNAL_LOG: "log", APPSIGNAL_LOG_LEVEL: "logLevel", APPSIGNAL_LOG_PATH: "logPath", @@ -80,6 +83,9 @@ export const JS_TO_RUBY_MAPPING: { [key: string]: string } = { ignoreActions: "ignore_actions", ignoreErrors: "ignore_errors", ignoreNamespaces: "ignore_namespaces", + instrumentHttp: "instrument_http", + instrumentPg: "instrument_pg", + instrumentRedis: "instrument_redis", log: "log", logLevel: "log_level", logPath: "log_path", diff --git a/packages/nodejs/src/interfaces/options.ts b/packages/nodejs/src/interfaces/options.ts index 0cc23ded..9cf1067c 100644 --- a/packages/nodejs/src/interfaces/options.ts +++ b/packages/nodejs/src/interfaces/options.ts @@ -18,6 +18,9 @@ export interface AppsignalOptions { ignoreErrors: string[] ignoreInstrumentation: string[] ignoreNamespaces: string[] + instrumentHttp: boolean + instrumentPg: boolean + instrumentRedis: boolean log: string logPath: string name: string diff --git a/test/integration/diagnose b/test/integration/diagnose index ad7f5062..7e2b7d0c 160000 --- a/test/integration/diagnose +++ b/test/integration/diagnose @@ -1 +1 @@ -Subproject commit ad7f5062b111ed3586b94a768e0642eee9d4a9f2 +Subproject commit 7e2b7d0ce708b7fc61e333f2d12a30a7c620d907