From b89d251ade7f3884834826f8e58457a24d20820c Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 21 Oct 2020 10:56:39 -0400 Subject: [PATCH 1/2] chore: remove explicit parent option --- .../opentelemetry-api/src/trace/NoopTracer.ts | 11 +++-- .../src/trace/SpanOptions.ts | 19 ++------- .../noop-implementations/noop-tracer.test.ts | 13 ------ .../test/NodeTracerProvider.test.ts | 37 ++++++++++------- .../opentelemetry-plugin-fetch/src/fetch.ts | 12 ++++-- packages/opentelemetry-tracing/src/Tracer.ts | 11 +---- .../test/BasicTracerProvider.test.ts | 40 +------------------ 7 files changed, 44 insertions(+), 99 deletions(-) diff --git a/packages/opentelemetry-api/src/trace/NoopTracer.ts b/packages/opentelemetry-api/src/trace/NoopTracer.ts index 7f61200d7e..b67e02c805 100644 --- a/packages/opentelemetry-api/src/trace/NoopTracer.ts +++ b/packages/opentelemetry-api/src/trace/NoopTracer.ts @@ -30,11 +30,14 @@ export class NoopTracer implements Tracer { // startSpan starts a noop span. startSpan(name: string, options?: SpanOptions, context?: Context): Span { - const parent = options?.parent; + const root = Boolean(options?.root); + if (root) { + return NOOP_SPAN; + } + const parentFromContext = context && getActiveSpan(context)?.context(); - if (isSpanContext(parent) && isSpanContextValid(parent)) { - return new NoopSpan(parent); - } else if ( + + if ( isSpanContext(parentFromContext) && isSpanContextValid(parentFromContext) ) { diff --git a/packages/opentelemetry-api/src/trace/SpanOptions.ts b/packages/opentelemetry-api/src/trace/SpanOptions.ts index e20f243eac..3c374044ae 100644 --- a/packages/opentelemetry-api/src/trace/SpanOptions.ts +++ b/packages/opentelemetry-api/src/trace/SpanOptions.ts @@ -17,8 +17,6 @@ import { Attributes } from './attributes'; import { Link } from './link'; import { SpanKind } from './span_kind'; -import { Span } from './span'; -import { SpanContext } from './span_context'; /** * Options needed for span creation @@ -36,20 +34,9 @@ export interface SpanOptions { /** {@link Link}s span to other spans */ links?: Link[]; - /** - * This option is NOT RECOMMENDED for normal use and should ONLY be used - * if your application manages context manually without the global context - * manager, or you are trying to override the parent extracted from context. - * - * A parent `SpanContext` (or `Span`, for convenience) that the newly-started - * span will be the child of. This overrides the parent span extracted from - * the currently active context. - * - * A null value here should prevent the SDK from extracting a parent from - * the current context, forcing the new span to be a root span. - */ - parent?: Span | SpanContext | null; - /** A manually specified start time for the created `Span` object. */ startTime?: number; + + /** The new span should be a root span. (Ignore parent from context). */ + root?: boolean; } diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts index 3f68c8a14c..bfc06b8133 100644 --- a/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts +++ b/packages/opentelemetry-api/test/noop-implementations/noop-tracer.test.ts @@ -60,19 +60,6 @@ describe('NoopTracer', () => { return patchedFn(); }); - it('should propagate valid spanContext on the span (from parent)', () => { - const tracer = new NoopTracer(); - const parent: SpanContext = { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - }; - const span = tracer.startSpan('test-1', { parent }); - assert(span.context().traceId === parent.traceId); - assert(span.context().spanId === parent.spanId); - assert(span.context().traceFlags === parent.traceFlags); - }); - it('should propagate valid spanContext on the span (from context)', () => { const tracer = new NoopTracer(); const parent: SpanContext = { diff --git a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts index debbca3dce..5fcc81e1b8 100644 --- a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { context, TraceFlags, setActiveSpan } from '@opentelemetry/api'; +import { + context, + TraceFlags, + setActiveSpan, + setExtractedSpanContext, +} from '@opentelemetry/api'; import { AlwaysOnSampler, AlwaysOffSampler, @@ -26,7 +31,7 @@ import { Span } from '@opentelemetry/tracing'; import { Resource, TELEMETRY_SDK_RESOURCE } from '@opentelemetry/resources'; import * as assert from 'assert'; import * as path from 'path'; -import { ContextManager } from '@opentelemetry/context-base'; +import { ContextManager, ROOT_CONTEXT } from '@opentelemetry/context-base'; import { NodeTracerProvider } from '../src/NodeTracerProvider'; const sleep = (time: number) => @@ -160,15 +165,15 @@ describe('NodeTracerProvider', () => { logger: new NoopLogger(), }); - const sampledParent = provider - .getTracer('default') - .startSpan('not-sampled-span', { - parent: { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - }, - }); + const sampledParent = provider.getTracer('default').startSpan( + 'not-sampled-span', + {}, + setExtractedSpanContext(ROOT_CONTEXT, { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + }) + ); assert.ok(sampledParent instanceof Span); assert.strictEqual( sampledParent.context().traceFlags, @@ -176,9 +181,13 @@ describe('NodeTracerProvider', () => { ); assert.strictEqual(sampledParent.isRecording(), true); - const span = provider.getTracer('default').startSpan('child-span', { - parent: sampledParent, - }); + const span = provider + .getTracer('default') + .startSpan( + 'child-span', + {}, + setActiveSpan(ROOT_CONTEXT, sampledParent) + ); assert.ok(span instanceof Span); assert.strictEqual(span.context().traceFlags, TraceFlags.SAMPLED); assert.strictEqual(span.isRecording(), true); diff --git a/packages/opentelemetry-plugin-fetch/src/fetch.ts b/packages/opentelemetry-plugin-fetch/src/fetch.ts index 11815c899a..20b28ea833 100644 --- a/packages/opentelemetry-plugin-fetch/src/fetch.ts +++ b/packages/opentelemetry-plugin-fetch/src/fetch.ts @@ -21,6 +21,7 @@ import * as web from '@opentelemetry/web'; import { AttributeNames } from './enums/AttributeNames'; import { FetchError, FetchResponse, SpanData } from './types'; import { VERSION } from './version'; +import { setActiveSpan } from '@opentelemetry/api'; // how long to wait for observer to collect information about resources // this is needed as event "load" is called before observer @@ -63,10 +64,13 @@ export class FetchPlugin extends core.BasePlugin> { span: api.Span, corsPreFlightRequest: PerformanceResourceTiming ): void { - const childSpan = this._tracer.startSpan('CORS Preflight', { - parent: span, - startTime: corsPreFlightRequest[web.PerformanceTimingNames.FETCH_START], - }); + const childSpan = this._tracer.startSpan( + 'CORS Preflight', + { + startTime: corsPreFlightRequest[web.PerformanceTimingNames.FETCH_START], + }, + setActiveSpan(api.context.active(), span) + ); web.addSpanNetworkEvents(childSpan, corsPreFlightRequest); childSpan.end( corsPreFlightRequest[web.PerformanceTimingNames.RESPONSE_END] diff --git a/packages/opentelemetry-tracing/src/Tracer.ts b/packages/opentelemetry-tracing/src/Tracer.ts index b0286fc65d..7702a59bd9 100644 --- a/packages/opentelemetry-tracing/src/Tracer.ts +++ b/packages/opentelemetry-tracing/src/Tracer.ts @@ -176,15 +176,6 @@ function getParent( options: api.SpanOptions, context: api.Context ): api.SpanContext | undefined { - if (options.parent === null) return undefined; - if (options.parent) return getContext(options.parent); + if (options.root) return undefined; return api.getParentSpanContext(context); } - -function getContext(span: api.Span | api.SpanContext): api.SpanContext { - return isSpan(span) ? span.context() : span; -} - -function isSpan(span: api.Span | api.SpanContext): span is api.Span { - return typeof (span as api.Span).context === 'function'; -} diff --git a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts index ac204f2770..f378805bd5 100644 --- a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts @@ -210,49 +210,13 @@ describe('BasicTracerProvider', () => { childSpan.end(); }); - it('should override context parent with option parent', () => { - const tracer = new BasicTracerProvider().getTracer('default'); - const span = tracer.startSpan('my-span'); - const overrideParent = tracer.startSpan('my-parent-override-span'); - const childSpan = tracer.startSpan( - 'child-span', - { - parent: overrideParent, - }, - setActiveSpan(ROOT_CONTEXT, span) - ); - const context = childSpan.context(); - assert.strictEqual(context.traceId, overrideParent.context().traceId); - assert.strictEqual(context.traceFlags, TraceFlags.SAMPLED); - span.end(); - childSpan.end(); - }); - - it('should override context parent with option parent context', () => { - const tracer = new BasicTracerProvider().getTracer('default'); - const span = tracer.startSpan('my-span'); - const overrideParent = tracer.startSpan('my-parent-override-span'); - const childSpan = tracer.startSpan( - 'child-span', - { - parent: overrideParent.context(), - }, - setActiveSpan(ROOT_CONTEXT, span) - ); - const context = childSpan.context(); - assert.strictEqual(context.traceId, overrideParent.context().traceId); - assert.strictEqual(context.traceFlags, TraceFlags.SAMPLED); - span.end(); - childSpan.end(); - }); - - it('should create a root span when parent is null', () => { + it('should create a root span when root is true', () => { const tracer = new BasicTracerProvider().getTracer('default'); const span = tracer.startSpan('my-span'); const overrideParent = tracer.startSpan('my-parent-override-span'); const rootSpan = tracer.startSpan( 'root-span', - { parent: null }, + { root: true }, setActiveSpan(ROOT_CONTEXT, span) ); const context = rootSpan.context(); From b5db3631b5f3a759d197bc051c00abb854dbaaeb Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 28 Oct 2020 09:14:20 -0400 Subject: [PATCH 2/2] chore: bump api compatibility version --- packages/opentelemetry-api/src/api/global-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-api/src/api/global-utils.ts b/packages/opentelemetry-api/src/api/global-utils.ts index 87791f2817..297836e009 100644 --- a/packages/opentelemetry-api/src/api/global-utils.ts +++ b/packages/opentelemetry-api/src/api/global-utils.ts @@ -65,4 +65,4 @@ export function makeGetter( * version. If the global API is not compatible with the API package * attempting to get it, a NOOP API implementation will be returned. */ -export const API_BACKWARDS_COMPATIBILITY_VERSION = 1; +export const API_BACKWARDS_COMPATIBILITY_VERSION = 2;