fix(otel): honor DD_TRACE_OTEL_ENABLED=false and OTEL_SDK_DISABLED=false (#8219)

* fix(otel): honor DD_TRACE_OTEL_ENABLED=false and OTEL_SDK_DISABLED=false

`otel-sdk-trace.js` and the `otel_enabled` telemetry tag in
`opentracing/span.js` checked these env vars for raw truthiness, which
made the string `'false'` itself truthy and mishandled the OTel spec's
positive opt-in form (`OTEL_SDK_DISABLED=false`). Replace the truthiness
check with explicit precedence: the Datadog opt-out
(`DD_TRACE_OTEL_ENABLED=false`) wins over every OTel signal, then the
OTel opt-out (`OTEL_SDK_DISABLED=true`) wins over the opt-in side, then
either explicit opt-in enables it; otherwise stay disabled by default.

Fixes: #4873

* refactor(span): read config off the parent tracer

DatadogSpan was reading `DD_TRACE_OTEL_ENABLED`, the experimental
flags, and `DD_TRACE_SPAN_LEAK_DEBUG` from `getValueFromEnvSources` at
module load. Anything that updated those values through the Config
singleton (remote config, programmatic options) never reached the span
gates because the gates had captured the env-time value. Read them off
`tracer._config` instead — the opentracing tracer already holds the
singleton.

Three coordinated pieces:

1. Convert `_parentTracer` to a `#parentTracer` private field. There
   are no external readers; `tracer()` is the public accessor. While
   in the file, fix three sites that were reading from a non-existent
   `this._tracer`.
2. Drop the comment block above the span-leak gate. It justified the
   `tracer?._config?.X` nullish chain that this refactor replaces.
3. The four spec files constructing `new DatadogSpan(...)` with
   `tracer = {}` (or `null`, or `{}` inline) now pass
   `tracer = { _config: getConfig() }`. The two
   `tracePropagationBehaviorExtract` overrides spread the real config
   so the constructor's other reads see real defaults.

Drive-by fix:

* Drop a long-dead commented-out `registry.register` line in
  spanleak.js, and the unused `operationName` argument span.js was
  passing to `addSpan(span)`.

Author: BridgeAR

packages/datadog-instrumentations/src/otel-sdk-trace.js

@@ -3,14 +3,10 @@
 const shimmer = require('../../datadog-shimmer')
 const tracer = require('../../dd-trace')
 const { getValueFromEnvSources } = require('../../dd-trace/src/config/helper')
+const { isFalse, isTrue } = require('../../dd-trace/src/util')
 const { addHook } = require('./helpers/instrument')
 
-const otelSdkEnabled = getValueFromEnvSources('DD_TRACE_OTEL_ENABLED') ||
-getValueFromEnvSources('OTEL_SDK_DISABLED')
-  ? !getValueFromEnvSources('OTEL_SDK_DISABLED')
-  : undefined
-
-if (otelSdkEnabled) {
+if (isOtelSdkEnabled()) {
   addHook({
     name: '@opentelemetry/sdk-trace-node',
     file: 'build/src/NodeTracerProvider.js',
@@ -22,3 +18,12 @@ if (otelSdkEnabled) {
     return mod
   })
 }
+
+function isOtelSdkEnabled () {
+  // Datadog explicit opt-out wins over every OTel signal; check it first.
+  const ddTraceOtelEnabled = getValueFromEnvSources('DD_TRACE_OTEL_ENABLED')
+  if (isFalse(ddTraceOtelEnabled)) return false
+  const otelSdkDisabled = getValueFromEnvSources('OTEL_SDK_DISABLED')
+  if (isTrue(otelSdkDisabled)) return false
+  return isTrue(ddTraceOtelEnabled) || isFalse(otelSdkDisabled)
+}

packages/datadog-instrumentations/test/otel-sdk-trace.spec.js

@@ -0,0 +1,96 @@
+'use strict'
+
+const assert = require('node:assert/strict')
+
+const { describe, it } = require('mocha')
+const proxyquire = require('proxyquire').noPreserveCache()
+const sinon = require('sinon')
+
+describe('otel-sdk-trace', () => {
+  /**
+   * Re-load `otel-sdk-trace.js` with the supplied env values stubbed in via
+   * `getValueFromEnvSources` and report what `addHook` saw.
+   *
+   * @param {{ ddTraceOtelEnabled?: string, otelSdkDisabled?: string }} env
+   * @param {{ TracerProvider?: unknown }} [tracerStub]
+   * @returns {{ addHook: sinon.SinonSpy, wrap: sinon.SinonSpy }}
+   */
+  function loadWithEnv (env, tracerStub = {}) {
+    const addHook = sinon.spy()
+    const wrap = sinon.spy()
+    const values = {
+      DD_TRACE_OTEL_ENABLED: env.ddTraceOtelEnabled,
+      OTEL_SDK_DISABLED: env.otelSdkDisabled,
+    }
+    proxyquire('../src/otel-sdk-trace', {
+      '../../datadog-shimmer': { wrap },
+      '../../dd-trace': tracerStub,
+      '../../dd-trace/src/config/helper': {
+        getValueFromEnvSources: (name) => values[name],
+      },
+      './helpers/instrument': { addHook },
+    })
+    return { addHook, wrap }
+  }
+
+  describe('gate precedence', () => {
+    it('disables when DD_TRACE_OTEL_ENABLED is the explicit opt-out', () => {
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'false' }).addHook.called, false)
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: '0' }).addHook.called, false)
+    })
+
+    it('keeps DD opt-out winning even when OTEL_SDK_DISABLED=false opts in', () => {
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'false', otelSdkDisabled: 'false' }).addHook.called, false)
+    })
+
+    it('disables when OTEL_SDK_DISABLED is the explicit opt-out', () => {
+      assert.equal(loadWithEnv({ otelSdkDisabled: 'true' }).addHook.called, false)
+      assert.equal(loadWithEnv({ otelSdkDisabled: '1' }).addHook.called, false)
+    })
+
+    it('keeps OTel opt-out winning even when DD_TRACE_OTEL_ENABLED=true opts in', () => {
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'true', otelSdkDisabled: 'true' }).addHook.called, false)
+    })
+
+    it('enables when DD_TRACE_OTEL_ENABLED is the explicit opt-in', () => {
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'true' }).addHook.called, true)
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: '1' }).addHook.called, true)
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'true', otelSdkDisabled: 'false' }).addHook.called, true)
+    })
+
+    it('enables when OTEL_SDK_DISABLED=false is the OTel positive opt-in', () => {
+      assert.equal(loadWithEnv({ otelSdkDisabled: 'false' }).addHook.called, true)
+      assert.equal(loadWithEnv({ otelSdkDisabled: '0' }).addHook.called, true)
+    })
+
+    it('stays disabled by default when neither env var is set', () => {
+      assert.equal(loadWithEnv({}).addHook.called, false)
+    })
+
+    it('stays disabled for unrecognized values on either side', () => {
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: 'maybe', otelSdkDisabled: 'sure' }).addHook.called, false)
+      assert.equal(loadWithEnv({ ddTraceOtelEnabled: '' }).addHook.called, false)
+    })
+  })
+
+  describe('hook registration', () => {
+    it('wraps NodeTracerProvider with the dd-trace TracerProvider', () => {
+      const tracerProvider = function FakeTracerProvider () {}
+      const { addHook, wrap } = loadWithEnv({ ddTraceOtelEnabled: 'true' }, { TracerProvider: tracerProvider })
+
+      sinon.assert.calledOnce(addHook)
+      const [hookOptions, transform] = addHook.firstCall.args
+      assert.deepStrictEqual(hookOptions, {
+        name: '@opentelemetry/sdk-trace-node',
+        file: 'build/src/NodeTracerProvider.js',
+        versions: ['*'],
+      })
+
+      const mod = { NodeTracerProvider: function OriginalProvider () {} }
+      assert.equal(transform(mod), mod)
+
+      sinon.assert.calledOnceWithExactly(wrap, mod, 'NodeTracerProvider', sinon.match.func)
+      assert.equal(wrap.firstCall.args[2](), tracerProvider)
+    })
+  })
+})

