Skip to content
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

feat(core): Deprecate Span.transaction in favor of getRootSpan #10134

Merged
merged 7 commits into from
Jan 11, 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
1 change: 1 addition & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
- `span.setTag()`: Use `span.setAttribute()` instead or set tags on the surrounding scope.
- `span.setData()`: Use `span.setAttribute()` instead.
- `span.instrumenter` This field was removed and will be replaced internally.
- `span.transaction`: Use `getRootSpan` utility function instead.
- `transaction.setContext()`: Set context on the surrounding scope instead.

## Deprecate `pushScope` & `popScope` in favor of `withScope`
Expand Down
7 changes: 4 additions & 3 deletions packages/astro/src/server/meta.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
spanToTraceHeader,
} from '@sentry/core';
import type { Client, Scope, Span } from '@sentry/types';
Expand Down Expand Up @@ -32,12 +33,12 @@ export function getTracingMetaTags(
client: Client | undefined,
): { sentryTrace: string; baggage?: string } {
const { dsc, sampled, traceId } = scope.getPropagationContext();
const transaction = span?.transaction;
const rootSpan = span && getRootSpan(span);

const sentryTrace = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, undefined, sampled);

const dynamicSamplingContext = transaction
? getDynamicSamplingContextFromSpan(transaction)
const dynamicSamplingContext = rootSpan
? getDynamicSamplingContextFromSpan(rootSpan)
: dsc
? dsc
: client
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export {
spanToJSON,
spanIsSampled,
} from './utils/spanUtils';
export { getRootSpan } from './utils/getRootSpan';
export { DEFAULT_ENVIRONMENT } from './constants';
export { ModuleMetadata } from './integrations/metadata';
export { RequestData } from './integrations/requestdata';
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,9 @@ 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;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/server-runtime-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
} from './tracing';
import { getRootSpan } from './utils/getRootSpan';
import { spanToTraceContext } from './utils/spanUtils';

