Skip to content

Commit

Permalink
feat(core): Add updateSpanName helper function (#14291)
Browse files Browse the repository at this point in the history
Add `Sentry.updateSpanName()` to reliably update span
names in OpenTelemetry environments; prevents overwrites from both
OpenTelemetry HTTP instrumentation and Sentry's span inference logic;
preserves custom names by marking them with 'custom' source and storing
in temp field which takes precedence over the actual name and gets
deleted at the end
  • Loading branch information
Lms24 committed Dec 16, 2024
1 parent c698ad9 commit 364ca57
Show file tree
Hide file tree
Showing 32 changed files with 830 additions and 40 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/core';
import { type Event, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core';

import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
Expand All @@ -10,27 +10,34 @@ import {
import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest('sets the source to custom when updating the transaction name', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}
sentryTest(
'sets the source to custom when updating the transaction name with `span.updateName`',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });
const url = await getLocalTestUrl({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

const traceContextData = eventData.contexts?.trace?.data;
const traceContextData = eventData.contexts?.trace?.data;

expect(traceContextData).toMatchObject({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
});
expect(traceContextData).toBeDefined();

expect(eventData.transaction).toBe('new name');
expect(eventData.transaction).toBe('new name');

expect(eventData.contexts?.trace?.op).toBe('pageload');
expect(eventData.spans?.length).toBeGreaterThan(0);
expect(eventData.transaction_info?.source).toEqual('custom');
});
expect(traceContextData).toMatchObject({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
});

expect(traceContextData![SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined();

expect(eventData.contexts?.trace?.op).toBe('pageload');
expect(eventData.spans?.length).toBeGreaterThan(0);
expect(eventData.transaction_info?.source).toEqual('custom');
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;
window._testBaseTimestamp = performance.timeOrigin / 1000;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [Sentry.browserTracingIntegration()],
tracesSampleRate: 1,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const activeSpan = Sentry.getActiveSpan();
const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan);

Sentry.updateSpanName(rootSpan, 'new name');
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { expect } from '@playwright/test';
import { type Event, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core';

import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/browser';
import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest(
'sets the source to custom when updating the transaction name with Sentry.updateSpanName',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

const traceContextData = eventData.contexts?.trace?.data;

expect(traceContextData).toBeDefined();

expect(traceContextData).toMatchObject({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
});

expect(traceContextData![SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined();

expect(eventData.transaction).toBe('new name');

expect(eventData.contexts?.trace?.op).toBe('pageload');
expect(eventData.spans?.length).toBeGreaterThan(0);
expect(eventData.transaction_info?.source).toEqual('custom');
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -181,5 +181,9 @@ async function captureErrorAndGetEnvelopeTraceHeader(page: Page): Promise<Partia
await page.locator('#btnCaptureError').click();

const [, errorEnvelopeTraceHeader] = (await errorEventPromise)[0];

// @ts-expect-error - EventEnvelopeHeaders type in (types/envelope.ts) suggests that trace_id is optional,
// which the DynamicSamplingContext type does not permit.
// TODO(v9): We should adjust the EventEnvelopeHeaders type because the trace header always needs to have a trace_id
return errorEnvelopeTraceHeader;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
const Sentry = require('@sentry/node');

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
// disable attaching headers to /test/* endpoints
tracePropagationTargets: [/^(?!.*test).*$/],
tracesSampleRate: 1.0,
transport: loggingTransport,
});

// express must be required after Sentry is initialized
const express = require('express');
const cors = require('cors');
const bodyParser = require('body-parser');
const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');

const app = express();

app.use(cors());
app.use(bodyParser.json());
app.use(bodyParser.text());
app.use(bodyParser.raw());

app.get('/test/:id/span-updateName', (_req, res) => {
const span = Sentry.getActiveSpan();
const rootSpan = Sentry.getRootSpan(span);
rootSpan.updateName('new-name');
res.send({ response: 'response 1' });
});

app.get('/test/:id/span-updateName-source', (_req, res) => {
const span = Sentry.getActiveSpan();
const rootSpan = Sentry.getRootSpan(span);
rootSpan.updateName('new-name');
rootSpan.setAttribute(Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
res.send({ response: 'response 2' });
});

app.get('/test/:id/updateSpanName', (_req, res) => {
const span = Sentry.getActiveSpan();
const rootSpan = Sentry.getRootSpan(span);
Sentry.updateSpanName(rootSpan, 'new-name');
res.send({ response: 'response 3' });
});

app.get('/test/:id/updateSpanNameAndSource', (_req, res) => {
const span = Sentry.getActiveSpan();
const rootSpan = Sentry.getRootSpan(span);
Sentry.updateSpanName(rootSpan, 'new-name');
rootSpan.setAttribute(Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'component');
res.send({ response: 'response 4' });
});

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

describe('express tracing', () => {
afterAll(() => {
cleanupChildProcesses();
});

describe('CJS', () => {
// This test documents the unfortunate behaviour of using `span.updateName` on the server-side.
// For http.server root spans (which is the root span on the server 99% of the time), Otel's http instrumentation
// calls `span.updateName` and overwrites whatever the name was set to before (by us or by users).
test("calling just `span.updateName` doesn't update the final name in express (missing source)", done => {
createRunner(__dirname, 'server.js')
.expect({
transaction: {
transaction: 'GET /test/:id/span-updateName',
transaction_info: {
source: 'route',
},
},
})
.start(done)
.makeRequest('get', '/test/123/span-updateName');
});

// Also calling `updateName` AND setting a source doesn't change anything - Otel has no concept of source, this is sentry-internal.
// Therefore, only the source is updated but the name is still overwritten by Otel.
test("calling `span.updateName` and setting attribute source doesn't update the final name in express but it updates the source", done => {
createRunner(__dirname, 'server.js')
.expect({
transaction: {
transaction: 'GET /test/:id/span-updateName-source',
transaction_info: {
source: 'custom',
},
},
})
.start(done)
.makeRequest('get', '/test/123/span-updateName-source');
});

// This test documents the correct way to update the span name (and implicitly the source) in Node:
test('calling `Sentry.updateSpanName` updates the final name and source in express', done => {
createRunner(__dirname, 'server.js')
.expect({
transaction: txnEvent => {
expect(txnEvent).toMatchObject({
transaction: 'new-name',
transaction_info: {
source: 'custom',
},
contexts: {
trace: {
op: 'http.server',
data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
},
},
});
// ensure we delete the internal attribute once we're done with it
expect(txnEvent.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined();
},
})
.start(done)
.makeRequest('get', '/test/123/updateSpanName');
});
});

// This test documents the correct way to update the span name (and implicitly the source) in Node:
test('calling `Sentry.updateSpanName` and setting source subsequently updates the final name and sets correct source', done => {
createRunner(__dirname, 'server.js')
.expect({
transaction: txnEvent => {
expect(txnEvent).toMatchObject({
transaction: 'new-name',
transaction_info: {
source: 'component',
},
contexts: {
trace: {
op: 'http.server',
data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component' },
},
},
});
// ensure we delete the internal attribute once we're done with it
expect(txnEvent.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined();
},
})
.start(done)
.makeRequest('get', '/test/123/updateSpanNameAndSource');
});
});
Original file line number Diff line number Diff line change
@@ -1,11 +1,42 @@
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should send a manually started root span', done => {
test('sends a manually started root span with source custom', done => {
createRunner(__dirname, 'scenario.ts')
.expect({ transaction: { transaction: 'test_span' } })
.expect({
transaction: {
transaction: 'test_span',
transaction_info: { source: 'custom' },
contexts: {
trace: {
span_id: expect.any(String),
trace_id: expect.any(String),
data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
},
},
},
})
.start(done);
});

test("doesn't change the name for manually started spans even if attributes triggering inference are set", done => {
createRunner(__dirname, 'scenario.ts')
.expect({
transaction: {
transaction: 'test_span',
transaction_info: { source: 'custom' },
contexts: {
trace: {
span_id: expect.any(String),
trace_id: expect.any(String),
data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' },
},
},
},
})
.start(done);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});

Sentry.startSpan(
{ name: 'test_span', attributes: { [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' } },
(span: Sentry.Span) => {
span.updateName('new name');
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node';
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('updates the span name when calling `span.updateName`', done => {
createRunner(__dirname, 'scenario.ts')
.expect({
transaction: {
transaction: 'new name',
transaction_info: { source: 'url' },
contexts: {
trace: {
span_id: expect.any(String),
trace_id: expect.any(String),
data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' },
},
},
},
})
.start(done);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});

Sentry.startSpan(
{ name: 'test_span', attributes: { [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' } },
(span: Sentry.Span) => {
Sentry.updateSpanName(span, 'new name');
},
);
Loading

0 comments on commit 364ca57

Please sign in to comment.