Skip to content

feat(core): Remove deprecated props from Span interface #10854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 29, 2024
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
28 changes: 13 additions & 15 deletions packages/astro/test/server/meta.test.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,23 @@
import * as SentryCore from '@sentry/core';
import { SentrySpan } from '@sentry/core';
import type { Transaction } from '@sentry/types';
import { vi } from 'vitest';

import { getTracingMetaTags, isValidBaggageString } from '../../src/server/meta';

const TRACE_FLAG_SAMPLED = 0x1;

const mockedSpan = {
isRecording: () => true,
spanContext: () => {
return {
traceId: '12345678901234567890123456789012',
spanId: '1234567890123456',
traceFlags: TRACE_FLAG_SAMPLED,
};
},
transaction: {
getDynamicSamplingContext: () => ({
environment: 'production',
}),
},
} as any;
const mockedSpan = new SentrySpan({
traceId: '12345678901234567890123456789012',
spanId: '1234567890123456',
sampled: true,
});
// eslint-disable-next-line deprecation/deprecation
mockedSpan.transaction = {
getDynamicSamplingContext: () => ({
environment: 'production',
}),
} as Transaction;

const mockedClient = {} as any;

Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
import { dateTimestampInSeconds, isPlainObject, logger, uuid4 } from '@sentry/utils';

import { updateSession } from './session';
import type { SentrySpan } from './tracing/sentrySpan';

/**
* Default value for maximum number of breadcrumbs added to an event.
Expand Down Expand Up @@ -329,10 +330,15 @@ export class Scope implements ScopeInterface {
// Often, this span (if it exists at all) will be a transaction, but it's not guaranteed to be. Regardless, it will
// have a pointer to the currently-active transaction.
const span = this._span;

// Cannot replace with getRootSpan because getRootSpan returns a span, not a transaction
// Also, this method will be removed anyway.
// eslint-disable-next-line deprecation/deprecation
return span && span.transaction;
if (span && (span as SentrySpan).transaction) {
// eslint-disable-next-line deprecation/deprecation
return (span as SentrySpan).transaction;
}
return undefined;
}

/**
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/tracing/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { getActiveTransaction } from './utils';

let errorsInstrumented = false;

/** Only exposed for testing */
export function _resetErrorsInstrumented(): void {
errorsInstrumented = false;
}