export interface ServerRuntimeClientOptions extends ClientOptions<BaseTransportOptions> {
Expand Down Expand Up @@ -262,7 +263,7 @@ export class ServerRuntimeClient<
// eslint-disable-next-line deprecation/deprecation
const span = scope.getSpan();
if (span) {
const samplingContext = span.transaction ? getDynamicSamplingContextFromSpan(span) : undefined;
const samplingContext = getRootSpan(span) ? getDynamicSamplingContextFromSpan(span) : undefined;
return [samplingContext, spanToTraceContext(span)];
}

Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/tracing/dynamicSamplingContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { dropUndefinedKeys } from '@sentry/utils';

import { DEFAULT_ENVIRONMENT } from '../constants';
import { getClient, getCurrentScope } from '../exports';
import { getRootSpan } from '../utils/getRootSpan';
import { spanIsSampled, spanToJSON } from '../utils/spanUtils';

/**
Expand Down Expand Up @@ -54,9 +55,8 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
// passing emit=false here to only emit later once the DSC is actually populated
const dsc = getDynamicSamplingContextFromClient(spanToJSON(span).trace_id || '', client, getCurrentScope());

// As long as we use `Transaction`s internally, this should be fine.
// TODO: We need to replace this with a `getRootSpan(span)` function though
const txn = span.transaction as TransactionWithV7FrozenDsc | undefined;
// TODO (v8): Remove v7FrozenDsc as a Transaction will no longer have _frozenDynamicSamplingContext
const txn = getRootSpan(span) as TransactionWithV7FrozenDsc | undefined;
if (!txn) {
return dsc;
}
Expand Down
17 changes: 12 additions & 5 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { getRootSpan } from '../utils/getRootSpan';
import {
TRACE_FLAG_NONE,
TRACE_FLAG_SAMPLED,
Expand Down Expand Up @@ -105,6 +106,7 @@ export class Span implements SpanInterface {

/**
* @inheritDoc
* @deprecated Use top level `Sentry.getRootSpan()` instead
*/
public transaction?: Transaction;

Expand Down Expand Up @@ -304,12 +306,16 @@ export class Span implements SpanInterface {
childSpan.spanRecorder.add(childSpan);
}

childSpan.transaction = this.transaction;
const rootSpan = getRootSpan(this);
// TODO: still set span.transaction here until we have a more permanent solution
// Probably similarly to the weakmap we hold in node-experimental
// eslint-disable-next-line deprecation/deprecation
childSpan.transaction = rootSpan as Transaction;

if (DEBUG_BUILD && childSpan.transaction) {
if (DEBUG_BUILD && rootSpan) {
const opStr = (spanContext && spanContext.op) || '< unknown op >';
const nameStr = spanToJSON(childSpan).description || '< unknown name >';
const idStr = childSpan.transaction.spanContext().spanId;
const idStr = rootSpan.spanContext().spanId;

const logMessage = `[Tracing] Starting '${opStr}' span on transaction '${nameStr}' (${idStr}).`;
logger.log(logMessage);
Expand Down Expand Up @@ -416,11 +422,12 @@ export class Span implements SpanInterface {

/** @inheritdoc */
public end(endTimestamp?: SpanTimeInput): void {
const rootSpan = getRootSpan(this);
if (
DEBUG_BUILD &&
// Don't call this for transactions
this.transaction &&
this.transaction.spanContext().spanId !== this._spanId
rootSpan &&
rootSpan.spanContext().spanId !== this._spanId
) {
const logMessage = this._logMessage;
if (logMessage) {
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
this._trimEnd = transactionContext.trimEnd;

// this is because transactions are also spans, and spans have a transaction pointer
// TODO (v8): Replace this with another way to set the root span
Copy link
Member

Choose a reason for hiding this comment

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

Is there even a todo for v8 here? Won't we get rid of the transaction class?

Copy link
Member Author

@Lms24 Lms24 Jan 10, 2024

Choose a reason for hiding this comment

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

Hmm I mean we somehow still need to construct a transaction for the event payload so I guess it's not gonna vanish completely. We'll just need some way to set and find the root span/txn of a span in v8.

// eslint-disable-next-line deprecation/deprecation
this.transaction = this;

// If Dynamic Sampling Context is provided during the creation of the transaction, we freeze it as it usually means
Expand Down
7 changes: 4 additions & 3 deletions packages/core/src/utils/applyScopeDataToEvent.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { Breadcrumb, Event, PropagationContext, ScopeData, Span } from '@sentry/types';
import { arrayify } from '@sentry/utils';
import { getDynamicSamplingContextFromSpan } from '../tracing/dynamicSamplingContext';
import { getRootSpan } from './getRootSpan';
import { spanToJSON, spanToTraceContext } from './spanUtils';

/**
Expand Down Expand Up @@ -174,13 +175,13 @@ function applySdkMetadataToEvent(

function applySpanToEvent(event: Event, span: Span): void {
event.contexts = { trace: spanToTraceContext(span), ...event.contexts };
const transaction = span.transaction;
if (transaction) {
const rootSpan = getRootSpan(span);
if (rootSpan) {
event.sdkProcessingMetadata = {
dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
...event.sdkProcessingMetadata,
};
const transactionName = spanToJSON(transaction).description;
const transactionName = spanToJSON(rootSpan).description;
if (transactionName) {
event.tags = { transaction: transactionName, ...event.tags };
}
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/utils/getRootSpan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import type { Span } from '@sentry/types';

/**
* Returns the root span of a given span.
*
* As long as we use `Transaction`s internally, the returned root span
* will be a `Transaction` but be aware that this might change in the future.
*
* If the given span has no root span or transaction, `undefined` is returned.
*/
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;
}
11 changes: 5 additions & 6 deletions packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
});

test('returns the DSC provided during transaction creation', () => {
// eslint-disable-next-line deprecation/deprecation
// eslint-disable-next-line deprecation/deprecation -- using old API on purpose
const transaction = new Transaction({
name: 'tx',
metadata: { dynamicSamplingContext: { environment: 'myEnv' } },
Expand Down Expand Up @@ -68,7 +68,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
});

test('returns a new DSC, if no DSC was provided during transaction creation (via new Txn and deprecated metadata)', () => {
// eslint-disable-next-line deprecation/deprecation
// eslint-disable-next-line deprecation/deprecation -- using old API on purpose
const transaction = new Transaction({
name: 'tx',
metadata: {
Expand All @@ -92,7 +92,7 @@ describe('getDynamicSamplingContextFromSpan', () => {

describe('Including transaction name in DSC', () => {
test('is not included if transaction source is url', () => {
// eslint-disable-next-line deprecation/deprecation
// eslint-disable-next-line deprecation/deprecation -- using old API on purpose
const transaction = new Transaction({
name: 'tx',
metadata: {
Expand All @@ -109,8 +109,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
['is included if transaction source is parameterized route/url', 'route'],
['is included if transaction source is a custom name', 'custom'],
])('%s', (_: string, source) => {
// eslint-disable-next-line deprecation/deprecation
const transaction = new Transaction({
const transaction = startInactiveSpan({
name: 'tx',
metadata: {
...(source && { source: source as TransactionSource }),
Expand All @@ -120,7 +119,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
// Only setting the attribute manually because we're directly calling new Transaction()
transaction?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);

const dsc = getDynamicSamplingContextFromSpan(transaction);
const dsc = getDynamicSamplingContextFromSpan(transaction!);

expect(dsc.transaction).toEqual('tx');
});
Expand Down
36 changes: 36 additions & 0 deletions packages/core/test/lib/utils/getRootSpan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { Span, Transaction, getRootSpan } from '../../../src';

describe('getRootSpan', () => {
it('returns the root span of a span (Span)', () => {
const root = new Span({ name: 'test' });
// @ts-expect-error this is highly illegal and shouldn't happen IRL
// eslint-disable-next-line deprecation/deprecation
root.transaction = root;

// eslint-disable-next-line deprecation/deprecation
const childSpan = root.startChild({ name: 'child' });
expect(getRootSpan(childSpan)).toBe(root);
});

it('returns the root span of a span (Transaction)', () => {
// eslint-disable-next-line deprecation/deprecation
const root = new Transaction({ name: 'test' });

// eslint-disable-next-line deprecation/deprecation
const childSpan = root.startChild({ name: 'child' });
expect(getRootSpan(childSpan)).toBe(root);
});

it('returns the span itself if it is a root span', () => {
// eslint-disable-next-line deprecation/deprecation
const span = new Transaction({ name: 'test' });

expect(getRootSpan(span)).toBe(span);
});

it('returns undefined if span has no root span', () => {
const span = new Span({ name: 'test' });

expect(getRootSpan(span)).toBe(undefined);
});
});
4 changes: 2 additions & 2 deletions packages/opentelemetry-node/src/propagator.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { Baggage, Context, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
import { TraceFlags, isSpanContextValid, propagation, trace } from '@opentelemetry/api';
import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
import { getDynamicSamplingContextFromSpan, spanToTraceHeader } from '@sentry/core';
import { getDynamicSamplingContextFromSpan, getRootSpan, spanToTraceHeader } from '@sentry/core';
import {
SENTRY_BAGGAGE_KEY_PREFIX,
baggageHeaderToDynamicSamplingContext,
Expand Down Expand Up @@ -35,7 +35,7 @@ export class SentryPropagator extends W3CBaggagePropagator {
if (span) {
setter.set(carrier, SENTRY_TRACE_HEADER, spanToTraceHeader(span));

if (span.transaction) {
if (getRootSpan(span)) {
const dynamicSamplingContext = getDynamicSamplingContextFromSpan(span);
baggage = Object.entries(dynamicSamplingContext).reduce<Baggage>((b, [dscKey, dscValue]) => {
if (dscValue) {
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-node/src/utils/spanMap.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getRootSpan } from '@sentry/core';
import type { Span as SentrySpan } from '@sentry/types';

interface SpanMapEntry {
Expand Down Expand Up @@ -31,7 +32,7 @@ export function getSentrySpan(spanId: string): SentrySpan | undefined {
export function setSentrySpan(spanId: string, sentrySpan: SentrySpan): void {
let ref: SpanRefType = SPAN_REF_ROOT;

const rootSpanId = sentrySpan.transaction?.spanContext().spanId;
const rootSpanId = getRootSpan(sentrySpan)?.spanContext().spanId;

if (rootSpanId && rootSpanId !== spanId) {
const root = SPAN_MAP.get(rootSpanId);
Expand Down
1 change: 1 addition & 0 deletions packages/svelte/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
},
"dependencies": {
"@sentry/browser": "7.93.0",
"@sentry/core": "7.93.0",
"@sentry/types": "7.93.0",
"@sentry/utils": "7.93.0",
"magic-string": "^0.30.0"
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/src/performance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { Span, Transaction } from '@sentry/types';
import { afterUpdate, beforeUpdate, onMount } from 'svelte';
import { current_component } from 'svelte/internal';

import { getRootSpan } from '@sentry/core';
import { DEFAULT_COMPONENT_NAME, UI_SVELTE_INIT, UI_SVELTE_UPDATE } from './constants';
import type { TrackComponentOptions } from './types';

Expand Down Expand Up @@ -74,7 +75,7 @@ function recordUpdateSpans(componentName: string, initSpan?: Span): void {
// If we are initializing the component when the update span is started, we start it as child
// of the init span. Else, we start it as a child of the transaction.
const parentSpan =
initSpan && !initSpan.endTimestamp && initSpan.transaction === transaction ? initSpan : transaction;
initSpan && !initSpan.endTimestamp && getRootSpan(initSpan) === transaction ? initSpan : transaction;

// eslint-disable-next-line deprecation/deprecation
updateSpan = parentSpan.startChild({
Expand Down
3 changes: 2 additions & 1 deletion packages/tracing-internal/src/browser/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
getCurrentScope,
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
hasTracingEnabled,
spanToJSON,
spanToTraceHeader,
Expand Down Expand Up @@ -298,7 +299,7 @@ export function xhrCallback(

if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) {
if (span) {
const transaction = span && span.transaction;
const transaction = span && getRootSpan(span);
const dynamicSamplingContext = transaction && getDynamicSamplingContextFromSpan(transaction);
const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
setHeaderOnXhr(xhr, spanToTraceHeader(span), sentryBaggageHeader);
Expand Down
3 changes: 2 additions & 1 deletion packages/tracing-internal/src/common/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getCurrentScope,
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
hasTracingEnabled,
spanToTraceHeader,
} from '@sentry/core';
Expand Down Expand Up @@ -134,7 +135,7 @@ export function addTracingHeadersToFetchRequest(
// eslint-disable-next-line deprecation/deprecation
const span = requestSpan || scope.getSpan();

const transaction = span && span.transaction;
const transaction = span && getRootSpan(span);

const { traceId, sampled, dsc } = scope.getPropagationContext();

Expand Down
1 change: 1 addition & 0 deletions packages/types/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ export interface Span extends SpanContext {

/**
* The transaction containing this span
* @deprecated Use top level `Sentry.getRootSpan()` instead
*/
transaction?: Transaction;

Expand Down