add experimental flag on plugins to disable them by default (#5985)

* add experimental flag on plugins to disable them by default

* fix support for environment variable

* change ordering to prioritize programmatic config

* remove experimental flag from prisma plugin

* remove old implementation

* Update packages/datadog-instrumentations/src/helpers/register.js

Co-authored-by: Thomas Watson <[email protected]>

* remove remaining plugin to instrumentation mapping code

---------

Co-authored-by: Thomas Watson <[email protected]>

Author: rochdev

packages/datadog-instrumentations/src/helpers/register.js

@@ -9,7 +9,6 @@ const log = require('../../../dd-trace/src/log')
 const checkRequireCache = require('./check-require-cache')
 const telemetry = require('../../../dd-trace/src/guardrails/telemetry')
 const { isInServerlessEnvironment } = require('../../../dd-trace/src/serverless')
-const { isFalse, isTrue, normalizePluginEnvName } = require('../../../dd-trace/src/util')
 const { getEnvironmentVariables } = require('../../../dd-trace/src/config-helper')
 
 const envs = getEnvironmentVariables()
@@ -25,22 +24,8 @@ const names = Object.keys(hooks)
 const pathSepExpr = new RegExp(`\\${path.sep}`, 'g')
 
 const disabledInstrumentations = new Set(
-  DD_TRACE_DISABLED_INSTRUMENTATIONS?.split(',').map(name => normalizePluginEnvName(name, true)) ?? []
+  DD_TRACE_DISABLED_INSTRUMENTATIONS?.split(',')
 )
-const reenabledInstrumentations = new Set()
-
-// Check for DD_TRACE_<INTEGRATION>_ENABLED environment variables
-for (const [key, value] of Object.entries(envs)) {
-  const match = key.match(/^DD_TRACE_(.+)_ENABLED$/)
-  if (match && value) {
-    const integration = normalizePluginEnvName(match[1], true)
-    if (isFalse(value)) {
-      disabledInstrumentations.add(integration)
-    } else if (isTrue(value)) {
-      reenabledInstrumentations.add(integration)
-    }
-  }
-}
 
 const loadChannel = channel('dd-trace:instrumentation:load')
 
@@ -65,8 +50,7 @@ const allInstrumentations = {}
 
 // TODO: make this more efficient
 for (const packageName of names) {
-  const normalizedPackageName = normalizePluginEnvName(packageName, true)
-  if (disabledInstrumentations.has(normalizedPackageName)) continue
+  if (disabledInstrumentations.has(packageName)) continue
 
   const hookOptions = {}
 
@@ -75,10 +59,6 @@ for (const packageName of names) {
   if (hook !== null && typeof hook === 'object') {
     if (hook.serverless === false && isInServerlessEnvironment()) continue
 
-    // some integrations are disabled by default, but can be enabled by setting
-    // the DD_TRACE_<INTEGRATION>_ENABLED environment variable to true
-    if (hook.disabled && !reenabledInstrumentations.has(normalizedPackageName)) continue
-
     hookOptions.internals = hook.esmFirst
     hook = hook.fn
   }

packages/datadog-instrumentations/test/helpers/register.spec.js

@@ -20,7 +20,6 @@ describe('register', () => {
 
     hooksMock = {
       '@confluentinc/kafka-javascript': {
-        disabled: true,
         fn: sinon.stub().returns('hooked')
       },
       'mongodb-core': {
@@ -72,54 +71,6 @@ describe('register', () => {
     }
   }
 
-  it('should skip hooks that are disabled by default and process enabled hooks', () => {
-    loadRegisterWithEnv()
-
-    expect(HookMock.callCount).to.equal(1)
-    expect(HookMock.args[0]).to.deep.include(['mongodb-core'], { internals: undefined })
-
-    runHookCallbacks(HookMock)
-
-    expect(hooksMock['@confluentinc/kafka-javascript'].fn).to.not.have.been.called
-    expect(hooksMock['mongodb-core'].fn).to.have.been.called
-  })
-
-  it('should enable disabled hooks when DD_TRACE_[pkg]_ENABLED is true', () => {
-    loadRegisterWithEnv({ DD_TRACE_CONFLUENTINC_KAFKA_JAVASCRIPT_ENABLED: 'true' })
-
-    expect(HookMock.callCount).to.equal(2)
-    expect(HookMock.args[0]).to.deep.include(['@confluentinc/kafka-javascript'], { internals: undefined })
-    expect(HookMock.args[1]).to.deep.include(['mongodb-core'], { internals: undefined })
-
-    runHookCallbacks(HookMock)
-
-    expect(hooksMock['@confluentinc/kafka-javascript'].fn).to.have.been.called
-    expect(hooksMock['mongodb-core'].fn).to.have.been.called
-  })
-
-  it('should not enable disabled hooks when DD_TRACE_[pkg]_ENABLED is false', () => {
-    loadRegisterWithEnv({ DD_TRACE_CONFLUENTINC_KAFKA_JAVASCRIPT_ENABLED: 'false' })
-
-    expect(HookMock.callCount).to.equal(1)
-    expect(HookMock.args[0]).to.deep.include(['mongodb-core'], { internals: undefined })
-
-    runHookCallbacks(HookMock)
-
-    expect(hooksMock['@confluentinc/kafka-javascript'].fn).to.not.have.been.called
-    expect(hooksMock['mongodb-core'].fn).to.have.been.called
-  })
-
-  it('should disable hooks that are disabled by DD_TRACE_[pkg]_ENABLED=false', () => {
-    loadRegisterWithEnv({ DD_TRACE_MONGODB_CORE_ENABLED: 'false' })
-
-    expect(HookMock.callCount).to.equal(0)
-
-    runHookCallbacks(HookMock)
-
-    expect(hooksMock['@confluentinc/kafka-javascript'].fn).to.not.have.been.called
-    expect(hooksMock['mongodb-core'].fn).to.not.have.been.called
-  })
-
   it('should disable hooks that are disabled by DD_TRACE_DISABLED_INSTRUMENTATIONS', () => {
     loadRegisterWithEnv({ DD_TRACE_DISABLED_INSTRUMENTATIONS: 'mongodb-core,@confluentinc/kafka-javascript' })
 

packages/dd-trace/src/plugin_manager.js

@@ -1,7 +1,7 @@
 'use strict'
 
 const { channel } = require('dc-polyfill')
-const { isFalse, normalizePluginEnvName } = require('./util')
+const { isFalse, isTrue, normalizePluginEnvName } = require('./util')
 const plugins = require('./plugins')
 const log = require('./log')
 const { getEnvironmentVariable } = require('../../dd-trace/src/config-helper')
@@ -32,8 +32,7 @@ loadChannel.subscribe(({ name }) => {
 function maybeEnable (Plugin) {
   if (!Plugin || typeof Plugin !== 'function') return
   if (!pluginClasses[Plugin.id]) {
-    const envName = `DD_TRACE_${Plugin.id.toUpperCase()}_ENABLED`
-    const enabled = getEnvironmentVariable(normalizePluginEnvName(envName))
+    const enabled = getEnvEnabled(Plugin)
 
     // TODO: remove the need to load the plugin class in order to disable the plugin
     if (isFalse(enabled) || disabledPlugins.has(Plugin.id)) {
@@ -46,6 +45,11 @@ function maybeEnable (Plugin) {
   }
 }
 
+function getEnvEnabled (Plugin) {
+  const envName = `DD_TRACE_${Plugin.id.toUpperCase()}_ENABLED`
+  return getEnvironmentVariable(normalizePluginEnvName(envName))
+}
+
 // TODO this must always be a singleton.
 module.exports = class PluginManager {
   constructor (tracer) {
@@ -74,7 +78,7 @@ module.exports = class PluginManager {
       this._pluginsByName[name] = new Plugin(this._tracer, this._tracerConfig)
     }
     const pluginConfig = this._configsByName[name] || {
-      enabled: this._tracerConfig.plugins !== false
+      enabled: this._tracerConfig.plugins !== false && (!Plugin.experimental || isTrue(getEnvEnabled(Plugin)))
     }
 
     // extracts predetermined configuration from tracer and combines it with plugin-specific config

packages/dd-trace/test/plugin_manager.spec.js

@@ -16,6 +16,7 @@ describe('Plugin Manager', () => {
   let Four
   let Five
   let Six
+  let Eight
   let pm
 
   beforeEach(() => {
@@ -45,7 +46,11 @@ describe('Plugin Manager', () => {
       six: class Six extends FakePlugin {
         static id = 'six'
       },
-      seven: {}
+      seven: {},
+      eight: class Eight extends FakePlugin {
+        static experimental = true
+        static id = 'eight'
+      }
     }
 
     Two = plugins.two
@@ -59,6 +64,9 @@ describe('Plugin Manager', () => {
     Six = plugins.six
     Six.prototype.configure = sinon.spy()
 
+    Eight = plugins.eight
+    Eight.prototype.configure = sinon.spy()
+
     process.env.DD_TRACE_DISABLED_PLUGINS = 'five,six,seven'
 
     PluginManager = proxyquire.noPreserveCache()('../src/plugin_manager', {
@@ -75,6 +83,7 @@ describe('Plugin Manager', () => {
 
   afterEach(() => {
     delete process.env.DD_TRACE_DISABLED_PLUGINS
+    delete process.env.DD_TRACE_EIGHT_ENABLED
     pm.destroy()
   })
 
@@ -265,6 +274,28 @@ describe('Plugin Manager', () => {
       })
     })
 
+    describe('with an experimental plugin', () => {
+      it('should disable the plugin by default', () => {
+        pm.configure()
+        loadChannel.publish({ name: 'eight' })
+        expect(Eight.prototype.configure).to.have.been.calledWithMatch({ enabled: false })
+      })
+
+      it('should enable the plugin when configured programmatically', () => {
+        pm.configure()
+        pm.configurePlugin('eight')
+        loadChannel.publish({ name: 'eight' })
+        expect(Eight.prototype.configure).to.have.been.calledWithMatch({ enabled: true })
+      })
+
+      it('should enable the plugin when configured with an environment variable', () => {
+        process.env.DD_TRACE_EIGHT_ENABLED = 'true'
+        pm.configure()
+        loadChannel.publish({ name: 'eight' })
+        expect(Eight.prototype.configure).to.have.been.calledWithMatch({ enabled: true })
+      })
+    })
+
     it('instantiates plugin classes', () => {
       pm.configure()
       loadChannel.publish({ name: 'two' })