/**
* Configures global error listeners
*/
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type {
Primitive,
Span as SpanInterface,
Span,
SpanAttributeValue,
SpanAttributes,
SpanContext,
Expand Down Expand Up @@ -62,7 +62,7 @@ export class SpanRecorder {
/**
* Span contains all data about a span
*/
export class SentrySpan implements SpanInterface {
export class SentrySpan implements Span {
/**
* Tags for the span.
* @deprecated Use `spanToJSON(span).atttributes` instead.
Expand Down Expand Up @@ -277,7 +277,7 @@ export class SentrySpan implements SpanInterface {
*/
public startChild(
spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'sampled' | 'traceId' | 'parentSpanId'>>,
): SpanInterface {
): Span {
const childSpan = new SentrySpan({
...spanContext,
parentSpanId: this._spanId,
Expand Down
12 changes: 6 additions & 6 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { Hub, Scope, Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';

import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';

import { getCurrentScope, getIsolationScope, withScope } from '../currentScopes';

import { DEBUG_BUILD } from '../debug-build';
Expand All @@ -10,6 +9,7 @@ import { handleCallbackErrors } from '../utils/handleCallbackErrors';
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
import { spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
import type { SentrySpan } from './sentrySpan';
import { addChildSpanToSpan, getActiveSpan, setCapturedScopesOnSpan } from './utils';

/**
Expand All @@ -30,7 +30,7 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();
const parentSpan = scope.getSpan() as SentrySpan | undefined;

const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan
Expand Down Expand Up @@ -79,7 +79,7 @@ export function startSpanManual<T>(
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();
const parentSpan = scope.getSpan() as SentrySpan | undefined;

const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan
Expand Down Expand Up @@ -130,8 +130,8 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
const hub = getCurrentHub();
const parentSpan = context.scope
? // eslint-disable-next-line deprecation/deprecation
context.scope.getSpan()
: getActiveSpan();
(context.scope.getSpan() as SentrySpan | undefined)
: (getActiveSpan() as SentrySpan | undefined);

const shouldSkipSpan = context.onlyIfParent && !parentSpan;

Expand Down Expand Up @@ -264,7 +264,7 @@ function createChildSpanOrTransaction(
forceTransaction,
scope,
}: {
parentSpan: Span | undefined;
parentSpan: SentrySpan | undefined;
spanContext: TransactionContext;
forceTransaction?: boolean;
scope: Scope;
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/utils/getRootSpan.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Span } from '@sentry/types';
import type { SentrySpan } from './../tracing/sentrySpan';

/**
* Returns the root span of a given span.
Expand All @@ -11,5 +12,5 @@ import type { Span } from '@sentry/types';
export function getRootSpan(span: Span): Span | undefined {
// TODO (v8): Remove this check and just return span
// eslint-disable-next-line deprecation/deprecation
return span.transaction;
return (span as SentrySpan).transaction ? (span as SentrySpan).transaction : undefined;
}
3 changes: 1 addition & 2 deletions packages/core/src/utils/prepareEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,8 +332,7 @@ function normalizeEvent(event: Event | null, depth: number, maxBreadth: number):

if (data) {
// This is a bit weird, as we generally have `Span` instances here, but to be safe we do not assume so
// eslint-disable-next-line deprecation/deprecation
span.data = normalize(data, depth, maxBreadth);
span.setAttributes(normalize(data, depth, maxBreadth));
}

return span;
Expand Down
11 changes: 6 additions & 5 deletions packages/core/src/utils/spanUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,27 +62,28 @@ function ensureTimestampInSeconds(timestamp: number): number {
return isMs ? timestamp / 1000 : timestamp;
}

type SpanWithToJSON = Span & { toJSON: () => SpanJSON };

/**
* 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<SpanJSON> {
if (spanIsSentrySpan(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();
if (typeof (span as SpanWithToJSON).toJSON === 'function') {
return (span as SpanWithToJSON).toJSON();
}

// TODO: Also handle OTEL spans here!

return {};
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/lib/hint.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { captureEvent, getCurrentScope } from '@sentry/core';
import { GLOBAL_OBJ } from '@sentry/utils';

import { captureEvent, getCurrentScope } from '../../src';
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually fixed a bunch of tests along there way here we we used to import from the package itself (we are in core here), which lead to tests only running on the built output, which is a bit weird IMHO.

import { initAndBind } from '../../src/sdk';
import { TestClient, getDefaultTestClientOptions } from '../mocks/client';
import { AddAttachmentTestIntegration } from '../mocks/integration';
Expand Down
5 changes: 2 additions & 3 deletions packages/core/test/lib/integrations/requestdata.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import type { IncomingMessage } from 'http';
import type { RequestDataIntegrationOptions } from '@sentry/core';
import { setCurrentClient } from '@sentry/core';
import { RequestData } from '@sentry/core';
import type { Event, EventProcessor } from '@sentry/types';
import * as sentryUtils from '@sentry/utils';
import type { RequestDataIntegrationOptions } from '../../../src';
import { RequestData, setCurrentClient } from '../../../src';

import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

Expand Down
14 changes: 6 additions & 8 deletions packages/core/test/lib/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -961,18 +961,16 @@ describe('withActiveSpan()', () => {
});
});

it('should create child spans when calling startSpan within the callback', done => {
expect.assertions(2);
it('should create child spans when calling startSpan within the callback', () => {
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();
const parentSpanId = withActiveSpan(inactiveSpan!, () => {
return startSpan({ name: 'child-span' }, childSpan => {
return spanToJSON(childSpan!).parent_span_id;
});
});

expect(parentSpanId).toBe(inactiveSpan?.spanContext().spanId);
});

it('when `null` is passed, no span should be active within the callback', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/lib/sdk.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { captureCheckIn, getCurrentScope, setCurrentClient } from '@sentry/core';
import type { Client, Integration, IntegrationFnResult } from '@sentry/types';
import { captureCheckIn, getCurrentScope, setCurrentClient } from '../../src';

import { installedIntegrations } from '../../src/integration';
import { initAndBind } from '../../src/sdk';
Expand Down
6 changes: 4 additions & 2 deletions packages/core/test/lib/tracing/errors.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { addTracingExtensions, setCurrentClient, spanToJSON, startInactiveSpan, startSpan } from '@sentry/core';
import type { HandlerDataError, HandlerDataUnhandledRejection } from '@sentry/types';
import { addTracingExtensions, setCurrentClient, spanToJSON, startInactiveSpan, startSpan } from '../../../src';

import { registerErrorInstrumentation } from '../../../src/tracing/errors';
import { _resetErrorsInstrumented, registerErrorInstrumentation } from '../../../src/tracing/errors';
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

const mockAddGlobalErrorInstrumentationHandler = jest.fn();
const mockAddGlobalUnhandledRejectionInstrumentationHandler = jest.fn();
let mockErrorCallback: (data: HandlerDataError) => void = () => {};
let mockUnhandledRejectionCallback: (data: HandlerDataUnhandledRejection) => void = () => {};

jest.mock('@sentry/utils', () => {
const actual = jest.requireActual('@sentry/utils');
return {
Expand Down Expand Up @@ -36,6 +37,7 @@ describe('registerErrorHandlers()', () => {
const client = new TestClient(options);
setCurrentClient(client);
client.init();
_resetErrorsInstrumented();
Copy link
Member Author

Choose a reason for hiding this comment

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

This apparently was not needed before because we imported this from the build output 🤔 now it should be more robust.

});

it('registers error instrumentation', () => {
Expand Down
13 changes: 8 additions & 5 deletions packages/core/test/lib/tracing/idletransaction.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
/* eslint-disable deprecation/deprecation */
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

import {
IdleTransaction,
SentrySpan,
TRACING_DEFAULTS,
Transaction,
getClient,
getCurrentHub,
getCurrentScope,
getGlobalScope,
Expand All @@ -10,11 +16,7 @@ import {
startInactiveSpan,
startSpan,
startSpanManual,
} from '@sentry/core';
/* eslint-disable deprecation/deprecation */
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

import { IdleTransaction, SentrySpan, getClient } from '../../../src';
} from '../../../src';
import { IdleTransactionSpanRecorder } from '../../../src/tracing/idletransaction';

const dsn = 'https://[email protected]/42';
Expand Down Expand Up @@ -47,6 +49,7 @@ describe('IdleTransaction', () => {
transaction.initSpanRecorder(10);

const scope = getCurrentScope();

// eslint-disable-next-line deprecation/deprecation
expect(scope.getTransaction()).toBe(transaction);
});
Expand Down
7 changes: 0 additions & 7 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,7 @@ describe('startSpan', () => {
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');
});

expect(getCurrentScope()).toBe(initialScope);
Expand Down Expand Up @@ -565,8 +562,6 @@ describe('startSpanManual', () => {
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 @@ -789,8 +784,6 @@ describe('startInactiveSpan', () => {

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();

span?.end();
Expand Down
4 changes: 2 additions & 2 deletions packages/node/src/integrations/http.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable max-lines */
import type * as http from 'http';
import type * as https from 'https';
import type { Hub } from '@sentry/core';
import type { Hub, SentrySpan } from '@sentry/core';
import { defineIntegration, getIsolationScope, hasTracingEnabled } from '@sentry/core';
import {
addBreadcrumb,
Expand Down Expand Up @@ -319,7 +319,7 @@ function _createWrappedRequestMethodFactory(

const scope = getCurrentScope();
const isolationScope = getIsolationScope();
const parentSpan = getActiveSpan();
const parentSpan = getActiveSpan() as SentrySpan;

const data = getRequestSpanData(requestUrl, requestOptions);

Expand Down
5 changes: 3 additions & 2 deletions packages/node/src/integrations/undici/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { SentrySpan } from '@sentry/core';
import {
addBreadcrumb,
defineIntegration,
Expand Down Expand Up @@ -183,7 +184,7 @@ export class Undici implements Integration {
const clientOptions = client.getOptions();
const scope = getCurrentScope();
const isolationScope = getIsolationScope();
const parentSpan = getActiveSpan();
const parentSpan = getActiveSpan() as SentrySpan;

const span = this._shouldCreateSpan(stringUrl) ? createRequestSpan(parentSpan, request, stringUrl) : undefined;
if (span) {
Expand Down Expand Up @@ -320,7 +321,7 @@ function setHeadersOnRequest(
}

function createRequestSpan(
activeSpan: Span | undefined,
activeSpan: SentrySpan | undefined,
request: RequestWithSentry,
stringUrl: string,
): Span | undefined {
Expand Down
Loading