From 0999ce6ab2bcde6ab77ad502f03a7c203667050d Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Wed, 8 Nov 2023 18:25:22 +0100 Subject: [PATCH] PROF-8545: Restore eager release of tags, adapt endpoint profiling code (#3759) * Restores eager release of tags for exported spans in `process_spans.js`. * Moves tag fetching in `wall.js` from `_updateContext` to `_enter` to compensate for above. * Fixes some tests that relied on tags being lazily released. --- .../dd-trace/src/profiling/profilers/wall.js | 45 ++++++++++++------- packages/dd-trace/src/span_processor.js | 4 ++ packages/dd-trace/test/tracer.spec.js | 41 +++++++---------- 3 files changed, 48 insertions(+), 42 deletions(-) diff --git a/packages/dd-trace/src/profiling/profilers/wall.js b/packages/dd-trace/src/profiling/profilers/wall.js index 5c6f01555e7..1e60572fb22 100644 --- a/packages/dd-trace/src/profiling/profilers/wall.js +++ b/packages/dd-trace/src/profiling/profilers/wall.js @@ -120,6 +120,7 @@ class NativeWallProfiler { this._pprof.time.setContext(this._currentContext) this._lastSpan = undefined this._lastStartedSpans = undefined + this._lastWebTags = undefined this._lastSampleCount = 0 beforeCh.subscribe(this._enter) @@ -145,10 +146,29 @@ class NativeWallProfiler { const span = getActiveSpan() if (span) { this._lastSpan = span - this._lastStartedSpans = getStartedSpans(span.context()) + const startedSpans = getStartedSpans(span.context()) + this._lastStartedSpans = startedSpans + if (this._endpointCollectionEnabled) { + let found = false + // Find the first webspan starting from the end: + // There might be several webspans, for example with next.js, http plugin creates a first span + // and then next.js plugin creates a child span, and this child span haves the correct endpoint information. + for (let i = startedSpans.length - 1; i >= 0; i--) { + const tags = getSpanContextTags(startedSpans[i]) + if (isWebServerSpan(tags)) { + this._lastWebTags = tags + found = true + break + } + } + if (!found) { + this._lastWebTags = undefined + } + } } else { this._lastStartedSpans = undefined this._lastSpan = undefined + this._lastWebTags = undefined } } @@ -163,21 +183,11 @@ class NativeWallProfiler { context.rootSpanId = rootSpan.context().toSpanId() } } - if (this._endpointCollectionEnabled) { - const startedSpans = this._lastStartedSpans - // Find the first webspan starting from the end: - // There might be several webspans, for example with next.js, http plugin creates a first span - // and then next.js plugin creates a child span, and this child span haves the correct endpoint information. - for (let i = startedSpans.length - 1; i >= 0; i--) { - const tags = getSpanContextTags(startedSpans[i]) - if (isWebServerSpan(tags)) { - context.webTags = tags - // endpoint may not be determined yet, but keep it as fallback - // if tags are not available anymore during serialization - context.endpoint = endpointNameFromTags(tags) - break - } - } + if (this._lastWebTags) { + context.webTags = this._lastWebTags + // endpoint may not be determined yet, but keep it as fallback + // if tags are not available anymore during serialization + context.endpoint = endpointNameFromTags(this._lastWebTags) } } @@ -224,6 +234,9 @@ class NativeWallProfiler { beforeCh.unsubscribe(this._enter) enterCh.unsubscribe(this._enter) this._profilerState = undefined + this._lastSpan = undefined + this._lastStartedSpans = undefined + this._lastWebTags = undefined } this._started = false diff --git a/packages/dd-trace/src/span_processor.js b/packages/dd-trace/src/span_processor.js index c6e8c300529..aea348b11fb 100644 --- a/packages/dd-trace/src/span_processor.js +++ b/packages/dd-trace/src/span_processor.js @@ -138,6 +138,10 @@ class SpanProcessor { } } + for (const span of trace.finished) { + span.context()._tags = {} + } + trace.started = active trace.finished = [] } diff --git a/packages/dd-trace/test/tracer.spec.js b/packages/dd-trace/test/tracer.spec.js index 79c81aafec8..8591ebe3b8f 100644 --- a/packages/dd-trace/test/tracer.spec.js +++ b/packages/dd-trace/test/tracer.spec.js @@ -13,6 +13,7 @@ const { DD_MAJOR } = require('../../../version') const SPAN_TYPE = tags.SPAN_TYPE const RESOURCE_NAME = tags.RESOURCE_NAME const SERVICE_NAME = tags.SERVICE_NAME +const EXPORT_SERVICE_NAME = 'service' const BASE_SERVICE = tags.BASE_SERVICE const describeOrphanable = DD_MAJOR < 4 ? describe : describe.skip @@ -39,6 +40,7 @@ describe('Tracer', () => { tracer = new Tracer(config) tracer._exporter.setUrl = sinon.stub() + tracer._exporter.export = sinon.stub() tracer._prioritySampler.configure = sinon.stub() }) @@ -89,38 +91,25 @@ describe('Tracer', () => { }) describe('_dd.base_service', () => { - let genSpan - it('should be set when tracer.trace service mismatches configured service', () => { - tracer.trace('name', { service: 'custom' }, span => { - genSpan = span - }) - const tags = genSpan.context()._tags - expect(genSpan).to.be.instanceof(Span) - expect(tags).to.include({ - [BASE_SERVICE]: 'service', - [SERVICE_NAME]: 'custom' - }) + tracer.trace('name', { service: 'custom' }, () => {}) + const trace = tracer._exporter.export.getCall(0).args[0][0] + expect(trace).to.have.property(EXPORT_SERVICE_NAME, 'custom') + expect(trace.meta).to.have.property(BASE_SERVICE, 'service') }) it('should not be set when tracer.trace service is not supplied', () => { - tracer.trace('name', {}, span => { - genSpan = span - }) - const tags = genSpan.context()._tags - expect(genSpan).to.be.instanceof(Span) - expect(tags).to.have.property(SERVICE_NAME, 'service') - expect(tags).to.not.have.property(BASE_SERVICE) + tracer.trace('name', {}, () => {}) + const trace = tracer._exporter.export.getCall(0).args[0][0] + expect(trace).to.have.property(EXPORT_SERVICE_NAME, 'service') + expect(trace.meta).to.not.have.property(BASE_SERVICE) }) - it('should be set when tracer.trace service matched configured service', () => { - tracer.trace('name', { service: 'service' }, span => { - genSpan = span - }) - const tags = genSpan.context()._tags - expect(genSpan).to.be.instanceof(Span) - expect(tags).to.have.property(SERVICE_NAME, 'service') - expect(tags).to.not.have.property(BASE_SERVICE) + it('should not be set when tracer.trace service matched configured service', () => { + tracer.trace('name', { service: 'service' }, () => {}) + const trace = tracer._exporter.export.getCall(0).args[0][0] + expect(trace).to.have.property(EXPORT_SERVICE_NAME, 'service') + expect(trace.meta).to.not.have.property(BASE_SERVICE) }) })