From 19beb863f68bbc0590ee80806073469d0e38cda5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 Feb 2024 09:33:30 +0100 Subject: [PATCH 1/2] ref: Align options for startSpan --- packages/opentelemetry/src/trace.ts | 55 +++++++++++++++++++++-------- packages/opentelemetry/src/types.ts | 18 +++------- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 83f69d385dfc..14358c136b28 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -1,4 +1,4 @@ -import type { Span, Tracer } from '@opentelemetry/api'; +import type { Span, SpanOptions, Tracer } from '@opentelemetry/api'; import { context } from '@opentelemetry/api'; import { SpanStatusCode, trace } from '@opentelemetry/api'; import { suppressTracing } from '@opentelemetry/core'; @@ -18,17 +18,19 @@ import { setSpanMetadata } from './utils/spanData'; * * Note that you'll always get a span passed to the callback, it may just be a NonRecordingSpan if the span is not sampled. */ -export function startSpan(spanContext: OpenTelemetrySpanContext, callback: (span: Span) => T): T { +export function startSpan(options: OpenTelemetrySpanContext, callback: (span: Span) => T): T { const tracer = getTracer(); - const { name } = spanContext; + const { name } = options; const activeCtx = context.active(); - const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const spanContext = getSpanContext(options); + return tracer.startActiveSpan(name, spanContext, ctx, span => { - _applySentryAttributesToSpan(span, spanContext); + _applySentryAttributesToSpan(span, options); return handleCallbackErrors( () => callback(span), @@ -49,17 +51,19 @@ export function startSpan(spanContext: OpenTelemetrySpanContext, callback: (s * * Note that you'll always get a span passed to the callback, it may just be a NonRecordingSpan if the span is not sampled. */ -export function startSpanManual(spanContext: OpenTelemetrySpanContext, callback: (span: Span) => T): T { +export function startSpanManual(options: OpenTelemetrySpanContext, callback: (span: Span) => T): T { const tracer = getTracer(); - const { name } = spanContext; + const { name } = options; const activeCtx = context.active(); - const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const spanContext = getSpanContext(options); + return tracer.startActiveSpan(name, spanContext, ctx, span => { - _applySentryAttributesToSpan(span, spanContext); + _applySentryAttributesToSpan(span, options); return handleCallbackErrors( () => callback(span), @@ -85,18 +89,20 @@ export const startActiveSpan = startSpan; * or you didn't set `tracesSampleRate` or `tracesSampler`, this function will not generate spans * and the `span` returned from the callback will be undefined. */ -export function startInactiveSpan(spanContext: OpenTelemetrySpanContext): Span { +export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { const tracer = getTracer(); - const { name } = spanContext; + const { name } = options; const activeCtx = context.active(); - const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const spanContext = getSpanContext(options); + const span = tracer.startSpan(name, spanContext, ctx); - _applySentryAttributesToSpan(span, spanContext); + _applySentryAttributesToSpan(span, options); return span; } @@ -120,8 +126,9 @@ function getTracer(): Tracer { return (client && client.tracer) || trace.getTracer('@sentry/opentelemetry', SDK_VERSION); } -function _applySentryAttributesToSpan(span: Span, spanContext: OpenTelemetrySpanContext): void { - const { origin, op, source, metadata } = spanContext; +function _applySentryAttributesToSpan(span: Span, options: OpenTelemetrySpanContext): void { + // eslint-disable-next-line deprecation/deprecation + const { origin, op, source, metadata } = options; if (origin) { span.setAttribute(InternalSentrySemanticAttributes.ORIGIN, origin); @@ -139,3 +146,21 @@ function _applySentryAttributesToSpan(span: Span, spanContext: OpenTelemetrySpan setSpanMetadata(span, metadata); } } + +function getSpanContext(options: OpenTelemetrySpanContext): SpanOptions { + const { startTime, attributes, kind } = options; + + // OTEL expects timestamps in ms, not seconds + const fixedStartTime = typeof startTime === 'number' ? ensureTimestampInMilliseconds(startTime) : startTime; + + return { + attributes, + kind, + startTime: fixedStartTime, + }; +} + +function ensureTimestampInMilliseconds(timestamp: number): number { + const isMs = timestamp < 9999999999; + return isMs ? timestamp * 1000 : timestamp; +} diff --git a/packages/opentelemetry/src/types.ts b/packages/opentelemetry/src/types.ts index 5abbdeeb4f26..ce16499c2a25 100644 --- a/packages/opentelemetry/src/types.ts +++ b/packages/opentelemetry/src/types.ts @@ -1,25 +1,15 @@ -import type { Attributes, Span as WriteableSpan, SpanKind, TimeInput, Tracer } from '@opentelemetry/api'; +import type { Span as WriteableSpan, SpanKind, Tracer } from '@opentelemetry/api'; import type { BasicTracerProvider, ReadableSpan, Span } from '@opentelemetry/sdk-trace-base'; -import type { Scope, SpanOrigin, TransactionMetadata, TransactionSource } from '@sentry/types'; +import type { Scope, StartSpanOptions } from '@sentry/types'; export interface OpenTelemetryClient { tracer: Tracer; traceProvider: BasicTracerProvider | undefined; } -export interface OpenTelemetrySpanContext { - name: string; - op?: string; - metadata?: Partial; - origin?: SpanOrigin; - source?: TransactionSource; - scope?: Scope; - onlyIfParent?: boolean; - - // Base SpanOptions we support - attributes?: Attributes; +export interface OpenTelemetrySpanContext extends StartSpanOptions { + // Additional otel-only option, for now...? kind?: SpanKind; - startTime?: TimeInput; } /** From f7572a95bb242ae81a4c035c2d1315a549811a37 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 Feb 2024 10:06:36 +0100 Subject: [PATCH 2/2] feat(opentelemetry): Align span options with core span options --- packages/core/test/lib/tracing/trace.test.ts | 2 +- packages/opentelemetry/src/contextManager.ts | 4 +- packages/opentelemetry/src/trace.ts | 20 ++- .../opentelemetry/src/utils/contextData.ts | 11 +- packages/opentelemetry/test/trace.test.ts | 150 ++++++++++++++++-- 5 files changed, 161 insertions(+), 26 deletions(-) diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index a260b8854cd0..5ce2e3e01c6b 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -291,7 +291,7 @@ describe('startSpan', () => { expect(spanToJSON(_span!).timestamp).toBeDefined(); }); - it('allows to pass a `startTime` yyy', () => { + it('allows to pass a `startTime`', () => { const start = startSpan({ name: 'outer', startTime: [1234, 0] }, span => { return spanToJSON(span!).start_timestamp; }); diff --git a/packages/opentelemetry/src/contextManager.ts b/packages/opentelemetry/src/contextManager.ts index aed172533eea..73bbd145b978 100644 --- a/packages/opentelemetry/src/contextManager.ts +++ b/packages/opentelemetry/src/contextManager.ts @@ -8,7 +8,7 @@ import { SENTRY_FORK_SET_SCOPE_CONTEXT_KEY, } from './constants'; import { getCurrentHub } from './custom/getCurrentHub'; -import { getScopesFromContext, setHubOnContext, setScopesOnContext } from './utils/contextData'; +import { getScopesFromContext, setContextOnScope, setHubOnContext, setScopesOnContext } from './utils/contextData'; /** * Wrap an OpenTelemetry ContextManager in a way that ensures the context is kept in sync with the Sentry Hub. @@ -70,6 +70,8 @@ export function wrapContextManagerClass(options: OpenTelemetrySpanContext, callback: (span: const { name } = options; - const activeCtx = context.active(); + const activeCtx = getContext(options.scope); const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; @@ -56,7 +57,7 @@ export function startSpanManual(options: OpenTelemetrySpanContext, callback: const { name } = options; - const activeCtx = context.active(); + const activeCtx = getContext(options.scope); const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; @@ -94,7 +95,7 @@ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { const { name } = options; - const activeCtx = context.active(); + const activeCtx = getContext(options.scope); const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; @@ -164,3 +165,14 @@ function ensureTimestampInMilliseconds(timestamp: number): number { const isMs = timestamp < 9999999999; return isMs ? timestamp * 1000 : timestamp; } + +function getContext(scope?: Scope): Context { + if (scope) { + const ctx = getContextFromScope(scope); + if (ctx) { + return ctx; + } + } + + return context.active(); +} diff --git a/packages/opentelemetry/src/utils/contextData.ts b/packages/opentelemetry/src/utils/contextData.ts index fa4dd56db99a..85c31ee822bf 100644 --- a/packages/opentelemetry/src/utils/contextData.ts +++ b/packages/opentelemetry/src/utils/contextData.ts @@ -55,12 +55,17 @@ export function getScopesFromContext(context: Context): CurrentScopes | undefine * This will return a forked context with the Propagation Context set. */ export function setScopesOnContext(context: Context, scopes: CurrentScopes): Context { - // So we can look up the context from the scope later - SCOPE_CONTEXT_MAP.set(scopes.scope, context); - return context.setValue(SENTRY_SCOPES_CONTEXT_KEY, scopes); } +/** + * Set the context on the scope so we can later look it up. + * We need this to get the context from the scope in the `trace` functions. + */ +export function setContextOnScope(scope: Scope, context: Context): void { + SCOPE_CONTEXT_MAP.set(scope, context); +} + /** * Get the context related to a scope. * TODO v8: Use this for the `trace` functions. diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index fa6e1d09dad8..969df0643da3 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -1,10 +1,10 @@ -import type { Span } from '@opentelemetry/api'; +import type { Span, TimeInput } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import { TraceFlags, context, trace } from '@opentelemetry/api'; import type { ReadableSpan } from '@opentelemetry/sdk-trace-base'; import { Span as SpanClass } from '@opentelemetry/sdk-trace-base'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, getClient } from '@sentry/core'; -import type { PropagationContext } from '@sentry/types'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, getClient, getCurrentScope } from '@sentry/core'; +import type { PropagationContext, Scope } from '@sentry/types'; import { InternalSentrySemanticAttributes } from '../src/semanticAttributes'; import { startInactiveSpan, startSpan, startSpanManual } from '../src/trace'; @@ -238,7 +238,7 @@ describe('trace', () => { }); it('allows to pass base SpanOptions', () => { - const date = Date.now() - 1000; + const date = [5000, 0] as TimeInput; startSpan( { @@ -248,12 +248,12 @@ describe('trace', () => { test1: 'test 1', test2: 2, }, - startTime: date, }, span => { expect(span).toBeDefined(); expect(getSpanName(span)).toEqual('outer'); + expect(getSpanStartTime(span)).toEqual(date); expect(getSpanAttributes(span)).toEqual({ [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, test1: 'test 1', @@ -264,6 +264,44 @@ describe('trace', () => { ); }); + it('allows to pass a startTime in seconds', () => { + const startTime = 1708504860.961; + const start = startSpan({ name: 'outer', startTime: startTime }, span => { + return getSpanStartTime(span); + }); + + expect(start).toEqual([1708504860, 961000000]); + }); + + it('allows to pass a scope', () => { + const initialScope = getCurrentScope(); + + let manualScope: Scope; + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + manualScope = getCurrentScope(); + manualScope.setTag('manual', 'tag'); + }); + + getCurrentScope().setTag('outer', 'tag'); + + startSpan({ name: 'GET users/[id]', scope: manualScope! }, span => { + expect(getCurrentScope()).not.toBe(initialScope); + + expect(getCurrentScope()).toEqual(manualScope); + expect(getActiveSpan()).toBe(span); + + expect(getSpanParentSpanId(span)).toBe(parentSpan.spanContext().spanId); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + + // TODO: propagation scope is not picked up by spans... + describe('onlyIfParent', () => { it('does not create a span if there is no parent', () => { const span = startSpan({ name: 'test span', onlyIfParent: true }, span => { @@ -355,7 +393,7 @@ describe('trace', () => { }); it('allows to pass base SpanOptions', () => { - const date = Date.now() - 1000; + const date = [5000, 0] as TimeInput; const span = startInactiveSpan({ name: 'outer', @@ -369,6 +407,7 @@ describe('trace', () => { expect(span).toBeDefined(); expect(getSpanName(span)).toEqual('outer'); + expect(getSpanStartTime(span)).toEqual(date); expect(getSpanAttributes(span)).toEqual({ [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, test1: 'test 1', @@ -377,6 +416,34 @@ describe('trace', () => { expect(getSpanKind(span)).toEqual(SpanKind.CLIENT); }); + it('allows to pass a startTime in seconds', () => { + const startTime = 1708504860.961; + const span = startInactiveSpan({ name: 'outer', startTime: startTime }); + + expect(getSpanStartTime(span)).toEqual([1708504860, 961000000]); + }); + + it('allows to pass a scope', () => { + const initialScope = getCurrentScope(); + + let manualScope: Scope; + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + manualScope = getCurrentScope(); + manualScope.setTag('manual', 'tag'); + }); + + getCurrentScope().setTag('outer', 'tag'); + + const span = startInactiveSpan({ name: 'GET users/[id]', scope: manualScope! }); + expect(getSpanParentSpanId(span)).toBe(parentSpan!.spanContext().spanId); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + describe('onlyIfParent', () => { it('does not create a span if there is no parent', () => { const span = startInactiveSpan({ name: 'test span', onlyIfParent: true }); @@ -439,7 +506,7 @@ describe('trace', () => { }); it('allows to pass base SpanOptions', () => { - const date = Date.now() - 1000; + const date = [5000, 0] as TimeInput; startSpanManual( { @@ -454,6 +521,7 @@ describe('trace', () => { span => { expect(span).toBeDefined(); expect(getSpanName(span)).toEqual('outer'); + expect(getSpanStartTime(span)).toEqual(date); expect(getSpanAttributes(span)).toEqual({ [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1, test1: 'test 1', @@ -463,27 +531,67 @@ describe('trace', () => { }, ); }); - }); - describe('onlyIfParent', () => { - it('does not create a span if there is no parent', () => { - const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => { - return span; + it('allows to pass a startTime in seconds', () => { + const startTime = 1708504860.961; + const start = startSpanManual({ name: 'outer', startTime: startTime }, span => { + const start = getSpanStartTime(span); + span.end(); + return start; }); - expect(span).not.toBeInstanceOf(SpanClass); + expect(start).toEqual([1708504860, 961000000]); }); - it('creates a span if there is a parent', () => { - const span = startSpan({ name: 'parent span' }, () => { + it('allows to pass a scope', () => { + const initialScope = getCurrentScope(); + + let manualScope: Scope; + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + manualScope = getCurrentScope(); + manualScope.setTag('manual', 'tag'); + }); + + getCurrentScope().setTag('outer', 'tag'); + + startSpanManual({ name: 'GET users/[id]', scope: manualScope! }, span => { + expect(getCurrentScope()).not.toBe(initialScope); + + expect(getCurrentScope()).toEqual(manualScope); + expect(getActiveSpan()).toBe(span); + + expect(getSpanParentSpanId(span)).toBe(parentSpan.spanContext().spanId); + + span.end(); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + + describe('onlyIfParent', () => { + it('does not create a span if there is no parent', () => { const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => { return span; }); - return span; + expect(span).not.toBeInstanceOf(SpanClass); }); - expect(span).toBeInstanceOf(SpanClass); + it('creates a span if there is a parent', () => { + const span = startSpan({ name: 'parent span' }, () => { + const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => { + return span; + }); + + return span; + }); + + expect(span).toBeInstanceOf(SpanClass); + }); }); }); }); @@ -835,6 +943,14 @@ function getSpanEndTime(span: AbstractSpan): [number, number] | undefined { return (span as ReadableSpan).endTime; } +function getSpanStartTime(span: AbstractSpan): [number, number] | undefined { + return (span as ReadableSpan).startTime; +} + function getSpanAttributes(span: AbstractSpan): Record | undefined { return spanHasAttributes(span) ? span.attributes : undefined; } + +function getSpanParentSpanId(span: AbstractSpan): string | undefined { + return (span as ReadableSpan).parentSpanId; +}