Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
- `span.instrumenter` This field was removed and will be replaced internally.
- `span.transaction`: Use `getRootSpan` utility function instead.
- `span.spanRecorder`: Span recording will be handled internally by the SDK.
- `span.status`: Use `.setStatus` to set or update and `spanToJSON()` to read the span status.
- `transaction.setMetadata()`: Use attributes instead, or set data on the scope.
- `transaction.metadata`: Use attributes instead, or set data on the scope.
- `transaction.setContext()`: Set context on the surrounding scope instead.
Expand Down
37 changes: 26 additions & 11 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ export class Span implements SpanInterface {
*/
public parentSpanId?: string;

/**
* Internal keeper of the status
*/
public status?: SpanStatusType | string;

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -128,6 +123,8 @@ export class Span implements SpanInterface {
protected _startTime: number;
/** Epoch timestamp in seconds when the span ended. */
protected _endTime?: number;
/** Internal keeper of the status */
protected _status?: SpanStatusType | string;

private _logMessage?: string;

Expand Down Expand Up @@ -164,7 +161,7 @@ export class Span implements SpanInterface {
this.op = spanContext.op;
}
if (spanContext.status) {
this.status = spanContext.status;
this._status = spanContext.status;
}
if (spanContext.endTimestamp) {
this._endTime = spanContext.endTimestamp;
Expand Down Expand Up @@ -302,6 +299,24 @@ export class Span implements SpanInterface {
this._endTime = endTime;
}

/**
* The status of the span.
*
* @deprecated Use `spanToJSON().status` instead to get the status.
*/
public get status(): SpanStatusType | string | undefined {
return this._status;
}

/**
* The status of the span.
*
* @deprecated Use `.setStatus()` instead to set or update the status.
*/
public set status(status: SpanStatusType | string | undefined) {
this._status = status;
}

/* eslint-enable @typescript-eslint/member-ordering */

/** @inheritdoc */
Expand Down Expand Up @@ -404,7 +419,7 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public setStatus(value: SpanStatusType): this {
this.status = value;
this._status = value;
return this;
}

Expand Down Expand Up @@ -444,7 +459,7 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public isSuccess(): boolean {
return this.status === 'ok';
return this._status === 'ok';
}

/**
Expand Down Expand Up @@ -502,7 +517,7 @@ export class Span implements SpanInterface {
sampled: this._sampled,
spanId: this._spanId,
startTimestamp: this._startTime,
status: this.status,
status: this._status,
// eslint-disable-next-line deprecation/deprecation
tags: this.tags,
traceId: this._traceId,
Expand All @@ -525,7 +540,7 @@ export class Span implements SpanInterface {
this._sampled = spanContext.sampled;
this._spanId = spanContext.spanId || this._spanId;
this._startTime = spanContext.startTimestamp || this._startTime;
this.status = spanContext.status;
this._status = spanContext.status;
// eslint-disable-next-line deprecation/deprecation
this.tags = spanContext.tags || {};
this._traceId = spanContext.traceId || this._traceId;
Expand Down Expand Up @@ -558,7 +573,7 @@ export class Span implements SpanInterface {
parent_span_id: this.parentSpanId,
span_id: this._spanId,
start_timestamp: this._startTime,
status: this.status,
status: this._status,
// eslint-disable-next-line deprecation/deprecation
tags: Object.keys(this.tags).length > 0 ? this.tags : undefined,
timestamp: this._endTime,
Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import type { Hub } from '../hub';
import { getCurrentHub } from '../hub';
import { handleCallbackErrors } from '../utils/handleCallbackErrors';
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
import { spanTimeInputToSeconds } from '../utils/spanUtils';
import { spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';

interface StartSpanOptions extends TransactionContext {
/** A manually specified start time for the created `Span` object. */
Expand Down Expand Up @@ -196,8 +196,11 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
() => callback(activeSpan),
() => {
// Only update the span status if it hasn't been changed yet
if (activeSpan && (!activeSpan.status || activeSpan.status === 'ok')) {
activeSpan.setStatus('internal_error');
if (activeSpan) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
}
}
},
() => activeSpan && activeSpan.end(),
Expand Down Expand Up @@ -245,8 +248,11 @@ export function startSpanManual<T>(
() => callback(activeSpan, finishAndSetSpan),
() => {
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
if (activeSpan && activeSpan.isRecording() && (!activeSpan.status || activeSpan.status === 'ok')) {
activeSpan.setStatus('internal_error');
if (activeSpan && activeSpan.isRecording()) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
}
}
},
);
Expand Down
17 changes: 14 additions & 3 deletions packages/core/test/lib/tracing/errors.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BrowserClient } from '@sentry/browser';
import { Hub, addTracingExtensions, makeMain, startInactiveSpan, startSpan } from '@sentry/core';
import { Hub, addTracingExtensions, makeMain, spanToJSON, startInactiveSpan, startSpan } from '@sentry/core';
import type { HandlerDataError, HandlerDataUnhandledRejection } from '@sentry/types';

import { getDefaultBrowserClientOptions } from '../../../../tracing/test/testutils';
Expand Down Expand Up @@ -51,13 +51,20 @@ describe('registerErrorHandlers()', () => {
registerErrorInstrumentation();

const transaction = startInactiveSpan({ name: 'test' })!;
// eslint-disable-next-line deprecation/deprecation
expect(transaction.status).toBe(undefined);
expect(spanToJSON(transaction).status).toBe(undefined);

mockErrorCallback({} as HandlerDataError);
// eslint-disable-next-line deprecation/deprecation
expect(transaction.status).toBe(undefined);
expect(spanToJSON(transaction).status).toBe(undefined);

mockUnhandledRejectionCallback({});
// eslint-disable-next-line deprecation/deprecation
expect(transaction.status).toBe(undefined);
expect(spanToJSON(transaction).status).toBe(undefined);

transaction.end();
});

