From 44a3f653fe808b51d0661157f6512af1452682d4 Mon Sep 17 00:00:00 2001 From: artahmetaj Date: Sun, 15 Oct 2023 13:27:41 +0200 Subject: [PATCH 1/4] fix(sdk-trace-base): processor onStart called with a span having empty attributes --- CHANGELOG.md | 2 + .../test/common/transform.test.ts | 34 ++++++++++++++++ .../opentelemetry-sdk-trace-base/src/Span.ts | 8 +++- .../src/Tracer.ts | 16 ++++---- .../test/common/Span.test.ts | 39 +++++++++++++++++++ 5 files changed, 91 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4b570dad46..bb156b4016e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) +* fix(sdk-trace-base): processor onStart called with a span having empty attributes + ## 1.18.1 ### :bug: (Bug Fix) diff --git a/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts b/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts index 4df7f73f6c3..c7def85fc42 100644 --- a/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts +++ b/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts @@ -236,6 +236,40 @@ describe('transform', () => { version: '1', }); }); + it('should map OpenTelemetry constructor attributes to a Zipkin tag', () => { + const span = new Span( + tracer, + api.ROOT_CONTEXT, + 'my-span', + spanContext, + api.SpanKind.SERVER, + parentId, + [], + undefined, + undefined, + { + key1: 'value1', + key2: 'value2', + } + ); + const tags: zipkinTypes.Tags = _toZipkinTags( + span, + defaultStatusCodeTagName, + defaultStatusErrorTagName + ); + + assert.deepStrictEqual(tags, { + key1: 'value1', + key2: 'value2', + [SemanticResourceAttributes.SERVICE_NAME]: 'zipkin-test', + 'telemetry.sdk.language': language, + 'telemetry.sdk.name': 'opentelemetry', + 'telemetry.sdk.version': VERSION, + cost: '112.12', + service: 'ui', + version: '1', + }); + }); it('should map OpenTelemetry SpanStatus.code to a Zipkin tag', () => { const span = new Span( tracer, diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 31fb1555ac7..665fbf59ab0 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -100,7 +100,8 @@ export class Span implements APISpan, ReadableSpan { parentSpanId?: string, links: Link[] = [], startTime?: TimeInput, - _deprecatedClock?: unknown // keeping this argument even though it is unused to ensure backwards compatibility + _deprecatedClock?: unknown, // keeping this argument even though it is unused to ensure backwards compatibility + initAttributes?: SpanAttributes ) { this.name = spanName; this._spanContext = spanContext; @@ -119,6 +120,11 @@ export class Span implements APISpan, ReadableSpan { this.resource = parentTracer.resource; this.instrumentationLibrary = parentTracer.instrumentationLibrary; this._spanLimits = parentTracer.getSpanLimits(); + + if (initAttributes != null) { + this.setAttributes(initAttributes); + } + this._spanProcessor = parentTracer.getActiveSpanProcessor(); this._spanProcessor.onStart(this, context); this._attributeValueLengthLimit = diff --git a/packages/opentelemetry-sdk-trace-base/src/Tracer.ts b/packages/opentelemetry-sdk-trace-base/src/Tracer.ts index b77a9427ec2..f943e6a11cc 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Tracer.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Tracer.ts @@ -132,6 +132,12 @@ export class Tracer implements api.Tracer { return nonRecordingSpan; } + // Set initial span attributes. The attributes object may have been mutated + // by the sampler, so we sanitize the merged attributes before setting them. + const initAttributes = sanitizeAttributes( + Object.assign(attributes, samplingResult.attributes) + ); + const span = new Span( this, context, @@ -140,14 +146,10 @@ export class Tracer implements api.Tracer { spanKind, parentSpanId, links, - options.startTime - ); - // Set initial span attributes. The attributes object may have been mutated - // by the sampler, so we sanitize the merged attributes before setting them. - const initAttributes = sanitizeAttributes( - Object.assign(attributes, samplingResult.attributes) + options.startTime, + undefined, + initAttributes ); - span.setAttributes(initAttributes); return span; } diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index 11a94ffc7c2..bd254ffb4cc 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1054,6 +1054,27 @@ describe('Span', () => { assert.ok(started); }); + it('should call onStart synchronously when span is started with initial attributes', () => { + let initAttributes; + const processor: SpanProcessor = { + onStart: span => { + initAttributes = { ...span.attributes }; + }, + forceFlush: () => Promise.resolve(), + onEnd() {}, + shutdown: () => Promise.resolve(), + }; + + const provider = new BasicTracerProvider(); + + provider.addSpanProcessor(processor); + + provider + .getTracer('default') + .startSpan('test', { attributes: { foo: 'bar' } }); + assert.deepStrictEqual(initAttributes, { foo: 'bar' }); + }); + it('should call onEnd synchronously when span is ended', () => { let ended = false; const processor: SpanProcessor = { @@ -1222,5 +1243,23 @@ describe('Span', () => { }); }); }); + + describe('when initial attributes are specified', () => { + it('should store specified attributes', () => { + const span = new Span( + tracer, + ROOT_CONTEXT, + name, + spanContext, + SpanKind.CLIENT, + undefined, + undefined, + undefined, + undefined, + { foo: 'bar' } + ); + assert.deepStrictEqual(span.attributes, { foo: 'bar' }); + }); + }); }); }); From 190fbf4882223be8a07b9e908bc314b5aae7081a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Fri, 10 Nov 2023 11:42:24 +0000 Subject: [PATCH 2/4] Improve test name --- packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index bd254ffb4cc..3018eb6ef6e 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1054,7 +1054,7 @@ describe('Span', () => { assert.ok(started); }); - it('should call onStart synchronously when span is started with initial attributes', () => { + it('include initial attributes in onStart', () => { let initAttributes; const processor: SpanProcessor = { onStart: span => { From 141143935fa2af86d9fc1de77184a3f27f5c069c Mon Sep 17 00:00:00 2001 From: Andre Cruz Date: Mon, 13 Nov 2023 13:15:45 +0000 Subject: [PATCH 3/4] Rename initAttributes to attributes --- packages/opentelemetry-sdk-trace-base/src/Span.ts | 6 +++--- .../test/common/Span.test.ts | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 665fbf59ab0..e3e37d82136 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -101,7 +101,7 @@ export class Span implements APISpan, ReadableSpan { links: Link[] = [], startTime?: TimeInput, _deprecatedClock?: unknown, // keeping this argument even though it is unused to ensure backwards compatibility - initAttributes?: SpanAttributes + attributes?: SpanAttributes ) { this.name = spanName; this._spanContext = spanContext; @@ -121,8 +121,8 @@ export class Span implements APISpan, ReadableSpan { this.instrumentationLibrary = parentTracer.instrumentationLibrary; this._spanLimits = parentTracer.getSpanLimits(); - if (initAttributes != null) { - this.setAttributes(initAttributes); + if (attributes != null) { + this.setAttributes(attributes); } this._spanProcessor = parentTracer.getActiveSpanProcessor(); diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index 3018eb6ef6e..09dfea1b949 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1054,11 +1054,11 @@ describe('Span', () => { assert.ok(started); }); - it('include initial attributes in onStart', () => { - let initAttributes; + it('include attributes in onStart', () => { + let attributes; const processor: SpanProcessor = { onStart: span => { - initAttributes = { ...span.attributes }; + attributes = { ...span.attributes }; }, forceFlush: () => Promise.resolve(), onEnd() {}, @@ -1072,7 +1072,7 @@ describe('Span', () => { provider .getTracer('default') .startSpan('test', { attributes: { foo: 'bar' } }); - assert.deepStrictEqual(initAttributes, { foo: 'bar' }); + assert.deepStrictEqual(attributes, { foo: 'bar' }); }); it('should call onEnd synchronously when span is ended', () => { @@ -1244,7 +1244,7 @@ describe('Span', () => { }); }); - describe('when initial attributes are specified', () => { + describe('when attributes are specified', () => { it('should store specified attributes', () => { const span = new Span( tracer, From 8bcd2a9fb6de62e5928c8cdac0d6a677915c164f Mon Sep 17 00:00:00 2001 From: Andre Cruz Date: Mon, 13 Nov 2023 13:18:02 +0000 Subject: [PATCH 4/4] Fix test name --- packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index 09dfea1b949..11b0f3f318e 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1054,7 +1054,7 @@ describe('Span', () => { assert.ok(started); }); - it('include attributes in onStart', () => { + it('should include attributes in onStart', () => { let attributes; const processor: SpanProcessor = { onStart: span => {