Skip to content

Commit

Permalink
feat(core): Deprecate Span.parentSpanId (#10244)
Browse files Browse the repository at this point in the history
Deprecate the `Span.parentSpanId` field on the interface and class. 

This required only a couple of code replacements and a bunch of test
adjustments. Also went ahead and changed the integration test event type
in the tests I was modifying.
  • Loading branch information
Lms24 committed Jan 19, 2024
1 parent f3b2e7d commit d3fecc1
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 48 deletions.
1 change: 1 addition & 0 deletions MIGRATION.md
Expand Up @@ -193,6 +193,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
- `span.getTraceContext()`: Use `spanToTraceContext(span)` utility function instead.
- `span.sampled`: Use `span.isRecording()` instead.
- `span.spanId`: Use `span.spanContext().spanId` instead.
- `span.parentSpanId`: Use `spanToJSON(span).parent_span_id` instead.
- `span.traceId`: Use `span.spanContext().traceId` instead.
- `span.name`: Use `spanToJSON(span).description` instead.
- `span.description`: Use `spanToJSON(span).description` instead.
Expand Down
@@ -1,5 +1,5 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import type { SerializedEvent } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
Expand All @@ -10,7 +10,7 @@ sentryTest('should send a transaction in an envelope', async ({ getLocalTestPath
}

const url = await getLocalTestPath({ testDir: __dirname });
const transaction = await getFirstSentryEnvelopeRequest<Event>(page, url);
const transaction = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

expect(transaction.transaction).toBe('parent_span');
expect(transaction.spans).toBeDefined();
Expand All @@ -22,14 +22,13 @@ sentryTest('should report finished spans as children of the root transaction', a
}

const url = await getLocalTestPath({ testDir: __dirname });
const transaction = await getFirstSentryEnvelopeRequest<Event>(page, url);
const transaction = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

const rootSpanId = transaction?.contexts?.trace?.spanId;
const rootSpanId = transaction?.contexts?.trace?.span_id;

expect(transaction.spans).toHaveLength(1);

const span_1 = transaction.spans?.[0];
// eslint-disable-next-line deprecation/deprecation
expect(span_1?.description).toBe('child_span');
expect(span_1?.parentSpanId).toEqual(rootSpanId);
expect(span_1?.parent_span_id).toEqual(rootSpanId);
});
@@ -1,5 +1,5 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import type { SerializedEvent } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
Expand All @@ -10,7 +10,7 @@ sentryTest('should report a transaction in an envelope', async ({ getLocalTestPa
}

const url = await getLocalTestPath({ testDir: __dirname });
const transaction = await getFirstSentryEnvelopeRequest<Event>(page, url);
const transaction = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

expect(transaction.transaction).toBe('test_transaction_1');
expect(transaction.spans).toBeDefined();
Expand All @@ -22,28 +22,26 @@ sentryTest('should report finished spans as children of the root transaction', a
}

const url = await getLocalTestPath({ testDir: __dirname });
const transaction = await getFirstSentryEnvelopeRequest<Event>(page, url);
const transaction = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

const rootSpanId = transaction?.contexts?.trace?.spanId;
const rootSpanId = transaction?.contexts?.trace?.span_id;

expect(transaction.spans).toHaveLength(3);

const span_1 = transaction.spans?.[0];

// eslint-disable-next-line deprecation/deprecation
expect(span_1?.op).toBe('span_1');
expect(span_1?.parentSpanId).toEqual(rootSpanId);
// eslint-disable-next-line deprecation/deprecation
expect(span_1?.data).toMatchObject({ foo: 'bar', baz: [1, 2, 3] });
expect(span_1).toBeDefined();
expect(span_1!.op).toBe('span_1');
expect(span_1!.parent_span_id).toEqual(rootSpanId);
expect(span_1!.data).toMatchObject({ foo: 'bar', baz: [1, 2, 3] });

const span_3 = transaction.spans?.[1];
// eslint-disable-next-line deprecation/deprecation
expect(span_3?.op).toBe('span_3');
expect(span_3?.parentSpanId).toEqual(rootSpanId);
expect(span_3).toBeDefined();
expect(span_3!.op).toBe('span_3');
expect(span_3!.parent_span_id).toEqual(rootSpanId);

const span_5 = transaction.spans?.[2];
// eslint-disable-next-line deprecation/deprecation
expect(span_5?.op).toBe('span_5');
// eslint-disable-next-line deprecation/deprecation
expect(span_5?.parentSpanId).toEqual(span_3?.spanId);
expect(span_5).toBeDefined();
expect(span_5!.op).toBe('span_5');
expect(span_5!.parent_span_id).toEqual(span_3!.span_id);
});
@@ -1,5 +1,5 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import type { SerializedEvent } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
Expand All @@ -16,16 +16,14 @@ sentryTest('should capture a FID vital.', async ({ browserName, getLocalTestPath
// To trigger FID
await page.click('#fid-btn');

const eventData = await getFirstSentryEnvelopeRequest<Event>(page);
const eventData = await getFirstSentryEnvelopeRequest<SerializedEvent>(page);

expect(eventData.measurements).toBeDefined();
expect(eventData.measurements?.fid?.value).toBeDefined();

// eslint-disable-next-line deprecation/deprecation
const fidSpan = eventData.spans?.filter(({ description }) => description === 'first input delay')[0];

expect(fidSpan).toBeDefined();
// eslint-disable-next-line deprecation/deprecation
expect(fidSpan?.op).toBe('ui.action');
expect(fidSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
expect(fidSpan?.parent_span_id).toBe(eventData.contexts?.trace?.span_id);
});
@@ -1,5 +1,5 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';
import type { SerializedEvent } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
Expand All @@ -11,18 +11,16 @@ sentryTest('should capture FP vital.', async ({ browserName, getLocalTestPath, p
}

const url = await getLocalTestPath({ testDir: __dirname });
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
const eventData = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

expect(eventData.measurements).toBeDefined();
expect(eventData.measurements?.fp?.value).toBeDefined();

// eslint-disable-next-line deprecation/deprecation
const fpSpan = eventData.spans?.filter(({ description }) => description === 'first-paint')[0];

expect(fpSpan).toBeDefined();
// eslint-disable-next-line deprecation/deprecation
expect(fpSpan?.op).toBe('paint');
expect(fpSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
expect(fpSpan?.parent_span_id).toBe(eventData.contexts?.trace?.span_id);
});

sentryTest('should capture FCP vital.', async ({ getLocalTestPath, page }) => {
Expand All @@ -31,16 +29,14 @@ sentryTest('should capture FCP vital.', async ({ getLocalTestPath, page }) => {
}

const url = await getLocalTestPath({ testDir: __dirname });
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
const eventData = await getFirstSentryEnvelopeRequest<SerializedEvent>(page, url);

expect(eventData.measurements).toBeDefined();
expect(eventData.measurements?.fcp?.value).toBeDefined();

// eslint-disable-next-line deprecation/deprecation
const fcpSpan = eventData.spans?.filter(({ description }) => description === 'first-contentful-paint')[0];

expect(fcpSpan).toBeDefined();
// eslint-disable-next-line deprecation/deprecation
expect(fcpSpan?.op).toBe('paint');
expect(fcpSpan?.parentSpanId).toBe(eventData.contexts?.trace_span_id);
expect(fcpSpan?.parent_span_id).toBe(eventData.contexts?.trace?.span_id);
});
2 changes: 1 addition & 1 deletion docs/v8-new-performance-apis.md
Expand Up @@ -46,7 +46,7 @@ below to see which things used to exist, and how they can/should be mapped going
| --------------------- | ---------------------------------------------------- |
| `traceId` | `spanContext().traceId` |
| `spanId` | `spanContext().spanId` |
| `parentSpanId` | Unchanged |
| `parentSpanId` | `spanToJSON(span).parent_span_id` |
| `status` | use utility method TODO |
| `sampled` | `spanIsSampled(span)` |
| `startTimestamp` | `startTime` - note that this has a different format! |
Expand Down
32 changes: 23 additions & 9 deletions packages/core/src/tracing/span.ts
Expand Up @@ -63,11 +63,6 @@ export class SpanRecorder {
* Span contains all data about a span
*/
export class Span implements SpanInterface {
/**
* @inheritDoc
*/
public parentSpanId?: string;

/**
* Tags for the span.
* @deprecated Use `getSpanAttributes(span)` instead.
Expand Down Expand Up @@ -112,6 +107,7 @@ export class Span implements SpanInterface {

protected _traceId: string;
protected _spanId: string;
protected _parentSpanId?: string;
protected _sampled: boolean | undefined;
protected _name?: string;
protected _attributes: SpanAttributes;
Expand Down Expand Up @@ -147,7 +143,7 @@ export class Span implements SpanInterface {
this._name = spanContext.name || spanContext.description;

if (spanContext.parentSpanId) {
this.parentSpanId = spanContext.parentSpanId;
this._parentSpanId = spanContext.parentSpanId;
}
// We want to include booleans as well here
if ('sampled' in spanContext) {
Expand Down Expand Up @@ -231,6 +227,24 @@ export class Span implements SpanInterface {
this._spanId = spanId;
}

/**
* @inheritDoc
*
* @deprecated Use `startSpan` functions instead.
*/
public set parentSpanId(string) {
this._parentSpanId = string;
}

/**
* @inheritDoc
*
* @deprecated Use `spanToJSON(span).parent_span_id` instead.
*/
public get parentSpanId(): string | undefined {
return this._parentSpanId;
}

/**
* Was this span chosen to be sent as part of the sample?
* @deprecated Use `isRecording()` instead.
Expand Down Expand Up @@ -531,7 +545,7 @@ export class Span implements SpanInterface {
endTimestamp: this._endTime,
// eslint-disable-next-line deprecation/deprecation
op: this.op,
parentSpanId: this.parentSpanId,
parentSpanId: this._parentSpanId,
sampled: this._sampled,
spanId: this._spanId,
startTimestamp: this._startTime,
Expand All @@ -555,7 +569,7 @@ export class Span implements SpanInterface {
this._endTime = spanContext.endTimestamp;
// eslint-disable-next-line deprecation/deprecation
this.op = spanContext.op;
this.parentSpanId = spanContext.parentSpanId;
this._parentSpanId = spanContext.parentSpanId;
this._sampled = spanContext.sampled;
this._spanId = spanContext.spanId || this._spanId;
this._startTime = spanContext.startTimestamp || this._startTime;
Expand Down Expand Up @@ -589,7 +603,7 @@ export class Span implements SpanInterface {
data: this._getData(),
description: this._name,
op: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] as string | undefined,
parent_span_id: this.parentSpanId,
parent_span_id: this._parentSpanId,
span_id: this._spanId,
start_timestamp: this._startTime,
status: this._status,
Expand Down
5 changes: 4 additions & 1 deletion packages/core/test/lib/scope.test.ts
Expand Up @@ -7,6 +7,7 @@ import {
getCurrentScope,
getIsolationScope,
makeMain,
spanToJSON,
startInactiveSpan,
startSpan,
withIsolationScope,
Expand Down Expand Up @@ -543,12 +544,14 @@ describe('withActiveSpan()', () => {
});

it('should create child spans when calling startSpan within the callback', done => {
expect.assertions(1);
expect.assertions(2);
const inactiveSpan = startInactiveSpan({ name: 'inactive-span' });

withActiveSpan(inactiveSpan!, () => {
startSpan({ name: 'child-span' }, childSpan => {
// eslint-disable-next-line deprecation/deprecation
expect(childSpan?.parentSpanId).toBe(inactiveSpan?.spanContext().spanId);
expect(spanToJSON(childSpan!).parent_span_id).toBe(inactiveSpan?.spanContext().spanId);
done();
});
});
Expand Down
6 changes: 6 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Expand Up @@ -276,6 +276,8 @@ describe('startSpan', () => {
expect(getCurrentScope()).toBe(manualScope);
expect(getActiveSpan()).toBe(span);

expect(spanToJSON(span!).parent_span_id).toBe('parent-span-id');
// eslint-disable-next-line deprecation/deprecation
expect(span?.parentSpanId).toBe('parent-span-id');
});

Expand Down Expand Up @@ -323,6 +325,8 @@ describe('startSpanManual', () => {
expect(getCurrentScope()).not.toBe(initialScope);
expect(getCurrentScope()).toBe(manualScope);
expect(getActiveSpan()).toBe(span);
expect(spanToJSON(span!).parent_span_id).toBe('parent-span-id');
// eslint-disable-next-line deprecation/deprecation
expect(span?.parentSpanId).toBe('parent-span-id');

finish();
Expand Down Expand Up @@ -377,6 +381,8 @@ describe('startInactiveSpan', () => {
const span = startInactiveSpan({ name: 'GET users/[id]', scope: manualScope });

expect(span).toBeDefined();
expect(spanToJSON(span!).parent_span_id).toBe('parent-span-id');
// eslint-disable-next-line deprecation/deprecation
expect(span?.parentSpanId).toBe('parent-span-id');
expect(getActiveSpan()).toBeUndefined();

Expand Down
23 changes: 23 additions & 0 deletions packages/opentelemetry-node/test/spanprocessor.test.ts
Expand Up @@ -93,6 +93,8 @@ describe('SentrySpanProcessor', () => {
expect(spanToJSON(sentrySpanTransaction!).description).toBe('GET /users');
expect(spanToJSON(sentrySpanTransaction!).start_timestamp).toEqual(startTimestampMs / 1000);
expect(sentrySpanTransaction?.spanContext().traceId).toEqual(otelSpan.spanContext().traceId);
expect(spanToJSON(sentrySpanTransaction!).parent_span_id).toEqual(otelSpan.parentSpanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpanTransaction?.parentSpanId).toEqual(otelSpan.parentSpanId);
expect(sentrySpanTransaction?.spanContext().spanId).toEqual(otelSpan.spanContext().spanId);

Expand Down Expand Up @@ -121,6 +123,9 @@ describe('SentrySpanProcessor', () => {
expect(sentrySpan ? spanToJSON(sentrySpan).description : undefined).toBe('SELECT * FROM users;');
expect(spanToJSON(sentrySpan!).start_timestamp).toEqual(startTimestampMs / 1000);
expect(sentrySpan?.spanContext().spanId).toEqual(childOtelSpan.spanContext().spanId);

expect(spanToJSON(sentrySpan!).parent_span_id).toEqual(sentrySpanTransaction?.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan?.parentSpanId).toEqual(sentrySpanTransaction?.spanContext().spanId);

// eslint-disable-next-line deprecation/deprecation
Expand Down Expand Up @@ -162,6 +167,9 @@ describe('SentrySpanProcessor', () => {
expect(spanToJSON(sentrySpan!).description).toBe('SELECT * FROM users;');
expect(spanToJSON(sentrySpan!).start_timestamp).toEqual(startTimestampMs / 1000);
expect(sentrySpan?.spanContext().spanId).toEqual(childOtelSpan.spanContext().spanId);

expect(spanToJSON(sentrySpan!).parent_span_id).toEqual(parentOtelSpan.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan?.parentSpanId).toEqual(parentOtelSpan.spanContext().spanId);

// eslint-disable-next-line deprecation/deprecation
Expand Down Expand Up @@ -194,8 +202,16 @@ describe('SentrySpanProcessor', () => {
const sentrySpan2 = getSpanForOtelSpan(span2);
const sentrySpan3 = getSpanForOtelSpan(span3);

expect(spanToJSON(sentrySpan1!).parent_span_id).toEqual(sentrySpanTransaction?.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan1?.parentSpanId).toEqual(sentrySpanTransaction?.spanContext().spanId);

expect(spanToJSON(sentrySpan2!).parent_span_id).toEqual(sentrySpanTransaction?.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan2?.parentSpanId).toEqual(sentrySpanTransaction?.spanContext().spanId);

expect(spanToJSON(sentrySpan3!).parent_span_id).toEqual(sentrySpanTransaction?.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan3?.parentSpanId).toEqual(sentrySpanTransaction?.spanContext().spanId);

expect(spanToJSON(sentrySpan1!).description).toEqual('SELECT * FROM users;');
Expand Down Expand Up @@ -255,7 +271,14 @@ describe('SentrySpanProcessor', () => {
expect(childSpan).toBeInstanceOf(Transaction);
expect(spanToJSON(parentSpan!).timestamp).toBeDefined();
expect(spanToJSON(childSpan!).timestamp).toBeDefined();
expect(spanToJSON(parentSpan!).parent_span_id).toBeUndefined();

expect(spanToJSON(parentSpan!).parent_span_id).toBeUndefined();
// eslint-disable-next-line deprecation/deprecation
expect(parentSpan?.parentSpanId).toBeUndefined();

expect(spanToJSON(childSpan!).parent_span_id).toEqual(parentSpan?.spanContext().spanId);
// eslint-disable-next-line deprecation/deprecation
expect(childSpan?.parentSpanId).toEqual(parentSpan?.spanContext().spanId);
});
});
Expand Down
3 changes: 2 additions & 1 deletion packages/tracing-internal/src/browser/browsertracing.ts
Expand Up @@ -6,6 +6,7 @@ import {
addTracingExtensions,
getActiveTransaction,
spanIsSampled,
spanToJSON,
startIdleTransaction,
} from '@sentry/core';
import type { EventProcessor, Integration, Transaction, TransactionContext, TransactionSource } from '@sentry/types';
Expand Down Expand Up @@ -396,7 +397,7 @@ export class BrowserTracing implements Integration {
scope.setPropagationContext({
traceId: idleTransaction.spanContext().traceId,
spanId: idleTransaction.spanContext().spanId,
parentSpanId: idleTransaction.parentSpanId,
parentSpanId: spanToJSON(idleTransaction).parent_span_id,
sampled: spanIsSampled(idleTransaction),
});
}
Expand Down

0 comments on commit d3fecc1

Please sign in to comment.