Skip to content

feat(core): Deprecate span name and description #10056

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
Jan 8, 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
2 changes: 2 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
* `span.sampled`: Use `span.isRecording()` instead.
* `span.spanId`: Use `span.spanContext().spanId` instead.
* `span.traceId`: Use `span.spanContext().traceId` instead.
* `span.name`: Use `spanToJSON(span).description` instead.
* `span.description`: Use `spanToJSON(span).description` instead.

## Deprecate `pushScope` & `popScope` in favor of `withScope`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ sentryTest('should report finished spans as children of the root transaction', a
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);
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ sentryTest('should capture a FID vital.', async ({ browserName, getLocalTestPath
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ sentryTest('should capture FP vital.', async ({ browserName, getLocalTestPath, p
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();
Expand All @@ -34,6 +35,7 @@ sentryTest('should capture FCP vital.', async ({ getLocalTestPath, page }) => {
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();
Expand Down
15 changes: 6 additions & 9 deletions packages/browser/src/profiling/hubextensions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* eslint-disable complexity */
import { spanToJSON } from '@sentry/core';
import type { Transaction } from '@sentry/types';
import { logger, timestampInSeconds, uuid4 } from '@sentry/utils';

Expand Down Expand Up @@ -56,7 +57,7 @@ export function startProfileForTransaction(transaction: Transaction): Transactio
}

if (DEBUG_BUILD) {
logger.log(`[Profiling] started profiling transaction: ${transaction.name || transaction.description}`);
logger.log(`[Profiling] started profiling transaction: ${spanToJSON(transaction).description}`);
}

// We create "unique" transaction names to avoid concurrent transactions with same names
Expand Down Expand Up @@ -87,11 +88,7 @@ export function startProfileForTransaction(transaction: Transaction): Transactio
}
if (processedProfile) {
if (DEBUG_BUILD) {
logger.log(
'[Profiling] profile for:',
transaction.name || transaction.description,
'already exists, returning early',
);
logger.log('[Profiling] profile for:', spanToJSON(transaction).description, 'already exists, returning early');
}
return null;
}
Expand All @@ -105,14 +102,14 @@ export function startProfileForTransaction(transaction: Transaction): Transactio
}

if (DEBUG_BUILD) {
logger.log(`[Profiling] stopped profiling of transaction: ${transaction.name || transaction.description}`);
logger.log(`[Profiling] stopped profiling of transaction: ${spanToJSON(transaction).description}`);
}

// In case of an overlapping transaction, stopProfiling may return null and silently ignore the overlapping profile.
if (!profile) {
if (DEBUG_BUILD) {
logger.log(
`[Profiling] profiler returned null profile for: ${transaction.name || transaction.description}`,
`[Profiling] profiler returned null profile for: ${spanToJSON(transaction).description}`,
'this may indicate an overlapping transaction or a call to stopProfiling with a profile title that was never started',
);
}
Expand All @@ -135,7 +132,7 @@ export function startProfileForTransaction(transaction: Transaction): Transactio
if (DEBUG_BUILD) {
logger.log(
'[Profiling] max profile duration elapsed, stopping profiling for:',
transaction.name || transaction.description,
spanToJSON(transaction).description,
);
}
// If the timeout exceeds, we want to stop profiling, but not finish the transaction
Expand Down
6 changes: 3 additions & 3 deletions packages/bun/test/integrations/bunserver.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { beforeAll, beforeEach, describe, expect, test } from 'bun:test';
import { Hub, makeMain } from '@sentry/core';
import { Hub, makeMain, spanToJSON } from '@sentry/core';

import { BunClient } from '../../src/client';
import { instrumentBunServe } from '../../src/integrations/bunserver';
Expand Down Expand Up @@ -30,7 +30,7 @@ describe('Bun Serve Integration', () => {
'http.status_code': '200',
});
expect(transaction.op).toEqual('http.server');
expect(transaction.name).toEqual('GET /');
expect(spanToJSON(transaction).description).toEqual('GET /');
});

const server = Bun.serve({
Expand All @@ -52,7 +52,7 @@ describe('Bun Serve Integration', () => {
'http.status_code': '200',
});
expect(transaction.op).toEqual('http.server');
expect(transaction.name).toEqual('POST /');
expect(spanToJSON(transaction).description).toEqual('POST /');
});

const server = Bun.serve({
Expand Down
7 changes: 5 additions & 2 deletions packages/core/src/integrations/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Client, IntegrationFn, Transaction } from '@sentry/types';
import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils';
import { addRequestDataToEvent, extractPathForTransaction } from '@sentry/utils';
import { convertIntegrationFnToClass } from '../integration';
import { spanToJSON } from '../utils/spanUtils';

export type RequestDataIntegrationOptions = {
/**
Expand Down Expand Up @@ -105,18 +106,20 @@ const requestDataIntegration: IntegrationFn = (options: RequestDataIntegrationOp
const reqWithTransaction = req as { _sentryTransaction?: Transaction };
const transaction = reqWithTransaction._sentryTransaction;
if (transaction) {
const name = spanToJSON(transaction).description || '';

// TODO (v8): Remove the nextjs check and just base it on `transactionNamingScheme` for all SDKs. (We have to
// keep it the way it is for the moment, because changing the names of transactions in Sentry has the potential
// to break things like alert rules.)
const shouldIncludeMethodInTransactionName =
getSDKName(client) === 'sentry.javascript.nextjs'
? transaction.name.startsWith('/api')
? name.startsWith('/api')
: transactionNamingScheme !== 'path';

const [transactionValue] = extractPathForTransaction(req, {
path: true,
method: shouldIncludeMethodInTransactionName,
customRoute: transaction.name,
customRoute: name,
});

processedEvent.transaction = transactionValue;
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/metrics/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { logger } from '@sentry/utils';
import type { BaseClient } from '../baseclient';
import { DEBUG_BUILD } from '../debug-build';
import { getClient, getCurrentScope } from '../exports';
import { spanToJSON } from '../utils/spanUtils';
import { COUNTER_METRIC_TYPE, DISTRIBUTION_METRIC_TYPE, GAUGE_METRIC_TYPE, SET_METRIC_TYPE } from './constants';
import { MetricsAggregator } from './integration';
import type { MetricType } from './types';
Expand Down Expand Up @@ -38,7 +39,7 @@ function addToMetricsAggregator(
metricTags.environment = environment;
}
if (transaction) {
metricTags.transaction = transaction.name;
metricTags.transaction = spanToJSON(transaction).description || '';
}

DEBUG_BUILD && logger.log(`Adding value of ${value} to ${metricType} metric ${name}`);
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/tracing/sampling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { isNaN, logger } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
import { spanToJSON } from '../utils/spanUtils';
import type { Transaction } from './transaction';

/**
Expand Down Expand Up @@ -100,7 +101,8 @@ export function sampleTransaction<T extends Transaction>(
return transaction;
}

DEBUG_BUILD && logger.log(`[Tracing] starting ${transaction.op} transaction - ${transaction.name}`);
DEBUG_BUILD &&
logger.log(`[Tracing] starting ${transaction.op} transaction - ${spanToJSON(transaction).description}`);
return transaction;
}

Expand Down
52 changes: 33 additions & 19 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
TRACE_FLAG_NONE,
TRACE_FLAG_SAMPLED,
spanTimeInputToSeconds,
spanToJSON,
spanToTraceContext,
spanToTraceHeader,
} from '../utils/spanUtils';
Expand Down Expand Up @@ -84,11 +85,6 @@ export class Span implements SpanInterface {
*/
public op?: string;

/**
* @inheritDoc
*/
public description?: string;

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -128,6 +124,7 @@ export class Span implements SpanInterface {
protected _traceId: string;
protected _spanId: string;
protected _sampled: boolean | undefined;
protected _name?: string;

/**
* You should never call the constructor manually, always use `Sentry.startTransaction()`
Expand All @@ -145,6 +142,8 @@ export class Span implements SpanInterface {
this.attributes = spanContext.attributes || {};
this.instrumenter = spanContext.instrumenter || 'sentry';
this.origin = spanContext.origin || 'manual';
// eslint-disable-next-line deprecation/deprecation
this._name = spanContext.name || spanContext.description;

if (spanContext.parentSpanId) {
this.parentSpanId = spanContext.parentSpanId;
Expand All @@ -156,12 +155,6 @@ export class Span implements SpanInterface {
if (spanContext.op) {
this.op = spanContext.op;
}
if (spanContext.description) {
this.description = spanContext.description;
}
if (spanContext.name) {
this.description = spanContext.name;
}
if (spanContext.status) {
this.status = spanContext.status;
}
Expand All @@ -170,20 +163,40 @@ export class Span implements SpanInterface {
}
}

// This conflicts with another eslint rule :(
// This rule conflicts with another rule :(
/* eslint-disable @typescript-eslint/member-ordering */

/** An alias for `description` of the Span. */
/**
* An alias for `description` of the Span.
* @deprecated Use `spanToJSON(span).description` instead.
*/
public get name(): string {
return this.description || '';
return this._name || '';
}
/**
* Update the name of the span.
* @deprecated Use `spanToJSON(span).description` instead.
*/
public set name(name: string) {
this.updateName(name);
}

/**
* Get the description of the Span.
* @deprecated Use `spanToJSON(span).description` instead.
*/
public get description(): string | undefined {
return this._name;
}

/**
* Get the description of the Span.
* @deprecated Use `spanToJSON(span).description` instead.
*/
public set description(description: string | undefined) {
this._name = description;
}

/**
* The ID of the trace.
* @deprecated Use `spanContext().traceId` instead.
Expand Down Expand Up @@ -269,7 +282,7 @@ export class Span implements SpanInterface {

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

const logMessage = `[Tracing] Starting '${opStr}' span on transaction '${nameStr}' (${idStr}).`;
Expand Down Expand Up @@ -342,7 +355,7 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public updateName(name: string): this {
this.description = name;
this._name = name;
return this;
}

Expand Down Expand Up @@ -392,7 +405,7 @@ export class Span implements SpanInterface {
public toContext(): SpanContext {
return dropUndefinedKeys({
data: this._getData(),
description: this.description,
description: this._name,
endTimestamp: this.endTimestamp,
op: this.op,
parentSpanId: this.parentSpanId,
Expand All @@ -410,7 +423,8 @@ export class Span implements SpanInterface {
*/
public updateWithContext(spanContext: SpanContext): this {
this.data = spanContext.data || {};
this.description = spanContext.description;
// eslint-disable-next-line deprecation/deprecation
this._name = spanContext.name || spanContext.description;
this.endTimestamp = spanContext.endTimestamp;
this.op = spanContext.op;
this.parentSpanId = spanContext.parentSpanId;
Expand All @@ -437,7 +451,7 @@ export class Span implements SpanInterface {
public getSpanJSON(): SpanJSON {
return dropUndefinedKeys({
data: this._getData(),
description: this.description,
description: this._name,
op: this.op,
parent_span_id: this.parentSpanId,
span_id: this._spanId,
Expand Down
13 changes: 4 additions & 9 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,11 @@ export function trace<T>(
// eslint-disable-next-line @typescript-eslint/no-empty-function
afterFinish: () => void = () => {},
): T {
const ctx = normalizeContext(context);

const hub = getCurrentHub();
const scope = getCurrentScope();
const parentSpan = scope.getSpan();

const ctx = normalizeContext(context);
const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx);

scope.setSpan(activeSpan);
Expand Down Expand Up @@ -259,16 +258,12 @@ function createChildSpanOrTransaction(
* Eventually the StartSpanOptions will be more aligned with OpenTelemetry.
*/
function normalizeContext(context: StartSpanOptions): TransactionContext {
const ctx = { ...context };
// If a name is set and a description is not, set the description to the name.
if (ctx.name !== undefined && ctx.description === undefined) {
ctx.description = ctx.name;
}

if (context.startTime) {
const ctx = { ...context };
ctx.startTimestamp = spanTimeInputToSeconds(context.startTime);
delete ctx.startTime;
return ctx;
}

return ctx;
return context;
}
Loading