Skip to content

Commit

Permalink
ref(span): deprecate span status enum
Browse files Browse the repository at this point in the history
  • Loading branch information
JonasBa committed Dec 15, 2021
1 parent 37a1c24 commit 55031de
Show file tree
Hide file tree
Showing 20 changed files with 138 additions and 101 deletions.
2 changes: 1 addition & 1 deletion packages/browser/test/unit/mocks/simpletransport.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { SyncPromise, statusFromHttpCode } from '@sentry/utils';
import { statusFromHttpCode,SyncPromise } from '@sentry/utils';

import { Event, Response } from '../../../src';
import { BaseTransport } from '../../../src/transports';
Expand Down
4 changes: 2 additions & 2 deletions packages/node/test/handlers.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as sentryCore from '@sentry/core';
import { Hub } from '@sentry/hub';
import * as sentryHub from '@sentry/hub';
import { SpanStatus, Transaction } from '@sentry/tracing';
import { Transaction } from '@sentry/tracing';
import { RequestSessionStatus, Runtime } from '@sentry/types';
import * as http from 'http';
import * as net from 'net';
Expand Down Expand Up @@ -403,7 +403,7 @@ describe('tracingHandler', () => {

setImmediate(() => {
expect(finishTransaction).toHaveBeenCalled();
expect(transaction.status).toBe(SpanStatus.Ok);
expect(transaction.status).toBe('ok');
expect(transaction.tags).toEqual(expect.objectContaining({ 'http.status_code': '200' }));
done();
});
Expand Down
7 changes: 4 additions & 3 deletions packages/tracing/src/browser/backgroundtab.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { SpanStatusType } from '@sentry/types';
import { getGlobalObject, logger } from '@sentry/utils';

import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from '../constants';
import { IdleTransaction } from '../idletransaction';
import { SpanStatus } from '../spanstatus';
import { getActiveTransaction } from '../utils';

const global = getGlobalObject<Window>();
Expand All @@ -16,13 +16,14 @@ export function registerBackgroundTabDetection(): void {
global.document.addEventListener('visibilitychange', () => {
const activeTransaction = getActiveTransaction() as IdleTransaction;
if (global.document.hidden && activeTransaction) {
const status: SpanStatusType = 'cancelled';
logger.log(
`[Tracing] Transaction: ${SpanStatus.Cancelled} -> since tab moved to the background, op: ${activeTransaction.op}`,
`[Tracing] Transaction: ${status} -> since tab moved to the background, op: ${activeTransaction.op}`,
);
// 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) {
activeTransaction.setStatus(SpanStatus.Cancelled);
activeTransaction.setStatus(status);
}
activeTransaction.setTag('visibilitychange', 'document.hidden');
activeTransaction.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[2]);
Expand Down
3 changes: 1 addition & 2 deletions packages/tracing/src/browser/browsertracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { getGlobalObject, logger } from '@sentry/utils';

import { startIdleTransaction } from '../hubextensions';
import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction';
import { SpanStatus } from '../spanstatus';
import { extractTraceparentData, secToMs } from '../utils';
import { registerBackgroundTabDetection } from './backgroundtab';
import { MetricsInstrumentation } from './metrics';
Expand Down Expand Up @@ -269,7 +268,7 @@ function adjustTransactionDuration(maxDuration: number, transaction: IdleTransac
const diff = endTimestamp - transaction.startTimestamp;
const isOutdatedTransaction = endTimestamp && (diff > maxDuration || diff < 0);
if (isOutdatedTransaction) {
transaction.setStatus(SpanStatus.DeadlineExceeded);
transaction.setStatus('deadline_exceeded');
transaction.setTag('maxTransactionDurationExceeded', 'true');
}
}
3 changes: 1 addition & 2 deletions packages/tracing/src/browser/request.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { addInstrumentationHandler, isInstanceOf, isMatchingPattern } from '@sentry/utils';

import { Span } from '../span';
import { SpanStatus } from '../spanstatus';
import { getActiveTransaction, hasTracingEnabled } from '../utils';

export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//];
Expand Down Expand Up @@ -156,7 +155,7 @@ export function fetchCallback(
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
span.setHttpStatus(handlerData.response.status);
} else if (handlerData.error) {
span.setStatus(SpanStatus.InternalError);
span.setStatus('internal_error');
}
span.finish();

Expand Down
7 changes: 4 additions & 3 deletions packages/tracing/src/errors.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { SpanStatusType } from '@sentry/types';
import { addInstrumentationHandler, logger } from '@sentry/utils';

import { SpanStatus } from './spanstatus';
import { getActiveTransaction } from './utils';

/**
Expand All @@ -23,7 +23,8 @@ export function registerErrorInstrumentation(): void {
function errorCallback(): void {
const activeTransaction = getActiveTransaction();
if (activeTransaction) {
logger.log(`[Tracing] Transaction: ${SpanStatus.InternalError} -> Global error occured`);
activeTransaction.setStatus(SpanStatus.InternalError);
const errorType: SpanStatusType = 'internal_error';
logger.log(`[Tracing] Transaction: ${errorType} -> Global error occured`);
activeTransaction.setStatus(errorType);
}
}
5 changes: 2 additions & 3 deletions packages/tracing/src/idletransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { logger, timestampWithMs } from '@sentry/utils';

import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from './constants';
import { Span, SpanRecorder } from './span';
import { SpanStatus } from './spanstatus';
import { Transaction } from './transaction';

export const DEFAULT_IDLE_TIMEOUT = 1000;
Expand Down Expand Up @@ -125,7 +124,7 @@ export class IdleTransaction extends Transaction {
// We cancel all pending spans with status "cancelled" to indicate the idle transaction was finished early
if (!span.endTimestamp) {
span.endTimestamp = endTimestamp;
span.setStatus(SpanStatus.Cancelled);
span.setStatus('cancelled');
logger.log('[Tracing] cancelling span since transaction ended early', JSON.stringify(span, undefined, 2));
}

Expand Down Expand Up @@ -253,7 +252,7 @@ export class IdleTransaction extends Transaction {

if (this._heartbeatCounter >= 3) {
logger.log(`[Tracing] Transaction finished because of no change for 3 heart beats`);
this.setStatus(SpanStatus.DeadlineExceeded);
this.setStatus('deadline_exceeded');
this.setTag(FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS[0]);
this.finish();
} else {
Expand Down
1 change: 1 addition & 0 deletions packages/tracing/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export {
RequestInstrumentationOptions,
defaultRequestInstrumentationOptions,
} from './browser';
/* eslint-disable-next-line deprecation/deprecation */
export { SpanStatus } from './spanstatus';
export { IdleTransaction } from './idletransaction';
export { startIdleTransaction } from './hubextensions';
Expand Down
16 changes: 7 additions & 9 deletions packages/tracing/src/span.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
/* eslint-disable max-lines */
import { Primitive, Span as SpanInterface, SpanContext, Transaction } from '@sentry/types';
import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils';

import { SpanStatus } from './spanstatus';
import { Primitive, Span as SpanInterface, SpanContext, SpanStatusType, Transaction } from '@sentry/types';
import { dropUndefinedKeys, spanStatusfromHttpCode, timestampWithMs, uuid4 } from '@sentry/utils';

/**
* Keeps track of finished spans for a given transaction
Expand Down Expand Up @@ -56,7 +54,7 @@ export class Span implements SpanInterface {
/**
* Internal keeper of the status
*/
public status?: SpanStatus | string;
public status?: SpanStatusType;

/**
* @inheritDoc
Expand Down Expand Up @@ -204,7 +202,7 @@ export class Span implements SpanInterface {
/**
* @inheritDoc
*/
public setStatus(value: SpanStatus): this {
public setStatus(value: SpanStatusType): this {
this.status = value;
return this;
}
Expand All @@ -214,8 +212,8 @@ export class Span implements SpanInterface {
*/
public setHttpStatus(httpStatus: number): this {
this.setTag('http.status_code', String(httpStatus));
const spanStatus = SpanStatus.fromHttpCode(httpStatus);
if (spanStatus !== SpanStatus.UnknownError) {
const spanStatus = spanStatusfromHttpCode(httpStatus);
if (spanStatus !== 'unknown_error') {
this.setStatus(spanStatus);
}
return this;
Expand All @@ -225,7 +223,7 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public isSuccess(): boolean {
return this.status === SpanStatus.Ok;
return this.status === 'ok';
}

/**
Expand Down
54 changes: 3 additions & 51 deletions packages/tracing/src/spanstatus.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/** The status of an Span. */
// eslint-disable-next-line import/export
/** JSDoc
* @deprecated Use string literals - if you require type casting, cast to SpanStatusType type
*/
export enum SpanStatus {
/** The operation completed successfully. */
Ok = 'ok',
Expand Down Expand Up @@ -36,52 +37,3 @@ export enum SpanStatus {
/** Unrecoverable data loss or corruption */
DataLoss = 'data_loss',
}

// eslint-disable-next-line @typescript-eslint/no-namespace, import/export
export namespace SpanStatus {
/**
* Converts a HTTP status code into a {@link SpanStatus}.
*
* @param httpStatus The HTTP response status code.
* @returns The span status or {@link SpanStatus.UnknownError}.
*/
export function fromHttpCode(httpStatus: number): SpanStatus {
if (httpStatus < 400 && httpStatus >= 100) {
return SpanStatus.Ok;
}

if (httpStatus >= 400 && httpStatus < 500) {
switch (httpStatus) {
case 401:
return SpanStatus.Unauthenticated;
case 403:
return SpanStatus.PermissionDenied;
case 404:
return SpanStatus.NotFound;
case 409:
return SpanStatus.AlreadyExists;
case 413:
return SpanStatus.FailedPrecondition;
case 429:
return SpanStatus.ResourceExhausted;
default:
return SpanStatus.InvalidArgument;
}
}

if (httpStatus >= 500 && httpStatus < 600) {
switch (httpStatus) {
case 501:
return SpanStatus.Unimplemented;
case 503:
return SpanStatus.Unavailable;
case 504:
return SpanStatus.DeadlineExceeded;
default:
return SpanStatus.InternalError;
}
}

return SpanStatus.UnknownError;
}
}
5 changes: 3 additions & 2 deletions packages/tracing/test/browser/backgroundtab.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { BrowserClient } from '@sentry/browser';
import { Hub, makeMain } from '@sentry/hub';
import { JSDOM } from 'jsdom';

import { SpanStatus } from '../../src';
import { SpanStatusType } from '../../../types/src/span';
import { registerBackgroundTabDetection } from '../../src/browser/backgroundtab';

describe('registerBackgroundTabDetection', () => {
Expand Down Expand Up @@ -49,7 +49,8 @@ describe('registerBackgroundTabDetection', () => {
global.document.hidden = true;
events.visibilitychange();

expect(transaction.status).toBe(SpanStatus.Cancelled);
const status: SpanStatusType = 'cancelled';
expect(transaction.status).toBe(status);
expect(transaction.tags.visibilitychange).toBe('document.hidden');
expect(transaction.endTimestamp).toBeDefined();
});
Expand Down
5 changes: 3 additions & 2 deletions packages/tracing/test/browser/browsertracing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Hub, makeMain } from '@sentry/hub';
import { getGlobalObject } from '@sentry/utils';
import { JSDOM } from 'jsdom';

import { SpanStatus } from '../../src';
import { SpanStatusType } from '../../../types/src/span';
import {
BrowserTracing,
BrowserTracingOptions,
Expand Down Expand Up @@ -273,7 +273,8 @@ describe('BrowserTracing', () => {
const transaction = getActiveTransaction(hub) as IdleTransaction;
transaction.finish(transaction.startTimestamp + secToMs(DEFAULT_MAX_TRANSACTION_DURATION_SECONDS) + 1);

expect(transaction.status).toBe(SpanStatus.DeadlineExceeded);
const status: SpanStatusType = 'deadline_exceeded';
expect(transaction.status).toBe(status);
expect(transaction.tags.maxTransactionDurationExceeded).toBeDefined();
});

Expand Down
6 changes: 3 additions & 3 deletions packages/tracing/test/browser/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { BrowserClient } from '@sentry/browser';
import { Hub, makeMain } from '@sentry/hub';
import * as utils from '@sentry/utils';

import { Span, SpanStatus, Transaction } from '../../src';
import { Span, Transaction } from '../../src';
import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback, XHRData } from '../../src/browser/request';
import { addExtensionMethods } from '../../src/hubextensions';
import * as tracingUtils from '../../src/utils';
Expand Down Expand Up @@ -176,7 +176,7 @@ describe('callbacks', () => {
// triggered by response coming back
fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, spans);

expect(newSpan!.status).toBe(SpanStatus.fromHttpCode(404));
expect(newSpan!.status).toBe(utils.spanStatusfromHttpCode(404));
});

it('adds sentry-trace header to fetch requests', () => {
Expand Down Expand Up @@ -267,7 +267,7 @@ describe('callbacks', () => {
// triggered by response coming back
xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, spans);

expect(newSpan!.status).toBe(SpanStatus.fromHttpCode(404));
expect(newSpan!.status).toBe(utils.spanStatusfromHttpCode(404));
});
});
});
8 changes: 5 additions & 3 deletions packages/tracing/test/errors.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { BrowserClient } from '@sentry/browser';
import { Hub, makeMain } from '@sentry/hub';

import { SpanStatus } from '../src';
import { SpanStatusType } from '../../types/src/span';
import { registerErrorInstrumentation } from '../src/errors';
import { _addTracingExtensions } from '../src/hubextensions';

Expand Down Expand Up @@ -71,7 +71,8 @@ describe('registerErrorHandlers()', () => {
hub.configureScope(scope => scope.setSpan(transaction));

mockErrorCallback();
expect(transaction.status).toBe(SpanStatus.InternalError);
const status: SpanStatusType = 'internal_error';
expect(transaction.status).toBe(status);

transaction.finish();
});
Expand All @@ -82,7 +83,8 @@ describe('registerErrorHandlers()', () => {
hub.configureScope(scope => scope.setSpan(transaction));

mockUnhandledRejectionCallback();
expect(transaction.status).toBe(SpanStatus.InternalError);
const status: SpanStatusType = 'internal_error';
expect(transaction.status).toBe(status);
transaction.finish();
});
});
15 changes: 8 additions & 7 deletions packages/tracing/test/idletransaction.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BrowserClient, Transports } from '@sentry/browser';
import { Hub } from '@sentry/hub';
import { Outcome } from '@sentry/types';
import { Outcome, SpanStatusType } from '@sentry/types';

import {
DEFAULT_IDLE_TIMEOUT,
Expand All @@ -9,7 +9,6 @@ import {
IdleTransactionSpanRecorder,
} from '../src/idletransaction';
import { Span } from '../src/span';
import { SpanStatus } from '../src/spanstatus';

export class SimpleTransport extends Transports.BaseTransport {}

Expand Down Expand Up @@ -160,7 +159,8 @@ describe('IdleTransaction', () => {

// Cancelled Span - has endtimestamp of transaction
expect(spans[2].spanId).toBe(cancelledSpan.spanId);
expect(spans[2].status).toBe(SpanStatus.Cancelled);
const status: SpanStatusType = 'cancelled';
expect(spans[2].status).toBe(status);
expect(spans[2].endTimestamp).toBe(transaction.endTimestamp);
}
});
Expand Down Expand Up @@ -203,22 +203,23 @@ describe('IdleTransaction', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 20000);
const mockFinish = jest.spyOn(transaction, 'finish');

expect(transaction.status).not.toEqual(SpanStatus.DeadlineExceeded);
const status: SpanStatusType = 'deadline_exceeded';
expect(transaction.status).not.toEqual(status);
expect(mockFinish).toHaveBeenCalledTimes(0);

// Beat 1
jest.advanceTimersByTime(HEARTBEAT_INTERVAL);
expect(transaction.status).not.toEqual(SpanStatus.DeadlineExceeded);
expect(transaction.status).not.toEqual(status);
expect(mockFinish).toHaveBeenCalledTimes(0);

// Beat 2
jest.advanceTimersByTime(HEARTBEAT_INTERVAL);
expect(transaction.status).not.toEqual(SpanStatus.DeadlineExceeded);
expect(transaction.status).not.toEqual(status);
expect(mockFinish).toHaveBeenCalledTimes(0);

// Beat 3
jest.advanceTimersByTime(HEARTBEAT_INTERVAL);
expect(transaction.status).not.toEqual(SpanStatus.DeadlineExceeded);
expect(transaction.status).not.toEqual(status);
expect(mockFinish).toHaveBeenCalledTimes(0);
});

Expand Down
Loading

0 comments on commit 55031de

Please sign in to comment.