packages/dd-trace/src/opentracing/span.js

@@ -11,21 +11,16 @@ const runtimeMetrics = require('../runtime_metrics')
 const log = require('../log')
 const { storage } = require('../../../datadog-core')
 const telemetryMetrics = require('../telemetry/metrics')
-const { getValueFromEnvSources } = require('../config/helper')
-const { isTrue } = require('../util')
 const SpanContext = require('./span_context')
 
 const dateNow = Date.now
 
 const tracerMetrics = telemetryMetrics.manager.namespace('tracers')
 
-const DD_TRACE_EXPERIMENTAL_STATE_TRACKING = isTrue(getValueFromEnvSources('DD_TRACE_EXPERIMENTAL_STATE_TRACKING'))
-const DD_TRACE_EXPERIMENTAL_SPAN_COUNTS = isTrue(getValueFromEnvSources('DD_TRACE_EXPERIMENTAL_SPAN_COUNTS'))
-
 const unfinishedRegistry = createRegistry('unfinished')
 const finishedRegistry = createRegistry('finished')
 
-const OTEL_ENABLED = !!getValueFromEnvSources('DD_TRACE_OTEL_ENABLED')
+let OTEL_ENABLED = false
 const ALLOWED = new Set(['string', 'number', 'boolean'])
 
 const integrationCounters = {
@@ -55,15 +50,19 @@ function getIntegrationCounter (event, integration) {
 }
 
 class DatadogSpan {
+  #parentTracer
+
   constructor (tracer, processor, prioritySampler, fields, debug) {
+    OTEL_ENABLED = tracer._config.DD_TRACE_OTEL_ENABLED
+
     const operationName = fields.operationName
     const parent = fields.parent || null
     // TODO(BridgeAR): Investigate why this is causing a performance regression
     // eslint-disable-next-line prefer-object-spread
     const tags = Object.assign({}, fields.tags)
     const hostname = fields.hostname
 
-    this._parentTracer = tracer
+    this.#parentTracer = tracer
     this._debug = debug
     this._processor = processor
     this._prioritySampler = prioritySampler
@@ -94,7 +93,7 @@ class DatadogSpan {
       attributes: this._sanitizeAttributes(link.attributes),
     })) ?? []
 
-    if (DD_TRACE_EXPERIMENTAL_SPAN_COUNTS && finishedRegistry) {
+    if (this.#parentTracer._config.DD_TRACE_EXPERIMENTAL_SPAN_COUNTS && finishedRegistry) {
       runtimeMetrics.increment('runtime.node.spans.unfinished')
       runtimeMetrics.increment('runtime.node.spans.unfinished.by.name', `span_name:${operationName}`)
 
@@ -104,16 +103,8 @@ class DatadogSpan {
       unfinishedRegistry.register(this, operationName, this)
     }
 
-    // Nullish operator is used here because both `tracer` and `tracer._config`
-    // can be null and there are tests passing invalid values to the `Span`
-    // constructor which still succeed today. Part of the problem is that `Span`
-    // stores only the tracer and not the config, so anything that needs the
-    // config has to read it from the tracer stored on the span, including
-    // even `Span` itself in this case.
-    //
-    // TODO: Refactor Tracer/Span + tests to avoid having to do nullish checks.
-    if (tracer?._config?.DD_TRACE_SPAN_LEAK_DEBUG > 0) {
-      require('../spanleak').addSpan(this, operationName)
+    if (tracer._config.DD_TRACE_SPAN_LEAK_DEBUG > 0) {
+      require('../spanleak').addSpan(this)
     }
 
     if (startCh.hasSubscribers) {
@@ -124,7 +115,7 @@ class DatadogSpan {
   [util.inspect.custom] () {
     return {
       ...this,
-      _parentTracer: `[${this._parentTracer.constructor.name}]`,
+      parentTracer: `[${this.#parentTracer.constructor.name}]`,
       _prioritySampler: `[${this._prioritySampler.constructor.name}]`,
       _processor: `[${this._processor.constructor.name}]`,
     }
@@ -156,7 +147,7 @@ class DatadogSpan {
   }
 
   tracer () {
-    return this._parentTracer
+    return this.#parentTracer
   }
 
   setOperationName (name) {
@@ -254,14 +245,14 @@ class DatadogSpan {
       return
     }
 
-    if (DD_TRACE_EXPERIMENTAL_STATE_TRACKING && !this._spanContext._tags['service.name']) {
+    if (this.#parentTracer._config.DD_TRACE_EXPERIMENTAL_STATE_TRACKING && !this._spanContext._tags['service.name']) {
       log.error('Finishing invalid span: %s', this)
     }
 
     getIntegrationCounter('spans_finished', this._integrationName).inc()
     this._spanContext._tags['_dd.integration'] = this._integrationName
 
-    if (DD_TRACE_EXPERIMENTAL_SPAN_COUNTS && finishedRegistry) {
+    if (this.#parentTracer._config.DD_TRACE_EXPERIMENTAL_SPAN_COUNTS && finishedRegistry) {
       runtimeMetrics.decrement('runtime.node.spans.unfinished')
       runtimeMetrics.decrement('runtime.node.spans.unfinished.by.name', `span_name:${this._name}`)
       runtimeMetrics.increment('runtime.node.spans.finished')
@@ -336,7 +327,7 @@ class DatadogSpan {
     let startTime
 
     let baggage = {}
-    const propagationBehavior = this._parentTracer?._config?.DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT
+    const propagationBehavior = this.#parentTracer._config.DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT
     if (parent && parent._isRemote && propagationBehavior !== 'continue') {
       baggage = parent._baggageItems
       parent = null

packages/dd-trace/src/spanleak.js

@@ -85,7 +85,6 @@ module.exports.addSpan = function (span) {
   const expiration = now + LIFETIME
   const wrapped = new WeakRef(span)
   spans.add(wrapped, expiration)
-  // registry.register(span, span._name)
 }
 
 function isEnabled () {

packages/dd-trace/test/llmobs/util.spec.js

@@ -3,6 +3,8 @@
 const assert = require('node:assert/strict')
 
 const { before, describe, it } = require('mocha')
+
+const getConfig = require('../../src/config')
 const {
   encodeUnicode,
   getFunctionArguments,
@@ -169,29 +171,31 @@ describe('util', () => {
 
   describe('spanHasError', () => {
     let Span
+    let tracer
     let ps
 
     before(() => {
       Span = require('../../src/opentracing/span')
+      tracer = { _config: getConfig() }
       ps = {
         sample () {},
       }
     })
 
     it('returns false when there is no error', () => {
-      const span = new Span(null, null, ps, {})
+      const span = new Span(tracer, null, ps, {})
       assert.strictEqual(spanHasError(span), false)
     })
 
     it('returns true if the span has an "error" tag', () => {
-      const span = new Span(null, null, ps, {})
+      const span = new Span(tracer, null, ps, {})
       span.setTag('error', true)
       assert.strictEqual(spanHasError(span), true)
     })
 
     it('returns true if the span has the error properties as tags', () => {
       const err = new Error('boom')
-      const span = new Span(null, null, ps, {})
+      const span = new Span(tracer, null, ps, {})
 
       span.setTag('error.type', err.name)
       span.setTag('error.msg', err.message)

packages/dd-trace/test/opentracing/span.spec.js

@@ -33,7 +33,7 @@ describe('Span', () => {
     id.onFirstCall().returns('123')
     id.onSecondCall().returns('456')
 
-    tracer = {}
+    tracer = { _config: getConfig() }
 
     processor = {
       process: sinon.stub(),
@@ -499,21 +499,13 @@ describe('Span', () => {
       })
 
       it('should not propagate baggage items when Trace_Propagation_Behavior_Extract is set to ignore', () => {
-        tracer = {
-          _config: {
-            DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT: 'ignore',
-          },
-        }
+        tracer = { _config: { ...getConfig(), DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT: 'ignore' } }
         span = new Span(tracer, processor, prioritySampler, { operationName: 'operation', parent })
         assert.deepStrictEqual(span._spanContext._baggageItems, {})
       })
 
       it('should propagate baggage items when Trace_Propagation_Behavior_Extract is set to restart', () => {
-        tracer = {
-          _config: {
-            DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT: 'restart',
-          },
-        }
+        tracer = { _config: { ...getConfig(), DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT: 'restart' } }
         span = new Span(tracer, processor, prioritySampler, { operationName: 'operation', parent })
         assert.deepStrictEqual(span._spanContext._baggageItems, { foo: 'bar' })
       })

packages/dd-trace/test/standalone/index.spec.js

@@ -8,6 +8,7 @@ const proxyquire = require('proxyquire')
 const { channel } = require('dc-polyfill')
 
 require('../setup/core')
+const getConfig = require('../../src/config')
 const standalone = require('../../src/standalone')
 const DatadogSpan = require('../../src/opentracing/span')
 
@@ -40,7 +41,7 @@ describe('Disabled APM Tracing or Standalone', () => {
       },
     }
 
-    tracer = {}
+    tracer = { _config: getConfig() }
     processor = {}
     prioritySampler = {}
   })

packages/dd-trace/test/standalone/tracesource_priority_sampler.spec.js

@@ -8,6 +8,7 @@ const proxyquire = require('proxyquire')
 
 require('../setup/core')
 const { USER_KEEP, AUTO_KEEP } = require('../../../../ext/priority')
+const getConfig = require('../../src/config')
 const DatadogSpan = require('../../src/opentracing/span')
 const TraceSourcePrioritySampler = require('../../src/standalone/tracesource_priority_sampler')
 const { TRACE_SOURCE_PROPAGATION_KEY } = require('../../src/constants')
@@ -36,7 +37,8 @@ describe('Disabled APM Tracing or Standalone - TraceSourcePrioritySampler', () =
 
   describe('sample', () => {
     it('should provide the context when invoking _getPriorityFromTags', () => {
-      const span = new DatadogSpan({}, {}, prioritySampler, {
+      const tracer = { _config: getConfig() }
+      const span = new DatadogSpan(tracer, {}, prioritySampler, {
         operationName: 'operation',
       })