Skip to content

ref: Migrate transaction source from metadata to attributes #10674

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 22 commits into from
Feb 19, 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
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ sentryTest('should capture feedback (@sentry-internal/feedback import)', async (
const feedbackEvent = envelopeRequestParser((await feedbackRequestPromise).request());
expect(feedbackEvent).toEqual({
type: 'feedback',
breadcrumbs: expect.any(Array),
contexts: {
feedback: {
contact_email: '[email protected]',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,16 @@ sentryTest('should capture feedback (@sentry-internal/feedback import)', async (

expect(breadcrumbs).toEqual(
expect.arrayContaining([
{
expect.objectContaining({
category: 'sentry.feedback',
data: { feedbackId: expect.any(String) },
},
}),
]),
);

expect(feedbackEvent).toEqual({
type: 'feedback',
breadcrumbs: expect.any(Array),
contexts: {
feedback: {
contact_email: '[email protected]',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as Sentry from '@sentry/browser';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/browser';
import { Integrations } from '@sentry/tracing';

window.Sentry = Sentry;
Expand All @@ -17,4 +18,4 @@ scope.addEventProcessor(event => {
event.transaction = 'testTransactionDSC';
return event;
});
scope.getTransaction().setMetadata({ source: 'custom' });
scope.getTransaction().setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
'sentry.source': 'route',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -89,6 +90,7 @@ test('Propagates trace for outgoing http requests', async ({ baseURL }) => {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
'sentry.source': 'route',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down Expand Up @@ -162,6 +164,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
'sentry.source': 'route',
},
op: 'http.server',
span_id: expect.any(String),
Expand All @@ -186,6 +189,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
'sentry.source': 'route',
},
op: 'http.server',
parent_span_id: outgoingHttpSpanId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.otel.http',
'sentry.source': 'route',
},
op: 'http.server',
span_id: expect.any(String),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';
import * as Tracing from '@sentry/tracing';
import express from 'express';

const app = express();
Expand All @@ -9,7 +8,7 @@ Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
// eslint-disable-next-line deprecation/deprecation
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Sentry.Integrations.Express({ app })],
tracesSampleRate: 1.0,
transport: loggingTransport,
});
Expand Down
5 changes: 4 additions & 1 deletion 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 } from '../currentScopes';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
import { getRootSpan } from '../utils/getRootSpan';
import { spanIsSampled, spanToJSON } from '../utils/spanUtils';

Expand Down Expand Up @@ -66,14 +67,16 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
// TODO (v8): Replace txn.metadata with txn.attributes[]
// We can't do this yet because attributes aren't always set yet.
// eslint-disable-next-line deprecation/deprecation
const { sampleRate: maybeSampleRate, source } = txn.metadata;
const { sampleRate: maybeSampleRate } = txn.metadata;
if (maybeSampleRate != null) {
dsc.sample_rate = `${maybeSampleRate}`;
}

// We don't want to have a transaction name in the DSC if the source is "url" because URLs might contain PII
const jsonSpan = spanToJSON(txn);

const source = (jsonSpan.data || {})[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];

// after JSON conversion, txn.name becomes jsonSpan.description
if (source && source !== 'url') {
dsc.transaction = jsonSpan.description;
Expand Down
17 changes: 9 additions & 8 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type {
TransactionContext,
TransactionEvent,
TransactionMetadata,
TransactionSource,
} from '@sentry/types';
import { dropUndefinedKeys, logger } from '@sentry/utils';

Expand Down Expand Up @@ -68,6 +69,11 @@ export class Transaction extends SentrySpan implements TransactionInterface {

this._trimEnd = transactionContext.trimEnd;

this._attributes = {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
...this._attributes,
};

// 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
// eslint-disable-next-line deprecation/deprecation
Expand Down Expand Up @@ -110,17 +116,12 @@ export class Transaction extends SentrySpan implements TransactionInterface {
// We merge attributes in for backwards compatibility
return {
// Defaults
// eslint-disable-next-line deprecation/deprecation
source: 'custom',
spanMetadata: {},

// Legacy metadata
...this._metadata,

// From attributes
...(this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] && {
source: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionMetadata['source'],
}),
...(this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] && {
sampleRate: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as TransactionMetadata['sampleRate'],
}),
Expand All @@ -142,7 +143,7 @@ export class Transaction extends SentrySpan implements TransactionInterface {
*
* @deprecated Use `.updateName()` and `.setAttribute()` instead.
*/
public setName(name: string, source: TransactionMetadata['source'] = 'custom'): void {
public setName(name: string, source: TransactionSource = 'custom'): void {
this._name = name;
this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
}
Expand Down Expand Up @@ -309,8 +310,8 @@ export class Transaction extends SentrySpan implements TransactionInterface {

// eslint-disable-next-line deprecation/deprecation
const { metadata } = this;
// eslint-disable-next-line deprecation/deprecation
const { source } = metadata;

const source = this._attributes['sentry.source'] as TransactionSource | undefined;

const transaction: TransactionEvent = {
contexts: {
Expand Down
14 changes: 9 additions & 5 deletions packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ describe('getDynamicSamplingContextFromSpan', () => {
name: 'tx',
metadata: {
sampleRate: 0.56,
source: 'route',
},
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
},
sampled: true,
});
Expand All @@ -98,9 +100,11 @@ describe('getDynamicSamplingContextFromSpan', () => {
const transaction = new Transaction({
name: 'tx',
metadata: {
source: 'url',
sampleRate: 0.56,
},
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
},
});

const dsc = getDynamicSamplingContextFromSpan(transaction);
Expand All @@ -110,11 +114,11 @@ describe('getDynamicSamplingContextFromSpan', () => {
test.each([
['is included if transaction source is parameterized route/url', 'route'],
['is included if transaction source is a custom name', 'custom'],
])('%s', (_: string, source) => {
] as const)('%s', (_: string, source: TransactionSource) => {
const transaction = startInactiveSpan({
name: 'tx',
metadata: {
...(source && { source: source as TransactionSource }),
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
},
});

Expand Down
1 change: 1 addition & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ describe('startSpan', () => {
data: {
'sentry.origin': 'auto.http.browser',
'sentry.sample_rate': 0,
'sentry.source': 'custom',
},
origin: 'auto.http.browser',
description: 'GET users/[id]',
Expand Down
41 changes: 19 additions & 22 deletions packages/core/test/lib/tracing/transaction.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, Transaction } from '../../../src';
import {
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
Transaction,
} from '../../../src';

describe('transaction', () => {
describe('name', () => {
Expand All @@ -16,7 +21,7 @@ describe('transaction', () => {
transaction.name = 'new name';

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('custom');
expect(transaction.attributes['sentry.source']).toEqual('custom');
});

it('allows to update the name via setName', () => {
Expand All @@ -27,7 +32,7 @@ describe('transaction', () => {
transaction.setName('new name');

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('custom');
expect(transaction.attributes['sentry.source']).toEqual('custom');
});

it('allows to update the name via updateName', () => {
Expand All @@ -38,7 +43,7 @@ describe('transaction', () => {
transaction.updateName('new name');

expect(transaction.name).toEqual('new name');
expect(transaction.metadata.source).toEqual('route');
expect(transaction.attributes['sentry.source']).toEqual('route');
});
/* eslint-enable deprecation/deprecation */
});
Expand All @@ -48,15 +53,13 @@ describe('transaction', () => {
it('works with defaults', () => {
const transaction = new Transaction({ name: 'span name' });
expect(transaction.metadata).toEqual({
source: 'custom',
spanMetadata: {},
});
});

it('allows to set metadata in constructor', () => {
const transaction = new Transaction({ name: 'span name', metadata: { source: 'url', request: {} } });
const transaction = new Transaction({ name: 'span name', metadata: { request: {} } });
expect(transaction.metadata).toEqual({
source: 'url',
spanMetadata: {},
request: {},
});
Expand All @@ -71,37 +74,31 @@ describe('transaction', () => {
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.5,
},
});

expect(transaction.metadata).toEqual({
source: 'url',
sampleRate: 0.5,
spanMetadata: {},
request: {},
});
});

it('allows to update metadata via setMetadata', () => {
const transaction = new Transaction({ name: 'span name', metadata: { source: 'url', request: {} } });

transaction.setMetadata({ source: 'route' });

expect(transaction.metadata).toEqual({
source: 'route',
spanMetadata: {},
request: {},
expect(transaction.attributes).toEqual({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.5,
});
});

it('allows to update metadata via setAttribute', () => {
const transaction = new Transaction({ name: 'span name', metadata: { source: 'url', request: {} } });
it('allows to update metadata via setMetadata', () => {
const transaction = new Transaction({ name: 'span name', metadata: {} });

transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
transaction.setMetadata({ request: {} });

expect(transaction.metadata).toEqual({
source: 'route',
spanMetadata: {},
request: {},
});
});

/* eslint-enable deprecation/deprecation */
});
});
35 changes: 0 additions & 35 deletions packages/deno/test/__snapshots__/mod.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -41,41 +41,6 @@ snapshot[`captureException 1`] = `
},
stacktrace: {
frames: [
{
colno: 20,
filename: "ext:cli/40_testing.js",
function: "outerWrapped",
in_app: false,
lineno: 472,
},
{
colno: 33,
filename: "ext:cli/40_testing.js",
function: "exitSanitizer",
in_app: false,
lineno: 458,
},
{
colno: 31,
filename: "ext:cli/40_testing.js",
function: "resourceSanitizer",
in_app: false,
lineno: 410,
},
{
colno: 33,
filename: "ext:cli/40_testing.js",
function: "asyncOpSanitizer",
in_app: false,
lineno: 177,
},
{
colno: 11,
filename: "ext:cli/40_testing.js",
function: "innerWrapped",
in_app: false,
lineno: 526,
},
{
colno: 27,
context_line: " client.captureException(something());",
Expand Down
4 changes: 2 additions & 2 deletions packages/deno/test/normalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ function normalizeEvent(event: sentryTypes.Event): sentryTypes.Event {
}

if (event.exception?.values?.[0].stacktrace?.frames) {
// Exlcude Deno frames since these may change between versions
// Exclude Deno frames since these may change between versions
event.exception.values[0].stacktrace.frames = event.exception.values[0].stacktrace.frames.filter(
frame => !frame.filename?.includes('deno:'),
frame => !frame.filename?.includes('deno:') && !frame.filename?.startsWith('ext:'),
);
}

Expand Down
Loading