fix(profiler): Remove endpoint recomputation, promptly observe tag updates (#7864)

Author: szegedi

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

@@ -35,6 +35,7 @@ const integrationCounters = {
 
 const startCh = channel('dd-trace:span:start')
 const finishCh = channel('dd-trace:span:finish')
+const tagsUpdateCh = channel('dd-trace:span:tags:update')
 
 function getIntegrationCounter (event, integration) {
   const counters = integrationCounters[event]
@@ -399,6 +400,10 @@ class DatadogSpan {
     tagger.add(this._spanContext._tags, keyValuePairs)
 
     this._prioritySampler.sample(this, false)
+
+    if (tagsUpdateCh.hasSubscribers) {
+      tagsUpdateCh.publish(this)
+    }
   }
 }
 

packages/dd-trace/src/profiling/profilers/wall.js

@@ -20,6 +20,7 @@ const TRACE_ENDPOINT_LABEL = 'trace endpoint'
 let beforeCh
 const enterCh = dc.channel('dd-trace:storage:enter')
 const spanFinishCh = dc.channel('dd-trace:span:finish')
+const tagsUpdateCh = dc.channel('dd-trace:span:tags:update')
 const profilerTelemetryMetrics = telemetryMetrics.manager.namespace('profilers')
 
 const ProfilingContext = Symbol('NativeWallProfiler.ProfilingContext')
@@ -122,6 +123,7 @@ class NativeWallProfiler {
   // Bind these to this so they can be used as callbacks
   #boundEnter = this.#enter.bind(this)
   #boundSpanFinished = this.#spanFinished.bind(this)
+  #boundSpanTagsUpdated = this.#spanTagsUpdated.bind(this)
   #boundGenerateLabels = this._generateLabels.bind(this)
 
   get type () { return 'wall' }
@@ -201,6 +203,9 @@ class NativeWallProfiler {
         }
         enterCh.subscribe(this.#boundEnter)
         spanFinishCh.subscribe(this.#boundSpanFinished)
+        if (this.#endpointCollectionEnabled) {
+          tagsUpdateCh.subscribe(this.#boundSpanTagsUpdated)
+        }
       }
     }
 
@@ -287,15 +292,7 @@ class NativeWallProfiler {
       }
 
       profilingContext = { spanId, rootSpanId, webTags }
-      // Don't cache if endpoint collection is enabled and webTags is undefined but
-      // the span's type hasn't been set yet. TracingPlugin.startSpan() calls
-      // enterWith() before the plugin sets span.type='web' via addRequestTags(),
-      // so the first enterCh event fires before the type is known. Without this
-      // guard we'd cache webTags=undefined and then serve that stale value on the
-      // subsequent activation (when span.type='web' is already set).
-      if (!this.#endpointCollectionEnabled || webTags !== undefined || context._tags['span.type']) {
-        span[ProfilingContext] = profilingContext
-      }
+      span[ProfilingContext] = profilingContext
     }
     return profilingContext
   }
@@ -314,6 +311,16 @@ class NativeWallProfiler {
     }
   }
 
+  #spanTagsUpdated (span) {
+    if (!this.#started) return
+    const profilingContext = span[ProfilingContext]
+    if (profilingContext === undefined || profilingContext.webTags !== undefined) return
+    const tags = span.context()._tags
+    if (isWebServerSpan(tags)) {
+      profilingContext.webTags = tags
+    }
+  }
+
   #reportV8bug (maybeBug) {
     const tag = `v8_profiler_bug_workaround_enabled:${this.#v8ProfilerBugWorkaroundEnabled}`
     const metric = `v8_cpu_profiler${maybeBug ? '_maybe' : ''}_stuck_event_loop`
@@ -352,6 +359,9 @@ class NativeWallProfiler {
         }
         enterCh.unsubscribe(this.#boundEnter)
         spanFinishCh.unsubscribe(this.#boundSpanFinished)
+        if (this.#endpointCollectionEnabled) {
+          tagsUpdateCh.unsubscribe(this.#boundSpanTagsUpdated)
+        }
         this._profilerState = undefined
       }
       this.#started = false

packages/dd-trace/test/profiling/profilers/wall.spec.js

@@ -300,16 +300,18 @@ describe('profilers/native/wall', () => {
     // TracingPlugin.startSpan() calls storage.enterWith({span}) immediately on span
     // creation, before the plugin calls addRequestTags() to set span.type='web'.
     // This means the first enterCh event fires with span.type unset. The profiler
-    // must not cache webTags=undefined from that first event, or the subsequent
-    // activation (with span.type='web' already set) would incorrectly use the
-    // stale cache and never produce trace endpoint labels.
+    // caches the profilingContext with webTags=undefined. When addRequestTags()
+    // later sets span.type='web', the dd-trace:span:tags:update channel fires and
+    // the profiler updates the cached context's webTags in place.
     let enterCh
+    let tagsUpdateCh
     let currentStore
     let localPprof
     let WallProfiler
 
     beforeEach(() => {
       enterCh = dc.channel('dd-trace:storage:enter')
+      tagsUpdateCh = dc.channel('dd-trace:span:tags:update')
       currentStore = null
 
       // Fresh setContext stub so we can track calls independently per test.
@@ -351,7 +353,7 @@ describe('profilers/native/wall', () => {
       return { span, tags }
     }
 
-    it('should recompute webTags on re-activation after span.type is set (ACF path)', () => {
+    it('should resolve webTags via tags update channel (ACF path)', () => {
       const { span: webSpan, tags: webSpanTags } = makeWebSpan()
       const profiler = new WallProfiler({
         endpointCollectionEnabled: true,
@@ -360,22 +362,26 @@ describe('profilers/native/wall', () => {
       })
       profiler.start()
 
-      // First activation: span.type not yet set → webTags cannot be determined
+      // First activation: span.type not yet set → webTags cached as undefined
       currentStore = { span: webSpan }
       enterCh.publish()
-      assert.strictEqual(localPprof.time.setContext.getCall(0).args[0].webTags, undefined)
+      const ctx0 = localPprof.time.setContext.getCall(0).args[0]
+      assert.strictEqual(ctx0.webTags, undefined)
 
-      // Plugin sets span.type='web' (simulating addRequestTags)
+      // Re-activation alone won't resolve webTags — cached context returned as-is
       webSpanTags['span.type'] = 'web'
-
-      // Second activation: span.type='web' → webTags must now be the tags object
       enterCh.publish()
-      assert.strictEqual(localPprof.time.setContext.getCall(1).args[0].webTags, webSpanTags)
+      assert.strictEqual(localPprof.time.setContext.getCall(1).args[0], ctx0)
+      assert.strictEqual(ctx0.webTags, undefined)
+
+      // The tags update channel resolves it in place — no re-activation needed
+      tagsUpdateCh.publish(webSpan)
+      assert.strictEqual(ctx0.webTags, webSpanTags)
 
       profiler.stop()
     })
 
-    it('should recompute webTags on re-activation after span.type is set (non-ACF path)', () => {
+    it('should resolve webTags via tags update channel (non-ACF path)', () => {
       const { span: webSpan, tags: webSpanTags } = makeWebSpan()
       const profiler = new WallProfiler({
         endpointCollectionEnabled: true,
@@ -384,26 +390,26 @@ describe('profilers/native/wall', () => {
       })
       profiler.start()
 
-      // In non-ACF mode, start() calls #setNewContext() which calls setContext({ref:{}}).
-      // Subsequent #enter() calls mutate the .ref property of that holder in place.
       const contextHolder = localPprof.time.setContext.getCall(0).args[0]
 
-      // First activation: span.type not yet set → webTags=undefined
+      // First activation: span.type not yet set → webTags cached as undefined
       currentStore = { span: webSpan }
       enterCh.publish()
       assert.strictEqual(contextHolder.ref.webTags, undefined)
 
-      // Plugin sets span.type='web'
+      // Re-activation alone won't resolve webTags — cached context returned as-is
       webSpanTags['span.type'] = 'web'
-
-      // Second activation: must recompute and find webTags
       enterCh.publish()
+      assert.strictEqual(contextHolder.ref.webTags, undefined)
+
+      // The tags update channel resolves it in place through the ref
+      tagsUpdateCh.publish(webSpan)
       assert.strictEqual(contextHolder.ref.webTags, webSpanTags)
 
       profiler.stop()
     })
 
-    it('should propagate webTags to child spans after web span type is set (ACF path)', () => {
+    it('should propagate webTags to child spans after tags update resolves parent (ACF path)', () => {
       const { span: webSpan, tags: webSpanTags, spanId: webSpanId } = makeWebSpan()
       const { span: childSpan } = makeChildSpan(webSpanId, webSpan)
 
@@ -414,11 +420,11 @@ describe('profilers/native/wall', () => {
       })
       profiler.start()
 
-      // Activate web span twice (first without type, then with type)
+      // Activate web span, then resolve via tags update channel
       currentStore = { span: webSpan }
       enterCh.publish()
       webSpanTags['span.type'] = 'web'
-      enterCh.publish()
+      tagsUpdateCh.publish(webSpan)
 
       // Now activate the child span — it must inherit webTags via parent walk
       currentStore = { span: childSpan }
@@ -428,5 +434,71 @@ describe('profilers/native/wall', () => {
 
       profiler.stop()
     })
+
+    it('should not update webTags for non-web spans via tags update channel', () => {
+      const { span: webSpan, spanId: webSpanId } = makeWebSpan()
+      const { span: childSpan } = makeChildSpan(webSpanId, webSpan)
+
+      const profiler = new WallProfiler({
+        endpointCollectionEnabled: true,
+        codeHotspotsEnabled: true,
+        asyncContextFrameEnabled: true,
+      })
+      profiler.start()
+
+      // Activate child span (router type)
+      currentStore = { span: childSpan }
+      enterCh.publish()
+      const childCtx = localPprof.time.setContext.lastCall.args[0]
+
+      // Tags update on child span should not set webTags (it's not a web span)
+      tagsUpdateCh.publish(childSpan)
+      assert.strictEqual(childCtx.webTags, undefined)
+
+      profiler.stop()
+    })
+
+    it('should ignore tags update for spans without cached profiling context', () => {
+      const { span: webSpan, tags: webSpanTags } = makeWebSpan()
+      const profiler = new WallProfiler({
+        endpointCollectionEnabled: true,
+        codeHotspotsEnabled: true,
+        asyncContextFrameEnabled: true,
+      })
+      profiler.start()
+
+      // Publish tags update before the span is ever activated — no cached context
+      webSpanTags['span.type'] = 'web'
+      tagsUpdateCh.publish(webSpan)
+
+      // No setContext call beyond the initial setup
+      sinon.assert.notCalled(localPprof.time.setContext)
+
+      profiler.stop()
+    })
+
+    it('should not update already-resolved webTags via tags update channel', () => {
+      const { span: webSpan, tags: webSpanTags } = makeWebSpan()
+      webSpanTags['span.type'] = 'web'
+
+      const profiler = new WallProfiler({
+        endpointCollectionEnabled: true,
+        codeHotspotsEnabled: true,
+        asyncContextFrameEnabled: true,
+      })
+      profiler.start()
+
+      // Activate with span.type already set → webTags resolved immediately
+      currentStore = { span: webSpan }
+      enterCh.publish()
+      const ctx0 = localPprof.time.setContext.getCall(0).args[0]
+      assert.strictEqual(ctx0.webTags, webSpanTags)
+
+      // Tags update should be a no-op since webTags is already set
+      tagsUpdateCh.publish(webSpan)
+      assert.strictEqual(ctx0.webTags, webSpanTags)
+
+      profiler.stop()
+    })
   })
 })