diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js index cb5bc5e3336b..6e93018cc063 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js @@ -1,3 +1,3 @@ const activeSpan = Sentry.getActiveSpan(); const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan); -rootSpan.updateName('new name'); +rootSpan?.updateName('new name'); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js b/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js index cff6a955eea2..c98e17276d92 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js +++ b/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js @@ -45,6 +45,14 @@ app.get('/test/:id/updateSpanName', (_req, res) => { 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); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts index ec3346dad096..a9ef1fa2a1d0 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts @@ -52,6 +52,7 @@ describe('express tracing', () => { }, contexts: { trace: { + op: 'http.server', data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' }, }, }, @@ -64,4 +65,29 @@ describe('express tracing', () => { .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?.['_sentry_span_name_set_by_user']).toBeUndefined(); + }, + }) + .start(done) + .makeRequest('get', '/test/123/updateSpanNameAndSource'); + }); }); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts index cb01ee8b4098..0370b123cab2 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/basic-usage/test.ts @@ -22,3 +22,21 @@ test('sends a manually started root span with source custom', done => { }) .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); +}); diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 4655cea14b9c..b69f7597796c 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -333,6 +333,8 @@ export function showSpanDropWarning(): void { */ export function updateSpanName(span: Span, name: string): void { span.updateName(name); - span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom'); - span.setAttribute('_sentry_span_name_set_by_user', name); + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + ['_sentry_span_name_set_by_user']: name, + }); } diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 993fc2fced8e..e800d6a48109 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -42,7 +42,7 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind // eslint-disable-next-line deprecation/deprecation const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD]; if (httpMethod) { - return descriptionForHttpMethod({ attributes, name: getOriginalName(spanName, attributes), kind }, httpMethod); + return descriptionForHttpMethod({ attributes, name: spanName, kind }, httpMethod); } // eslint-disable-next-line deprecation/deprecation @@ -64,9 +64,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind const rpcService = attributes[SEMATTRS_RPC_SERVICE]; if (rpcService) { return { + ...getUserUpdatedNameAndSource(spanName, attributes, 'route'), op: 'rpc', - description: getOriginalName(spanName, attributes), - source: customSourceOrRoute, }; } @@ -75,9 +74,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind const messagingSystem = attributes[SEMATTRS_MESSAGING_SYSTEM]; if (messagingSystem) { return { + ...getUserUpdatedNameAndSource(spanName, attributes, customSourceOrRoute), op: 'message', - description: getOriginalName(spanName, attributes), - source: customSourceOrRoute, }; } @@ -86,9 +84,8 @@ export function inferSpanData(spanName: string, attributes: SpanAttributes, kind const faasTrigger = attributes[SEMATTRS_FAAS_TRIGGER]; if (faasTrigger) { return { + ...getUserUpdatedNameAndSource(spanName, attributes, customSourceOrRoute), op: faasTrigger.toString(), - description: getOriginalName(spanName, attributes), - source: customSourceOrRoute, }; } @@ -108,14 +105,27 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { const attributes = spanHasAttributes(span) ? span.attributes : {}; const name = spanHasName(span) ? span.name : ''; const kind = getSpanKind(span); + // console.log('x parseSpanDesc', { attributes, name, kind }); - return inferSpanData(name, attributes, kind); + const res = inferSpanData(name, attributes, kind); + + // console.log('x parseSpanDesc res', res); + return res; } function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription { - // if we already set the source to custom, we don't overwrite the span description but just adjust the op + // if we already have a custom name, we don't overwrite it but only set the op + if (typeof attributes['_sentry_span_name_set_by_user'] === 'string') { + return { + op: 'db', + description: attributes['_sentry_span_name_set_by_user'], + source: (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || 'custom', + }; + } + + // if we already have the source set to custom, we don't overwrite the span description but only set the op if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom') { - return { op: 'db', description: getOriginalName(name, attributes), source: 'custom' }; + return { op: 'db', description: name, source: 'custom' }; } // Use DB statement (Ex "SELECT * FROM table") if possible as description. @@ -151,7 +161,7 @@ export function descriptionForHttpMethod( const { urlPath, url, query, fragment, hasRoute } = getSanitizedUrl(attributes, kind); if (!urlPath) { - return { op: opParts.join('.'), description: getOriginalName(name, attributes), source: 'custom' }; + return { ...getUserUpdatedNameAndSource(name, attributes), op: opParts.join('.') }; } const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION]; @@ -161,12 +171,12 @@ export function descriptionForHttpMethod( // When the http span has a graphql operation, append it to the description // We add these in the graphqlIntegration - const description = graphqlOperationsAttribute + const inferredDescription = graphqlOperationsAttribute ? `${baseDescription} (${getGraphqlOperationNamesFromAttribute(graphqlOperationsAttribute)})` : baseDescription; // If `httpPath` is a root path, then we can categorize the transaction source as route. - const source: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url'; + const inferredSource: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url'; const data: Record = {}; @@ -190,15 +200,22 @@ export function descriptionForHttpMethod( const origin = attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] || 'manual'; const isManualSpan = !`${origin}`.startsWith('auto'); - // If users (or in very rare occasions we) set the source to custom, we don't overwrite it + // If users (or in very rare occasions we) set the source to custom, we don't overwrite the name const alreadyHasCustomSource = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom'; - const useInferredDescription = !alreadyHasCustomSource && (isClientOrServerKind || !isManualSpan); + const useInferredDescription = + !alreadyHasCustomSource && + attributes['_sentry_span_name_set_by_user'] == null && + (isClientOrServerKind || !isManualSpan); + + const { description, source } = useInferredDescription + ? { description: inferredDescription, source: inferredSource } + : getUserUpdatedNameAndSource(name, attributes); return { op: opParts.join('.'), - description: useInferredDescription ? description : getOriginalName(name, attributes), - source: useInferredDescription ? source : 'custom', + description, + source, data, }; } @@ -265,15 +282,32 @@ export function getSanitizedUrl( } /** - * Because Otel decided to mutate span names via `span.updateName`, the only way to ensure - * that a user-set span name is preserved is to store it as a tmp attribute on the span. + * Because Otel instrumentation sometimes mutates span names via `span.updateName`, the only way + * to ensure that a user-set span name is preserved is to store it as a tmp attribute on the span. * We delete this attribute once we're done with it when preparing the event envelope. + * + * This temp attribute always takes precedence over the original name. + * + * We also need to take care of setting the correct source. Users can always update the source + * after updating the name, so we need to respect that. + * * @internal exported only for testing */ -export function getOriginalName(name: string, attributes: Attributes): string { - return attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' && - attributes['_sentry_span_name_set_by_user'] && - typeof attributes['_sentry_span_name_set_by_user'] === 'string' - ? attributes['_sentry_span_name_set_by_user'] - : name; +export function getUserUpdatedNameAndSource( + originalName: string, + attributes: Attributes, + fallbackSource: TransactionSource = 'custom', +): { + description: string; + source: TransactionSource; +} { + const source = (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || fallbackSource; + + if (attributes['_sentry_span_name_set_by_user'] && typeof attributes['_sentry_span_name_set_by_user'] === 'string') + return { + description: attributes['_sentry_span_name_set_by_user'], + source, + }; + + return { description: originalName, source }; } diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index a9107109444e..6656ccac1c4e 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -18,7 +18,7 @@ import { import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import { descriptionForHttpMethod, - getOriginalName, + getUserUpdatedNameAndSource, getSanitizedUrl, parseSpanDescription, } from '../../src/utils/parseSpanDescription'; @@ -118,6 +118,22 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with db system and component source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMATTRS_DB_SYSTEM]: 'mysql', + [SEMATTRS_DB_STATEMENT]: 'SELECT * from users', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + SpanKind.CLIENT, + { + description: 'custom name', + op: 'db', + source: 'component', + }, + ], [ 'works with db system without statement', { @@ -173,6 +189,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with rpc service and component source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMATTRS_RPC_SERVICE]: 'rpc-test-service', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + undefined, + { + description: 'custom name', + op: 'rpc', + source: 'component', + }, + ], [ 'works with messaging system', { @@ -215,6 +246,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with messaging system and component source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMATTRS_MESSAGING_SYSTEM]: 'test-messaging-system', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + undefined, + { + description: 'custom name', + op: 'message', + source: 'component', + }, + ], [ 'works with faas trigger', { @@ -257,6 +303,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with faas trigger and component source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMATTRS_FAAS_TRIGGER]: 'test-faas-trigger', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + undefined, + { + description: 'custom name', + op: 'test-faas-trigger', + source: 'component', + }, + ], ])('%s', (_, attributes, name, kind, expected) => { const actual = parseSpanDescription({ attributes, kind, name } as unknown as Span); expect(actual).toEqual(expected); @@ -417,6 +478,28 @@ describe('descriptionForHttpMethod', () => { source: 'custom', }, ], + [ + 'takes user-passed span name (with source component)', + 'GET', + { + [SEMATTRS_HTTP_METHOD]: 'GET', + [SEMATTRS_HTTP_URL]: 'https://www.example.com/my-path/123', + [SEMATTRS_HTTP_TARGET]: '/my-path/123', + [ATTR_HTTP_ROUTE]: '/my-path/:id', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + SpanKind.CLIENT, + { + op: 'http.client', + description: 'custom name', + data: { + url: 'https://www.example.com/my-path/123', + }, + source: 'component', + }, + ], ])('%s', (_, httpMethod, attributes, name, kind, expected) => { const actual = descriptionForHttpMethod({ attributes, kind, name }, httpMethod); expect(actual).toEqual(expected); @@ -571,25 +654,37 @@ describe('getSanitizedUrl', () => { }); }); -describe('getOriginalName', () => { - it('returns param name if source is not custom', () => { - expect(getOriginalName('base name', {})).toBe('base name'); +describe('getUserUpdatedNameAndSource', () => { + it('returns param name if `_sentry_span_name_set_by_user` attribute is not set', () => { + expect(getUserUpdatedNameAndSource('base name', {})).toEqual({ description: 'base name', source: 'custom' }); }); - it('returns param name if `_sentry_span_name_set_by_user` attribute is not set', () => { - expect(getOriginalName('base name', { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' })).toBe('base name'); + it('returns param name with custom fallback source if `_sentry_span_name_set_by_user` attribute is not set', () => { + expect(getUserUpdatedNameAndSource('base name', {}, 'route')).toEqual({ + description: 'base name', + source: 'route', + }); }); it('returns param name if `_sentry_span_name_set_by_user` attribute is not a string', () => { - expect(getOriginalName('base name', { ['_sentry_span_name_set_by_user']: 123 })).toBe('base name'); + expect(getUserUpdatedNameAndSource('base name', { ['_sentry_span_name_set_by_user']: 123 })).toEqual({ + description: 'base name', + source: 'custom', + }); }); - it('returns `_sentry_span_name_set_by_user` attribute if is a string and source is custom', () => { - expect( - getOriginalName('base name', { - ['_sentry_span_name_set_by_user']: 'custom name', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - }), - ).toBe('custom name'); - }); + it.each(['custom', 'task', 'url', 'route'])( + 'returns `_sentry_span_name_set_by_user` attribute if is a string and source is %s', + source => { + expect( + getUserUpdatedNameAndSource('base name', { + ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, + }), + ).toEqual({ + description: 'custom name', + source, + }); + }, + ); });