From a9970374e3e451c95330073ec74a337b5e9823f3 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Jan 2024 14:20:21 +0100 Subject: [PATCH] feat(core): Add `spanToJSON()` method to get span properties This is supposed to be an internal API and not necessarily to be used by users. add comment for circular dependency --- packages/core/package.json | 7 ++ packages/core/src/index.ts | 5 +- packages/core/src/tracing/span.ts | 26 +++---- packages/core/src/tracing/transaction.ts | 1 + packages/core/src/utils/spanUtils.ts | 38 +++++++++- .../core/test/lib/utils/spanUtils.test.ts | 71 ++++++++++++++++++- .../test/integration/transactions.test.ts | 5 +- .../test/custom/transaction.test.ts | 5 +- .../test/integration/transactions.test.ts | 5 +- packages/types/src/index.ts | 10 ++- packages/types/src/span.ts | 34 +++++---- 11 files changed, 167 insertions(+), 40 deletions(-) diff --git a/packages/core/package.json b/packages/core/package.json index 3ef693023f85..61c2e117e6ea 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -56,5 +56,12 @@ "volta": { "extends": "../../package.json" }, + "madge": { + "detectiveOptions": { + "ts": { + "skipTypeImports": true + } + } + }, "sideEffects": false } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2c423f0744c3..3fe76b805fb3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -72,7 +72,10 @@ export { createCheckInEnvelope } from './checkin'; export { hasTracingEnabled } from './utils/hasTracingEnabled'; export { isSentryRequestUrl } from './utils/isSentryRequestUrl'; export { handleCallbackErrors } from './utils/handleCallbackErrors'; -export { spanToTraceHeader } from './utils/spanUtils'; +export { + spanToTraceHeader, + spanToJSON, +} from './utils/spanUtils'; export { DEFAULT_ENVIRONMENT } from './constants'; export { ModuleMetadata } from './integrations/metadata'; export { RequestData } from './integrations/requestdata'; diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts index a3126e5763b0..e66f9c5caa04 100644 --- a/packages/core/src/tracing/span.ts +++ b/packages/core/src/tracing/span.ts @@ -6,6 +6,7 @@ import type { SpanAttributeValue, SpanAttributes, SpanContext, + SpanJSON, SpanOrigin, SpanTimeInput, TraceContext, @@ -372,22 +373,9 @@ export class Span implements SpanInterface { } /** - * @inheritDoc + * Get JSON representation of this span. */ - public toJSON(): { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: { [key: string]: any }; - description?: string; - op?: string; - parent_span_id?: string; - span_id: string; - start_timestamp: number; - status?: string; - tags?: { [key: string]: Primitive }; - timestamp?: number; - trace_id: string; - origin?: SpanOrigin; - } { + public getSpanJSON(): SpanJSON { return dropUndefinedKeys({ data: this._getData(), description: this.description, @@ -408,6 +396,14 @@ export class Span implements SpanInterface { return !this.endTimestamp && !!this.sampled; } + /** + * Convert the object to JSON. + * @deprecated Use `spanToJSON(span)` instead. + */ + public toJSON(): SpanJSON { + return this.getSpanJSON(); + } + /** * Get the merged data for this span. * For now, this combines `data` and `attributes` together, diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index 7142ec3419e7..e0635c7e6b80 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -285,6 +285,7 @@ export class Transaction extends SpanClass implements TransactionInterface { // We don't want to override trace context trace: spanToTraceContext(this), }, + // TODO: Pass spans serialized via `spanToJSON()` here instead in v8. spans: finishedSpans, start_timestamp: this.startTimestamp, tags: this.tags, diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 9c4dcff17b62..9ecb98a6c990 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -1,11 +1,13 @@ -import type { Span, SpanTimeInput, TraceContext } from '@sentry/types'; +import type { Span, SpanJSON, SpanTimeInput, TraceContext } from '@sentry/types'; import { dropUndefinedKeys, generateSentryTraceHeader, timestampInSeconds } from '@sentry/utils'; +import type { Span as SpanClass } from '../tracing/span'; /** * Convert a span to a trace context, which can be sent as the `trace` context in an event. */ export function spanToTraceContext(span: Span): TraceContext { - const { data, description, op, parent_span_id, span_id, status, tags, trace_id, origin } = span.toJSON(); + const { spanId: span_id, traceId: trace_id } = span; + const { data, description, op, parent_span_id, status, tags, origin } = spanToJSON(span); return dropUndefinedKeys({ data, @@ -54,3 +56,35 @@ function ensureTimestampInSeconds(timestamp: number): number { const isMs = timestamp > 9999999999; return isMs ? timestamp / 1000 : timestamp; } + +/** + * Convert a span to a JSON representation. + * Note that all fields returned here are optional and need to be guarded against. + * + * Note: Because of this, we currently have a circular type dependency (which we opted out of in package.json). + * This is not avoidable as we need `spanToJSON` in `spanUtils.ts`, which in turn is needed by `span.ts` for backwards compatibility. + * And `spanToJSON` needs the Span class from `span.ts` to check here. + * TODO v8: When we remove the deprecated stuff from `span.ts`, we can remove the circular dependency again. + */ +export function spanToJSON(span: Span): Partial { + if (spanIsSpanClass(span)) { + return span.getSpanJSON(); + } + + // Fallback: We also check for `.toJSON()` here... + // eslint-disable-next-line deprecation/deprecation + if (typeof span.toJSON === 'function') { + // eslint-disable-next-line deprecation/deprecation + return span.toJSON(); + } + + return {}; +} + +/** + * Sadly, due to circular dependency checks we cannot actually import the Span class here and check for instanceof. + * :( So instead we approximate this by checking if it has the `getSpanJSON` method. + */ +function spanIsSpanClass(span: Span): span is SpanClass { + return typeof (span as SpanClass).getSpanJSON === 'function'; +} diff --git a/packages/core/test/lib/utils/spanUtils.test.ts b/packages/core/test/lib/utils/spanUtils.test.ts index a6d84cf31166..d4b0afc7a7a6 100644 --- a/packages/core/test/lib/utils/spanUtils.test.ts +++ b/packages/core/test/lib/utils/spanUtils.test.ts @@ -1,6 +1,6 @@ import { TRACEPARENT_REGEXP, timestampInSeconds } from '@sentry/utils'; import { Span, spanToTraceHeader } from '../../../src'; -import { spanTimeInputToSeconds } from '../../../src/utils/spanUtils'; +import { spanTimeInputToSeconds, spanToJSON } from '../../../src/utils/spanUtils'; describe('spanToTraceHeader', () => { test('simple', () => { @@ -46,3 +46,72 @@ describe('spanTimeInputToSeconds', () => { expect(spanTimeInputToSeconds(timestamp)).toEqual(seconds + 0.000009); }); }); + +describe('spanToJSON', () => { + it('works with a simple span', () => { + const span = new Span(); + expect(spanToJSON(span)).toEqual({ + span_id: span.spanId, + trace_id: span.traceId, + origin: 'manual', + start_timestamp: span.startTimestamp, + }); + }); + + it('works with a full span', () => { + const span = new Span({ + name: 'test name', + op: 'test op', + parentSpanId: '1234', + spanId: '5678', + status: 'ok', + tags: { + foo: 'bar', + }, + traceId: 'abcd', + origin: 'auto', + startTimestamp: 123, + }); + + expect(spanToJSON(span)).toEqual({ + description: 'test name', + op: 'test op', + parent_span_id: '1234', + span_id: '5678', + status: 'ok', + tags: { + foo: 'bar', + }, + trace_id: 'abcd', + origin: 'auto', + start_timestamp: 123, + }); + }); + + it('works with a custom class without spanToJSON', () => { + const span = { + toJSON: () => { + return { + span_id: 'span_id', + trace_id: 'trace_id', + origin: 'manual', + start_timestamp: 123, + }; + }, + } as unknown as Span; + + expect(spanToJSON(span)).toEqual({ + span_id: 'span_id', + trace_id: 'trace_id', + origin: 'manual', + start_timestamp: 123, + }); + }); + + it('returns empty object if span does not have getter methods', () => { + // eslint-disable-next-line + const span = new Span().toJSON(); + + expect(spanToJSON(span as unknown as Span)).toEqual({}); + }); +}); diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts index be48f5f9e6b5..5198c532ef79 100644 --- a/packages/node-experimental/test/integration/transactions.test.ts +++ b/packages/node-experimental/test/integration/transactions.test.ts @@ -1,6 +1,7 @@ import { SpanKind, TraceFlags, context, trace } from '@opentelemetry/api'; import type { SpanProcessor } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; +import { spanToJSON } from '@sentry/core'; import { SentrySpanProcessor, getCurrentHub, setPropagationContextOnContext } from '@sentry/opentelemetry'; import type { Integration, PropagationContext, TransactionEvent } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -145,7 +146,7 @@ describe('Integration | Transactions', () => { // note: Currently, spans do not have any context/span added to them // This is the same behavior as for the "regular" SDKs - expect(spans.map(span => span.toJSON())).toEqual([ + expect(spans.map(span => spanToJSON(span))).toEqual([ { data: { 'otel.kind': 'INTERNAL' }, description: 'inner span 1', @@ -399,7 +400,7 @@ describe('Integration | Transactions', () => { // note: Currently, spans do not have any context/span added to them // This is the same behavior as for the "regular" SDKs - expect(spans.map(span => span.toJSON())).toEqual([ + expect(spans.map(span => spanToJSON(span))).toEqual([ { data: { 'otel.kind': 'INTERNAL' }, description: 'inner span 1', diff --git a/packages/opentelemetry/test/custom/transaction.test.ts b/packages/opentelemetry/test/custom/transaction.test.ts index 043d76235140..5377371b3077 100644 --- a/packages/opentelemetry/test/custom/transaction.test.ts +++ b/packages/opentelemetry/test/custom/transaction.test.ts @@ -1,3 +1,4 @@ +import { spanToJSON } from '@sentry/core'; import { getCurrentHub } from '../../src/custom/hub'; import { OpenTelemetryScope } from '../../src/custom/scope'; import { OpenTelemetryTransaction, startTransaction } from '../../src/custom/transaction'; @@ -157,7 +158,7 @@ describe('startTranscation', () => { spanMetadata: {}, }); - expect(transaction.toJSON()).toEqual( + expect(spanToJSON(transaction)).toEqual( expect.objectContaining({ origin: 'manual', span_id: expect.any(String), @@ -186,7 +187,7 @@ describe('startTranscation', () => { spanMetadata: {}, }); - expect(transaction.toJSON()).toEqual( + expect(spanToJSON(transaction)).toEqual( expect.objectContaining({ origin: 'manual', span_id: 'span1', diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index 787d251b0558..de5193292cef 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -4,6 +4,7 @@ import { addBreadcrumb, setTag } from '@sentry/core'; import type { PropagationContext, TransactionEvent } from '@sentry/types'; import { logger } from '@sentry/utils'; +import { spanToJSON } from '@sentry/core'; import { getCurrentHub } from '../../src/custom/hub'; import { SentrySpanProcessor } from '../../src/spanProcessor'; import { startInactiveSpan, startSpan } from '../../src/trace'; @@ -142,7 +143,7 @@ describe('Integration | Transactions', () => { // note: Currently, spans do not have any context/span added to them // This is the same behavior as for the "regular" SDKs - expect(spans.map(span => span.toJSON())).toEqual([ + expect(spans.map(span => spanToJSON(span))).toEqual([ { data: { 'otel.kind': 'INTERNAL' }, description: 'inner span 1', @@ -393,7 +394,7 @@ describe('Integration | Transactions', () => { // note: Currently, spans do not have any context/span added to them // This is the same behavior as for the "regular" SDKs - expect(spans.map(span => span.toJSON())).toEqual([ + expect(spans.map(span => spanToJSON(span))).toEqual([ { data: { 'otel.kind': 'INTERNAL' }, description: 'inner span 1', diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 655962f592fc..46ddba04812d 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -89,7 +89,15 @@ export type { // eslint-disable-next-line deprecation/deprecation export type { Severity, SeverityLevel } from './severity'; -export type { Span, SpanContext, SpanOrigin, SpanAttributeValue, SpanAttributes, SpanTimeInput } from './span'; +export type { + Span, + SpanContext, + SpanOrigin, + SpanAttributeValue, + SpanAttributes, + SpanTimeInput, + SpanJSON, +} from './span'; export type { StackFrame } from './stackframe'; export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace'; export type { TextEncoderInternal } from './textencoder'; diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 4ad0e5aa1110..af51afd0e5e9 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -28,6 +28,21 @@ export type SpanAttributes = Record; /** This type is aligned with the OpenTelemetry TimeInput type. */ export type SpanTimeInput = HrTime | number | Date; +/** A JSON representation of a span. */ +export interface SpanJSON { + data?: { [key: string]: any }; + description?: string; + op?: string; + parent_span_id?: string; + span_id: string; + start_timestamp: number; + status?: string; + tags?: { [key: string]: Primitive }; + timestamp?: number; + trace_id: string; + origin?: SpanOrigin; +} + /** Interface holding all properties that can be set on a Span on creation. */ export interface SpanContext { /** @@ -256,20 +271,11 @@ export interface Span extends SpanContext { */ getTraceContext(): TraceContext; - /** Convert the object to JSON */ - toJSON(): { - data?: { [key: string]: any }; - description?: string; - op?: string; - parent_span_id?: string; - span_id: string; - start_timestamp: number; - status?: string; - tags?: { [key: string]: Primitive }; - timestamp?: number; - trace_id: string; - origin?: SpanOrigin; - }; + /** + * Convert the object to JSON. + * @deprecated Use `spanToJSON(span)` instead. + */ + toJSON(): SpanJSON; /** * If this is span is actually recording data.