Expand All @@ -66,7 +73,9 @@ describe('registerErrorHandlers()', () => {

startSpan({ name: 'test' }, span => {
mockErrorCallback({} as HandlerDataError);
expect(span?.status).toBe('internal_error');
// eslint-disable-next-line deprecation/deprecation
expect(span!.status).toBe('internal_error');
expect(spanToJSON(span!).status).toBe('internal_error');
});
});

Expand All @@ -75,7 +84,9 @@ describe('registerErrorHandlers()', () => {

startSpan({ name: 'test' }, span => {
mockUnhandledRejectionCallback({});
expect(span?.status).toBe('internal_error');
// eslint-disable-next-line deprecation/deprecation
expect(span!.status).toBe('internal_error');
expect(spanToJSON(span!).status).toBe('internal_error');
});
});
});
2 changes: 2 additions & 0 deletions packages/node/test/handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,9 @@ describe('tracingHandler', () => {

setImmediate(() => {
expect(finishTransaction).toHaveBeenCalled();
// eslint-disable-next-line deprecation/deprecation
expect(transaction.status).toBe('ok');
expect(spanToJSON(transaction).status).toBe('ok');
// eslint-disable-next-line deprecation/deprecation
expect(transaction.tags).toEqual(expect.objectContaining({ 'http.status_code': '200' }));
expect(spanToJSON(transaction).data).toEqual(expect.objectContaining({ 'http.response.status_code': 200 }));
Expand Down
10 changes: 10 additions & 0 deletions packages/opentelemetry-node/test/spanprocessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,11 +344,15 @@ describe('SentrySpanProcessor', () => {
const transaction = getSpanForOtelSpan(otelSpan) as Transaction;

// status is only set after end
// eslint-disable-next-line deprecation/deprecation
expect(transaction?.status).toBe(undefined);
expect(spanToJSON(transaction!).status).toBe(undefined);

otelSpan.end();

// eslint-disable-next-line deprecation/deprecation
expect(transaction?.status).toBe('ok');
expect(spanToJSON(transaction!).status).toBe('ok');
});

it('sets status for span', async () => {
Expand All @@ -358,11 +362,15 @@ describe('SentrySpanProcessor', () => {
tracer.startActiveSpan('SELECT * FROM users;', child => {
const sentrySpan = getSpanForOtelSpan(child);

// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan?.status).toBe(undefined);
expect(spanToJSON(sentrySpan!).status).toBe(undefined);

child.end();

// eslint-disable-next-line deprecation/deprecation
expect(sentrySpan?.status).toBe('ok');
expect(spanToJSON(sentrySpan!).status).toBe('ok');

parentOtelSpan.end();
});
Expand Down Expand Up @@ -444,7 +452,9 @@ describe('SentrySpanProcessor', () => {
}

otelSpan.end();
// eslint-disable-next-line deprecation/deprecation
expect(transaction?.status).toBe(expected);
expect(spanToJSON(transaction!).status).toBe(expected);
},
);
});
Expand Down
4 changes: 2 additions & 2 deletions packages/tracing-internal/src/browser/backgroundtab.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { IdleTransaction, SpanStatusType } from '@sentry/core';
import { getActiveTransaction } from '@sentry/core';
import { getActiveTransaction, spanToJSON } from '@sentry/core';
import { logger } from '@sentry/utils';

import { DEBUG_BUILD } from '../common/debug-build';
Expand All @@ -23,7 +23,7 @@ export function registerBackgroundTabDetection(): void {
);
// We should not set status if it is already set, this prevent important statuses like
// error or data loss from being overwritten on transaction.
if (!activeTransaction.status) {
if (!spanToJSON(activeTransaction).status) {
activeTransaction.setStatus(statusType);
}
// TODO: Can we rewrite this to an attribute?
Expand Down
6 changes: 5 additions & 1 deletion packages/tracing-internal/test/browser/backgroundtab.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,14 @@ conditionalTest({ min: 10 })('registerBackgroundTabDetection', () => {
global.document.hidden = true;
events.visibilitychange();

const { status, timestamp } = spanToJSON(span!);

// eslint-disable-next-line deprecation/deprecation
expect(span?.status).toBe('cancelled');
expect(status).toBeDefined();
// eslint-disable-next-line deprecation/deprecation
expect(span?.tags.visibilitychange).toBe('document.hidden');
expect(spanToJSON(span!).timestamp).toBeDefined();
expect(timestamp).toBeDefined();
});
});
});
9 changes: 9 additions & 0 deletions packages/types/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,15 @@ export interface Span extends SpanContext {
*/
instrumenter: Instrumenter;

/**
* Completion status of the Span.
*
* See: {@sentry/tracing SpanStatus} for possible values
*
* @deprecated Use `.setStatus` to set or update and `spanToJSON()` to read the status.
*/
status?: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to re-declare status on the Span interface because it's inherited from SpanContext. I don't think we want to deprecate it there in v7 but tbh not sure.

Side note:
This is similar to #10189 where I realized I also didn't deprecate op on the Span interface because of this inheritance yet. I'll tackle this in a follow-up PR though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think that's fine! I'd deprecate stuff on the Span interface directly and leave the span context alone, it will go away "automatically" when all methods that use it are gone in v8!

And yeah, if we missed some there let's just add it, may very well be! 😅


/**
* Get context data for this span.
* This includes the spanId & the traceId.
Expand Down