From f49b92b1abdcc81b70b91101169c8ad027371b74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerhard=20St=C3=B6bich?= <18708370+Flarna@users.noreply.github.com> Date: Wed, 7 Dec 2022 15:46:19 +0100 Subject: [PATCH 1/4] fix(api): use active context as default in NoopTracer To support the API without SDK use cast to forward a span context it's needed to fetch active context via ContextManager also in NoopTracer. Otherwise only direct passing of a context works but not if context.with() is used. --- api/CHANGELOG.md | 1 + api/src/trace/NoopTracer.ts | 6 ++++- .../noop-implementations/noop-tracer.test.ts | 23 +++++++++++++++++++ .../proxy-tracer.test.ts | 6 ++--- 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 3784f8abc8..c0cdb334b0 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. * fix(api): deprecate MetricAttributes and MetricAttributeValue [#3406](https://github.com/open-telemetry/opentelemetry-js/pull/3406) @blumamir * test(api): disable module concatenation in tree-shaking test [#3409](https://github.com/open-telemetry/opentelemetry-js/pull/3409) @legendecas +* fix(api): use active context as default in NoopTracer [ToDo] (https://github.com/open-telemetry/opentelemetry-js/pull/ToDo) @flarna ## [1.3.0](https://www.github.com/open-telemetry/opentelemetry-js-api/compare/v1.2.0...v1.3.0) diff --git a/api/src/trace/NoopTracer.ts b/api/src/trace/NoopTracer.ts index cf386c4f57..0656429b86 100644 --- a/api/src/trace/NoopTracer.ts +++ b/api/src/trace/NoopTracer.ts @@ -31,7 +31,11 @@ const contextApi = ContextAPI.getInstance(); */ export class NoopTracer implements Tracer { // startSpan starts a noop span. - startSpan(name: string, options?: SpanOptions, context?: Context): Span { + startSpan( + name: string, + options?: SpanOptions, + context = contextApi.active() + ): Span { const root = Boolean(options?.root); if (root) { return new NonRecordingSpan(); diff --git a/api/test/common/noop-implementations/noop-tracer.test.ts b/api/test/common/noop-implementations/noop-tracer.test.ts index 429f6d481d..1cb82e71fe 100644 --- a/api/test/common/noop-implementations/noop-tracer.test.ts +++ b/api/test/common/noop-implementations/noop-tracer.test.ts @@ -15,8 +15,10 @@ */ import * as assert from 'assert'; +import * as sinon from 'sinon'; import { context, + ROOT_CONTEXT, Span, SpanContext, SpanKind, @@ -27,6 +29,10 @@ import { NonRecordingSpan } from '../../../src/trace/NonRecordingSpan'; import { NoopTracer } from '../../../src/trace/NoopTracer'; describe('NoopTracer', () => { + afterEach(() => { + sinon.restore(); + }); + it('should not crash', () => { const tracer = new NoopTracer(); @@ -58,6 +64,23 @@ describe('NoopTracer', () => { assert(span.spanContext().traceFlags === parent.traceFlags); }); + it('should propagate valid spanContext on the span (from current context)', () => { + const tracer = new NoopTracer(); + const parent: SpanContext = { + traceId: 'd4cda95b652f4a1592b449dd92ffda3b', + spanId: '6e0c63ffe4e34c42', + traceFlags: TraceFlags.NONE, + }; + + const ctx = trace.setSpanContext(ROOT_CONTEXT, parent); + const activeStub = sinon.stub(context, 'active'); + activeStub.returns(ctx); + const span = tracer.startSpan('test-1'); + assert(span.spanContext().traceId === parent.traceId); + assert(span.spanContext().spanId === parent.spanId); + assert(span.spanContext().traceFlags === parent.traceFlags); + }); + it('should accept 2 to 4 args and start an active span', () => { const tracer = new NoopTracer(); const name = 'span-name'; diff --git a/api/test/common/proxy-implementations/proxy-tracer.test.ts b/api/test/common/proxy-implementations/proxy-tracer.test.ts index 9bf7e94409..707224a6d0 100644 --- a/api/test/common/proxy-implementations/proxy-tracer.test.ts +++ b/api/test/common/proxy-implementations/proxy-tracer.test.ts @@ -174,9 +174,9 @@ describe('ProxyTracer', () => { tracer.startSpan(name, options, ctx); // Assert the proxy tracer has the full API of the NoopTracer - assert.strictEqual( - NoopTracer.prototype.startSpan.length, - ProxyTracer.prototype.startSpan.length + assert.ok( + ProxyTracer.prototype.startSpan.length >= + NoopTracer.prototype.startSpan.length ); assert.deepStrictEqual(Object.getOwnPropertyNames(NoopTracer.prototype), [ 'constructor', From 19538da6b2b73905f5e7188ecd2200c061652358 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerhard=20St=C3=B6bich?= Date: Wed, 7 Dec 2022 15:56:49 +0100 Subject: [PATCH 2/4] update PR link --- api/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index c0cdb334b0..44fc3dc66b 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. * fix(api): deprecate MetricAttributes and MetricAttributeValue [#3406](https://github.com/open-telemetry/opentelemetry-js/pull/3406) @blumamir * test(api): disable module concatenation in tree-shaking test [#3409](https://github.com/open-telemetry/opentelemetry-js/pull/3409) @legendecas -* fix(api): use active context as default in NoopTracer [ToDo] (https://github.com/open-telemetry/opentelemetry-js/pull/ToDo) @flarna +* fix(api): use active context as default in NoopTracer [#3476] (https://github.com/open-telemetry/opentelemetry-js/pull/3476) @flarna ## [1.3.0](https://www.github.com/open-telemetry/opentelemetry-js-api/compare/v1.2.0...v1.3.0) From 987546f4156ee82bf4af89e99c7a5902e0e70bc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerhard=20St=C3=B6bich?= Date: Wed, 7 Dec 2022 16:04:05 +0100 Subject: [PATCH 3/4] fixup: correct link --- api/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 44fc3dc66b..eb11d5c2f9 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. * fix(api): deprecate MetricAttributes and MetricAttributeValue [#3406](https://github.com/open-telemetry/opentelemetry-js/pull/3406) @blumamir * test(api): disable module concatenation in tree-shaking test [#3409](https://github.com/open-telemetry/opentelemetry-js/pull/3409) @legendecas -* fix(api): use active context as default in NoopTracer [#3476] (https://github.com/open-telemetry/opentelemetry-js/pull/3476) @flarna +* fix(api): use active context as default in NoopTracer [#3476](https://github.com/open-telemetry/opentelemetry-js/pull/3476) @flarna ## [1.3.0](https://www.github.com/open-telemetry/opentelemetry-js-api/compare/v1.2.0...v1.3.0) From 85e008dee0cd97e83ec84a74dad40d88e86dbc6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerhard=20St=C3=B6bich?= <18708370+Flarna@users.noreply.github.com> Date: Wed, 14 Dec 2022 09:15:35 +0100 Subject: [PATCH 4/4] remove unneeded assert --- api/test/common/proxy-implementations/proxy-tracer.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/test/common/proxy-implementations/proxy-tracer.test.ts b/api/test/common/proxy-implementations/proxy-tracer.test.ts index 707224a6d0..55c39b3271 100644 --- a/api/test/common/proxy-implementations/proxy-tracer.test.ts +++ b/api/test/common/proxy-implementations/proxy-tracer.test.ts @@ -174,10 +174,6 @@ describe('ProxyTracer', () => { tracer.startSpan(name, options, ctx); // Assert the proxy tracer has the full API of the NoopTracer - assert.ok( - ProxyTracer.prototype.startSpan.length >= - NoopTracer.prototype.startSpan.length - ); assert.deepStrictEqual(Object.getOwnPropertyNames(NoopTracer.prototype), [ 'constructor', 'startSpan',