From 1be63e28f8f46aed32f981ce02be774ea1834722 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 12:14:20 +0100 Subject: [PATCH 01/18] feat(core): Add `updateSpanName` helper function --- packages/core/src/utils/spanUtils.ts | 27 ++++- .../core/test/lib/utils/spanUtils.test.ts | 21 +++- .../src/utils/parseSpanDescription.ts | 35 +++++-- .../test/utils/parseSpanDescription.test.ts | 99 +++++++++++++++++++ 4 files changed, 169 insertions(+), 13 deletions(-) diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 594a297f9395..fa3592fe5ada 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -3,7 +3,11 @@ import { getMainCarrier } from '../carrier'; import { getCurrentScope } from '../currentScopes'; import { getMetricSummaryJsonForSpan, updateMetricSummaryOnSpan } from '../metrics/metric-summary'; import type { MetricType } from '../metrics/types'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../semanticAttributes'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, +} from '../semanticAttributes'; import type { SentrySpan } from '../tracing/sentrySpan'; import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus'; import type { @@ -310,3 +314,24 @@ export function showSpanDropWarning(): void { hasShownSpanDropWarning = true; } } + +/** + * Updates the name of the given span and ensures that the span name is not + * overwritten by the Sentry SDK. + * + * Use this function instead of `span.updateName()` if you want to make sure that + * your name is kept. For some spans, for example root `http.server` spans the + * Sentry SDK would otherwise overwrite the span name with a high-quality name + * it infers when the span ends. + * + * Use this function in server code or when your span is started on the server + * and on the client (browser). If you only update a span name on the client, + * you can also use `span.updateName()` the SDK does not overwrite the name. + * + * @param span - The span to update the name of. + * @param name - The name to set on the span. + */ +export function updateSpanName(span: Span, name: string): void { + span.updateName(name); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom'); +} diff --git a/packages/core/test/lib/utils/spanUtils.test.ts b/packages/core/test/lib/utils/spanUtils.test.ts index f7187695a025..aa6d4bf4cb2f 100644 --- a/packages/core/test/lib/utils/spanUtils.test.ts +++ b/packages/core/test/lib/utils/spanUtils.test.ts @@ -1,6 +1,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SPAN_STATUS_ERROR, SPAN_STATUS_OK, SPAN_STATUS_UNSET, @@ -14,8 +15,14 @@ import { } from '../../../src'; import type { Span, SpanAttributes, SpanStatus, SpanTimeInput } from '../../../src/types-hoist'; import type { OpenTelemetrySdkTraceBaseSpan } from '../../../src/utils/spanUtils'; -import { spanToTraceContext } from '../../../src/utils/spanUtils'; -import { getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../../../src/utils/spanUtils'; +import { + getRootSpan, + spanIsSampled, + spanTimeInputToSeconds, + spanToJSON, + spanToTraceContext, + updateSpanName, +} from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; function createMockedOtelSpan({ @@ -332,3 +339,13 @@ describe('getRootSpan', () => { }); }); }); + +describe('updateSpanName', () => { + it('updates the span name and source', () => { + const span = new SentrySpan({ name: 'old-name', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' } }); + updateSpanName(span, 'new-name'); + const spanJSON = spanToJSON(span); + expect(spanJSON.description).toBe('new-name'); + expect(spanJSON.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toBe('custom'); + }); +}); diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index a1aa47e5b6ce..6e15c74af12a 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -17,6 +17,7 @@ import type { SpanAttributes, TransactionSource } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment, @@ -36,12 +37,12 @@ interface SpanDescription { /** * Infer the op & description for a set of name, attributes and kind of a span. */ -export function inferSpanData(name: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription { +export function inferSpanData(originalName: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription { // if http.method exists, this is an http request span // eslint-disable-next-line deprecation/deprecation const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD]; if (httpMethod) { - return descriptionForHttpMethod({ attributes, name, kind }, httpMethod); + return descriptionForHttpMethod({ attributes, name: originalName, kind }, httpMethod); } // eslint-disable-next-line deprecation/deprecation @@ -53,17 +54,19 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp // If db.type exists then this is a database call span // If the Redis DB is used as a cache, the span description should not be changed if (dbSystem && !opIsCache) { - return descriptionForDbSystem({ attributes, name }); + return descriptionForDbSystem({ attributes, name: originalName }); } + const customSourceOrRoute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' ? 'custom' : 'route'; + // If rpc.service exists then this is a rpc call span. // eslint-disable-next-line deprecation/deprecation const rpcService = attributes[SEMATTRS_RPC_SERVICE]; if (rpcService) { return { op: 'rpc', - description: name, - source: 'route', + description: originalName, + source: customSourceOrRoute, }; } @@ -73,8 +76,8 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp if (messagingSystem) { return { op: 'message', - description: name, - source: 'route', + description: originalName, + source: customSourceOrRoute, }; } @@ -82,15 +85,19 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp // eslint-disable-next-line deprecation/deprecation const faasTrigger = attributes[SEMATTRS_FAAS_TRIGGER]; if (faasTrigger) { - return { op: faasTrigger.toString(), description: name, source: 'route' }; + return { op: faasTrigger.toString(), description: originalName, source: customSourceOrRoute }; } - return { op: undefined, description: name, source: 'custom' }; + return { op: undefined, description: originalName, source: 'custom' }; } /** * Extract better op/description from an otel span. * + * Does not overwrite the span name if the source is already set to custom to ensure + * that user-updated span names are preserved. In this case, we only adjust the op but + * leave span description and source unchanged. + * * Based on https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7422ce2a06337f68a59b552b8c5a2ac125d6bae5/exporter/sentryexporter/sentry_exporter.go#L306 */ export function parseSpanDescription(span: AbstractSpan): SpanDescription { @@ -102,6 +109,11 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { } 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 (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom') { + return { op: 'db', description: name, source: 'custom' }; + } + // Use DB statement (Ex "SELECT * FROM table") if possible as description. // eslint-disable-next-line deprecation/deprecation const statement = attributes[SEMATTRS_DB_STATEMENT]; @@ -174,7 +186,10 @@ export function descriptionForHttpMethod( const origin = attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] || 'manual'; const isManualSpan = !`${origin}`.startsWith('auto'); - const useInferredDescription = isClientOrServerKind || !isManualSpan; + // If users (or in very rare occasions we) set the source to custom, we don't overwrite it + const alreadyHasCustomSource = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom'; + + const useInferredDescription = !alreadyHasCustomSource && (isClientOrServerKind || !isManualSpan); return { op: opParts.join('.'), diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index c44645c62888..36eb87b94fe6 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -15,6 +15,7 @@ import { SEMATTRS_RPC_SERVICE, } from '@opentelemetry/semantic-conventions'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import { descriptionForHttpMethod, getSanitizedUrl, parseSpanDescription } from '../../src/utils/parseSpanDescription'; describe('parseSpanDescription', () => { @@ -81,6 +82,21 @@ describe('parseSpanDescription', () => { source: 'task', }, ], + [ + 'works with db system and custom source', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_DB_SYSTEM]: 'mysql', + [SEMATTRS_DB_STATEMENT]: 'SELECT * from users', + }, + 'test name', + SpanKind.CLIENT, + { + description: 'test name', + op: 'db', + source: 'custom', + }, + ], [ 'works with db system without statement', { @@ -107,6 +123,20 @@ describe('parseSpanDescription', () => { source: 'route', }, ], + [ + 'works with rpc service and custom source', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_RPC_SERVICE]: 'rpc-test-service', + }, + 'test name', + undefined, + { + description: 'test name', + op: 'rpc', + source: 'custom', + }, + ], [ 'works with messaging system', { @@ -120,6 +150,20 @@ describe('parseSpanDescription', () => { source: 'route', }, ], + [ + 'works with messaging system and custom source', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_MESSAGING_SYSTEM]: 'test-messaging-system', + }, + 'test name', + undefined, + { + description: 'test name', + op: 'message', + source: 'custom', + }, + ], [ 'works with faas trigger', { @@ -133,6 +177,20 @@ describe('parseSpanDescription', () => { source: 'route', }, ], + [ + 'works with faas trigger and custom source', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_FAAS_TRIGGER]: 'test-faas-trigger', + }, + 'test name', + undefined, + { + description: 'test name', + op: 'test-faas-trigger', + source: 'custom', + }, + ], ])('%s', (_, attributes, name, kind, expected) => { const actual = parseSpanDescription({ attributes, kind, name } as unknown as Span); expect(actual).toEqual(expected); @@ -172,6 +230,26 @@ describe('descriptionForHttpMethod', () => { source: 'url', }, ], + [ + 'works with prefetch request', + 'GET', + { + [SEMATTRS_HTTP_METHOD]: 'GET', + [SEMATTRS_HTTP_URL]: 'https://www.example.com/my-path', + [SEMATTRS_HTTP_TARGET]: '/my-path', + 'sentry.http.prefetch': true, + }, + 'test name', + SpanKind.CLIENT, + { + op: 'http.client.prefetch', + description: 'GET https://www.example.com/my-path', + data: { + url: 'https://www.example.com/my-path', + }, + source: 'url', + }, + ], [ 'works with basic server POST', 'POST', @@ -230,6 +308,27 @@ describe('descriptionForHttpMethod', () => { source: 'custom', }, ], + [ + "doesn't overwrite name with source custom", + '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]: 'custom', + }, + 'test name', + SpanKind.CLIENT, + { + op: 'http.client', + description: 'test name', + data: { + url: 'https://www.example.com/my-path/123', + }, + source: 'custom', + }, + ], ])('%s', (_, httpMethod, attributes, name, kind, expected) => { const actual = descriptionForHttpMethod({ attributes, kind, name }, httpMethod); expect(actual).toEqual(expected); From 7886728fbf2a1ffb256df361298d1dfcbcd4d462 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 12:37:32 +0100 Subject: [PATCH 02/18] add exports --- packages/astro/src/index.server.ts | 1 + packages/aws-serverless/src/index.ts | 1 + packages/browser/src/exports.ts | 1 + packages/bun/src/index.ts | 1 + packages/cloudflare/src/index.ts | 1 + packages/core/src/index.ts | 1 + packages/deno/src/index.ts | 1 + packages/google-cloud-serverless/src/index.ts | 1 + packages/node/src/index.ts | 1 + packages/remix/src/index.server.ts | 1 + packages/solidstart/src/server/index.ts | 1 + packages/sveltekit/src/server/index.ts | 1 + 12 files changed, 12 insertions(+) diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 7eca9de9a41a..ad7481816b4d 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -138,6 +138,7 @@ export { startSpanManual, tediousIntegration, trpcMiddleware, + updateSpanName, withActiveSpan, withIsolationScope, withMonitor, diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index 3f167b62a7e3..5146fe423b45 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -121,6 +121,7 @@ export { spanToTraceHeader, spanToBaggageHeader, trpcMiddleware, + updateSpanName, // eslint-disable-next-line deprecation/deprecation addOpenTelemetryInstrumentation, zodErrorsIntegration, diff --git a/packages/browser/src/exports.ts b/packages/browser/src/exports.ts index 492f9da23b38..295e6daa36cc 100644 --- a/packages/browser/src/exports.ts +++ b/packages/browser/src/exports.ts @@ -62,6 +62,7 @@ export { spanToJSON, spanToTraceHeader, spanToBaggageHeader, + updateSpanName, } from '@sentry/core'; export { diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index 1ba5f2de4786..dcae6e98aa8d 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -141,6 +141,7 @@ export { spanToTraceHeader, spanToBaggageHeader, trpcMiddleware, + updateSpanName, // eslint-disable-next-line deprecation/deprecation addOpenTelemetryInstrumentation, zodErrorsIntegration, diff --git a/packages/cloudflare/src/index.ts b/packages/cloudflare/src/index.ts index f3c80b8ddf32..fb8c34694282 100644 --- a/packages/cloudflare/src/index.ts +++ b/packages/cloudflare/src/index.ts @@ -89,6 +89,7 @@ export { spanToJSON, spanToTraceHeader, spanToBaggageHeader, + updateSpanName, } from '@sentry/core'; export { withSentry } from './handler'; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 77259d2434d4..5baf88f38e1c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -81,6 +81,7 @@ export { getActiveSpan, addChildSpanToSpan, spanTimeInputToSeconds, + updateSpanName, } from './utils/spanUtils'; export { parseSampleRate } from './utils/parseSampleRate'; export { applySdkMetadata } from './utils/sdkMetadata'; diff --git a/packages/deno/src/index.ts b/packages/deno/src/index.ts index 892f6ce681c0..cea4effad4bd 100644 --- a/packages/deno/src/index.ts +++ b/packages/deno/src/index.ts @@ -89,6 +89,7 @@ export { spanToJSON, spanToTraceHeader, spanToBaggageHeader, + updateSpanName, } from '@sentry/core'; export { DenoClient } from './client'; diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index 6f89769c2a37..95d30ec8b072 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -118,6 +118,7 @@ export { spanToTraceHeader, spanToBaggageHeader, trpcMiddleware, + updateSpanName, // eslint-disable-next-line deprecation/deprecation addOpenTelemetryInstrumentation, zodErrorsIntegration, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index fa16ac4e6b3d..215d53bdbde3 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -142,6 +142,7 @@ export { spanToTraceHeader, spanToBaggageHeader, trpcMiddleware, + updateSpanName, zodErrorsIntegration, profiler, } from '@sentry/core'; diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index f6a5f5060dd9..4bb6539dbd33 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -134,6 +134,7 @@ export { startSpanManual, tediousIntegration, trpcMiddleware, + updateSpanName, withActiveSpan, withIsolationScope, withMonitor, diff --git a/packages/solidstart/src/server/index.ts b/packages/solidstart/src/server/index.ts index 450420a2b586..4c1f192b0c36 100644 --- a/packages/solidstart/src/server/index.ts +++ b/packages/solidstart/src/server/index.ts @@ -126,6 +126,7 @@ export { startSpanManual, tediousIntegration, trpcMiddleware, + updateSpanName, withActiveSpan, withIsolationScope, withMonitor, diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index bb88e121244f..4996dcc0e7ca 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -128,6 +128,7 @@ export { startSpanManual, tediousIntegration, trpcMiddleware, + updateSpanName, withActiveSpan, withIsolationScope, withMonitor, From 3028f4588b39bc32e1ca23315a6a51d088f2090e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 15:57:55 +0100 Subject: [PATCH 03/18] node tests and jsdoc --- .../express/tracing/updateName/server.js | 50 ++++++++++++++ .../suites/express/tracing/updateName/test.ts | 67 +++++++++++++++++++ .../public-api/startSpan/basic-usage/test.ts | 17 ++++- .../startSpan/updateName-method/scenario.ts | 16 +++++ .../startSpan/updateName-method/test.ts | 24 +++++++ .../updateSpanName-function/scenario.ts | 16 +++++ .../startSpan/updateSpanName-function/test.ts | 24 +++++++ .../suites/tracing/httpIntegration/test.ts | 6 ++ packages/core/src/envelope.ts | 8 +++ packages/core/src/types-hoist/span.ts | 16 +++++ packages/core/src/utils/spanUtils.ts | 1 + .../src/utils/parseSpanDescription.ts | 32 ++++++--- 12 files changed, 265 insertions(+), 12 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts 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 new file mode 100644 index 000000000000..cff6a955eea2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/updateName/server.js @@ -0,0 +1,50 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/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' }); +}); + +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 new file mode 100644 index 000000000000..ec3346dad096 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts @@ -0,0 +1,67 @@ +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: { + data: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom' }, + }, + }, + }); + // 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/updateSpanName'); + }); + }); +}); 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 86b3bf6d9d22..cb01ee8b4098 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 @@ -1,11 +1,24 @@ +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); }); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/scenario.ts new file mode 100644 index 000000000000..ea30608c1c5c --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/scenario.ts @@ -0,0 +1,16 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/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'); + }, +); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts new file mode 100644 index 000000000000..676071926b91 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateName-method/test.ts @@ -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); +}); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/scenario.ts new file mode 100644 index 000000000000..ecf7670fa23d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/scenario.ts @@ -0,0 +1,16 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/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'); + }, +); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts new file mode 100644 index 000000000000..c5b325fc0ea2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/updateSpanName-function/test.ts @@ -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 and source when calling `updateSpanName`', done => { + createRunner(__dirname, 'scenario.ts') + .expect({ + transaction: { + transaction: 'new name', + 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/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts index 7fc6a5f05efa..c36159f09931 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts @@ -169,4 +169,10 @@ describe('httpIntegration', () => { runner.makeRequest('get', '/testRequest'); }); }); + + describe('does not override the span name set by the user', () => { + test('via `span.updateName`', done => { + createRunner(__dirname, 'server-updateName.js').start(done); + }); + }); }); diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 999fb0681cf0..82800b0179c5 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -95,6 +95,14 @@ export function createEventEnvelope( // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid // of this `delete`, lest we miss putting it back in the next time the property is in use.) delete event.sdkProcessingMetadata; + try { + // @ts-expect-error - for bundle size we try/catch the access to this property + delete event.contexts.trace.data._sentry_span_name_set_by_user; + // @ts-expect-error - for bundle size we try/catch the access to this property + event.spans.forEach(span => delete span.data._sentry_span_name_set_by_user); + } catch { + // Do nothing + } const eventItem: EventItem = [{ type: eventType }, event]; return createEnvelope(envelopeHeaders, [eventItem]); diff --git a/packages/core/src/types-hoist/span.ts b/packages/core/src/types-hoist/span.ts index a2ee74fd7cfa..3a65f01573c1 100644 --- a/packages/core/src/types-hoist/span.ts +++ b/packages/core/src/types-hoist/span.ts @@ -234,6 +234,22 @@ export interface Span { /** * Update the name of the span. + * + * **Important:** You most likely want to use `Sentry.updateSpanName(span, name)` instead! + * + * This method will update the current span name but cannot guarantee that the new name will be + * the final name of the span. Instrumentation might still overwrite the name with an automatically + * computed name, for example in `http.server` or `db` spans. + * + * You can ensure that your name is kept and not overwritten by + * - either calling `Sentry.updateSpanName(span, name)` + * - or by calling `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom')` + * in addition to `span.updateName`. + * + * If you want to update a span name in a browser-only app, `span.updateName` and `Sentry.updateSpanName` + * are identical: In both cases, the name will not be overwritten by the Sentry SDK. + * + * @param name the new name of the span */ updateName(name: string): this; diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index fa3592fe5ada..4655cea14b9c 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -334,4 +334,5 @@ 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); } diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 6e15c74af12a..4b2fb96490e6 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -37,12 +37,12 @@ interface SpanDescription { /** * Infer the op & description for a set of name, attributes and kind of a span. */ -export function inferSpanData(originalName: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription { +export function inferSpanData(spanName: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription { // if http.method exists, this is an http request span // eslint-disable-next-line deprecation/deprecation const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD]; if (httpMethod) { - return descriptionForHttpMethod({ attributes, name: originalName, kind }, httpMethod); + return descriptionForHttpMethod({ attributes, name: getOriginalName(spanName, attributes), kind }, httpMethod); } // eslint-disable-next-line deprecation/deprecation @@ -54,7 +54,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes, // If db.type exists then this is a database call span // If the Redis DB is used as a cache, the span description should not be changed if (dbSystem && !opIsCache) { - return descriptionForDbSystem({ attributes, name: originalName }); + return descriptionForDbSystem({ attributes, name: spanName }); } const customSourceOrRoute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' ? 'custom' : 'route'; @@ -65,7 +65,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes, if (rpcService) { return { op: 'rpc', - description: originalName, + description: getOriginalName(spanName, attributes), source: customSourceOrRoute, }; } @@ -76,7 +76,7 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes, if (messagingSystem) { return { op: 'message', - description: originalName, + description: getOriginalName(spanName, attributes), source: customSourceOrRoute, }; } @@ -85,10 +85,14 @@ export function inferSpanData(originalName: string, attributes: SpanAttributes, // eslint-disable-next-line deprecation/deprecation const faasTrigger = attributes[SEMATTRS_FAAS_TRIGGER]; if (faasTrigger) { - return { op: faasTrigger.toString(), description: originalName, source: customSourceOrRoute }; + return { + op: faasTrigger.toString(), + description: getOriginalName(spanName, attributes), + source: customSourceOrRoute, + }; } - return { op: undefined, description: originalName, source: 'custom' }; + return { op: undefined, description: spanName, source: 'custom' }; } /** @@ -111,7 +115,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { 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 (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom') { - return { op: 'db', description: name, source: 'custom' }; + return { op: 'db', description: getOriginalName(name, attributes), source: 'custom' }; } // Use DB statement (Ex "SELECT * FROM table") if possible as description. @@ -147,7 +151,7 @@ export function descriptionForHttpMethod( const { urlPath, url, query, fragment, hasRoute } = getSanitizedUrl(attributes, kind); if (!urlPath) { - return { op: opParts.join('.'), description: name, source: 'custom' }; + return { op: opParts.join('.'), description: getOriginalName(name, attributes), source: 'custom' }; } const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION]; @@ -193,7 +197,7 @@ export function descriptionForHttpMethod( return { op: opParts.join('.'), - description: useInferredDescription ? description : name, + description: useInferredDescription ? description : getOriginalName(name, attributes), source: useInferredDescription ? source : 'custom', data, }; @@ -259,3 +263,11 @@ export function getSanitizedUrl( return { urlPath: undefined, url, query, fragment, hasRoute: false }; } + +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; +} From 1cfb1c2ed759f3ed96450b4043ba48515e2e15db Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 16:34:24 +0100 Subject: [PATCH 04/18] browser integration test --- .../pageload-update-txn-name/subject.js | 2 +- .../pageload-update-txn-name/test.ts | 43 +++++++++++-------- .../pageload-updateSpanName/init.js | 10 +++++ .../pageload-updateSpanName/subject.js | 4 ++ .../pageload-updateSpanName/test.ts | 43 +++++++++++++++++++ .../tracing/dsc-txn-name-update/test.ts | 4 ++ 6 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts 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 6e93018cc063..cb5bc5e3336b 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/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts index 6226ff75dbb9..64584672e18b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts @@ -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(page, url); + const eventData = await getFirstSentryEnvelopeRequest(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!._sentry_span_name_set_by_user).toBeUndefined(); + + expect(eventData.contexts?.trace?.op).toBe('pageload'); + expect(eventData.spans?.length).toBeGreaterThan(0); + expect(eventData.transaction_info?.source).toEqual('custom'); + }, +); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/init.js new file mode 100644 index 000000000000..1f0b64911a75 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/init.js @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; +window._testBaseTimestamp = performance.timeOrigin / 1000; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.browserTracingIntegration()], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/subject.js new file mode 100644 index 000000000000..7f0ad0fea340 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/subject.js @@ -0,0 +1,4 @@ +const activeSpan = Sentry.getActiveSpan(); +const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan); + +Sentry.updateSpanName(rootSpan, 'new name'); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts new file mode 100644 index 000000000000..c49a1b3116a1 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts @@ -0,0 +1,43 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +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 ({ getLocalTestPath, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(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!._sentry_span_name_set_by_user).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'); + }, +); diff --git a/dev-packages/browser-integration-tests/suites/tracing/dsc-txn-name-update/test.ts b/dev-packages/browser-integration-tests/suites/tracing/dsc-txn-name-update/test.ts index 7ce5f7195a5b..34e15d1be573 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/dsc-txn-name-update/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/dsc-txn-name-update/test.ts @@ -181,5 +181,9 @@ async function captureErrorAndGetEnvelopeTraceHeader(page: Page): Promise Date: Thu, 14 Nov 2024 16:34:34 +0100 Subject: [PATCH 05/18] more unit tests --- .../src/utils/parseSpanDescription.ts | 8 +- .../test/utils/parseSpanDescription.test.ts | 115 +++++++++++++++++- 2 files changed, 120 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 4b2fb96490e6..963475128be9 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -264,7 +264,13 @@ export function getSanitizedUrl( return { urlPath: undefined, url, query, fragment, hasRoute: false }; } -function getOriginalName(name: string, attributes: Attributes): string { +/** + * 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. + * We delete this attribute once we're done with it when preparing the event envelope. + * @internal + */ +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' diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index 36eb87b94fe6..a9107109444e 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -16,7 +16,12 @@ import { } from '@opentelemetry/semantic-conventions'; import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; -import { descriptionForHttpMethod, getSanitizedUrl, parseSpanDescription } from '../../src/utils/parseSpanDescription'; +import { + descriptionForHttpMethod, + getOriginalName, + getSanitizedUrl, + parseSpanDescription, +} from '../../src/utils/parseSpanDescription'; describe('parseSpanDescription', () => { it.each([ @@ -97,6 +102,22 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with db system and custom source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [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: 'custom', + }, + ], [ 'works with db system without statement', { @@ -137,6 +158,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with rpc service and custom source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_RPC_SERVICE]: 'rpc-test-service', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + undefined, + { + description: 'custom name', + op: 'rpc', + source: 'custom', + }, + ], [ 'works with messaging system', { @@ -164,6 +200,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with messaging system and custom source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [SEMATTRS_MESSAGING_SYSTEM]: 'test-messaging-system', + ['_sentry_span_name_set_by_user']: 'custom name', + }, + 'test name', + undefined, + { + description: 'custom name', + op: 'message', + source: 'custom', + }, + ], [ 'works with faas trigger', { @@ -191,6 +242,21 @@ describe('parseSpanDescription', () => { source: 'custom', }, ], + [ + 'works with faas trigger and custom source and custom name', + { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + [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: 'custom', + }, + ], ])('%s', (_, attributes, name, kind, expected) => { const actual = parseSpanDescription({ attributes, kind, name } as unknown as Span); expect(actual).toEqual(expected); @@ -309,7 +375,7 @@ describe('descriptionForHttpMethod', () => { }, ], [ - "doesn't overwrite name with source custom", + "doesn't overwrite span name with source custom", 'GET', { [SEMATTRS_HTTP_METHOD]: 'GET', @@ -329,6 +395,28 @@ describe('descriptionForHttpMethod', () => { source: 'custom', }, ], + [ + 'takes user-passed span name (with source custom)', + '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]: 'custom', + ['_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: 'custom', + }, + ], ])('%s', (_, httpMethod, attributes, name, kind, expected) => { const actual = descriptionForHttpMethod({ attributes, kind, name }, httpMethod); expect(actual).toEqual(expected); @@ -482,3 +570,26 @@ describe('getSanitizedUrl', () => { expect(actual).toEqual(expected); }); }); + +describe('getOriginalName', () => { + it('returns param name if source is not custom', () => { + expect(getOriginalName('base name', {})).toBe('base name'); + }); + + 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 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'); + }); + + 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'); + }); +}); From 914ce83a3d3d7ef18e14c276a109287537567cbe Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 11 Dec 2024 11:52:21 +0100 Subject: [PATCH 06/18] possibly fix minified bundle tests --- dev-packages/rollup-utils/plugins/bundlePlugins.mjs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index dce0ca15bf35..080a3c118d55 100644 --- a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs +++ b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs @@ -140,6 +140,8 @@ export function makeTerserPlugin() { '_isShim', // This is used in metadata integration '_sentryModuleMetadata', + // _sentry_span_name_set_by_user is set by the user and is used to store the original span name + '_sentry_span_name_set_by_user', ], }, }, From dcde0386f2590bb38c8a2d26f09a16902f5f15fa Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 17:04:04 +0100 Subject: [PATCH 07/18] remove unnecessary test --- .../suites/tracing/httpIntegration/test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts index c36159f09931..7fc6a5f05efa 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts @@ -169,10 +169,4 @@ describe('httpIntegration', () => { runner.makeRequest('get', '/testRequest'); }); }); - - describe('does not override the span name set by the user', () => { - test('via `span.updateName`', done => { - createRunner(__dirname, 'server-updateName.js').start(done); - }); - }); }); From 44167222a73aea1c0104ac2277aca91ac7273580 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Nov 2024 17:52:35 +0100 Subject: [PATCH 08/18] jsdoc update --- packages/core/src/types-hoist/span.ts | 5 +---- packages/opentelemetry/src/utils/parseSpanDescription.ts | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/core/src/types-hoist/span.ts b/packages/core/src/types-hoist/span.ts index 3a65f01573c1..08178aaf689f 100644 --- a/packages/core/src/types-hoist/span.ts +++ b/packages/core/src/types-hoist/span.ts @@ -241,10 +241,7 @@ export interface Span { * the final name of the span. Instrumentation might still overwrite the name with an automatically * computed name, for example in `http.server` or `db` spans. * - * You can ensure that your name is kept and not overwritten by - * - either calling `Sentry.updateSpanName(span, name)` - * - or by calling `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom')` - * in addition to `span.updateName`. + * You can ensure that your name is kept and not overwritten by calling `Sentry.updateSpanName(span, name)` * * If you want to update a span name in a browser-only app, `span.updateName` and `Sentry.updateSpanName` * are identical: In both cases, the name will not be overwritten by the Sentry SDK. diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 963475128be9..993fc2fced8e 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -268,7 +268,7 @@ 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. * We delete this attribute once we're done with it when preparing the event envelope. - * @internal + * @internal exported only for testing */ export function getOriginalName(name: string, attributes: Attributes): string { return attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'custom' && From 95348109a4b4001359a3042d9a64c469f993121f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 15 Nov 2024 19:08:23 +0100 Subject: [PATCH 09/18] adjust parseSpanDescription and tests --- .../pageload-update-txn-name/subject.js | 2 +- .../express/tracing/updateName/server.js | 8 ++ .../suites/express/tracing/updateName/test.ts | 26 ++++ .../public-api/startSpan/basic-usage/test.ts | 18 +++ packages/core/src/utils/spanUtils.ts | 6 +- .../src/utils/parseSpanDescription.ts | 84 ++++++++---- .../test/utils/parseSpanDescription.test.ts | 125 +++++++++++++++--- 7 files changed, 226 insertions(+), 43 deletions(-) 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, + }); + }, + ); }); From 5e69e6a718ba9eb43df829b5c7eeba7836e806ef Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 12 Dec 2024 17:45:19 +0100 Subject: [PATCH 10/18] fix integration test --- .../pageload-updateSpanName/test.ts | 6 +++--- .../opentelemetry/test/utils/parseSpanDescription.test.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts index c49a1b3116a1..2dad90486b73 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { Event } from '@sentry/types'; +import type { Event } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -12,12 +12,12 @@ import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../. sentryTest( 'sets the source to custom when updating the transaction name with Sentry.updateSpanName', - async ({ getLocalTestPath, page }) => { + async ({ getLocalTestUrl, page }) => { if (shouldSkipTracingTest()) { sentryTest.skip(); } - const url = await getLocalTestPath({ testDir: __dirname }); + const url = await getLocalTestUrl({ testDir: __dirname }); const eventData = await getFirstSentryEnvelopeRequest(page, url); diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index 6656ccac1c4e..2e87808568a5 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -18,8 +18,8 @@ import { import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import { descriptionForHttpMethod, - getUserUpdatedNameAndSource, getSanitizedUrl, + getUserUpdatedNameAndSource, parseSpanDescription, } from '../../src/utils/parseSpanDescription'; From 99dc49adfe39c4097ab833398453020dad6688a8 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 12 Dec 2024 19:31:42 +0100 Subject: [PATCH 11/18] size limit --- .size-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index 6e73c9234c09..45324236f6ac 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -93,7 +93,7 @@ module.exports = [ path: 'packages/browser/build/npm/esm/index.js', import: createImport('init', 'feedbackIntegration'), gzip: true, - limit: '41 KB', + limit: '42 KB', }, { name: '@sentry/browser (incl. sendFeedback)', From 64437edc9877df21d24ca5287b6b0475e4300b0b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 10:18:57 +0100 Subject: [PATCH 12/18] s/_sentry_span_name_set_by_user/SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME --- .../pageload-update-txn-name/test.ts | 4 +-- .../pageload-updateSpanName/test.ts | 4 +-- .../suites/express/tracing/updateName/test.ts | 5 +-- .../rollup-utils/plugins/bundlePlugins.mjs | 2 -- packages/core/src/envelope.ts | 7 ++-- packages/core/src/semanticAttributes.ts | 9 +++++ packages/core/src/utils/spanUtils.ts | 3 +- .../src/utils/parseSpanDescription.ts | 17 ++++++---- .../test/utils/parseSpanDescription.test.ts | 34 +++++++++---------- 9 files changed, 50 insertions(+), 35 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts index 64584672e18b..a957418852db 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { Event } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, type Event } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -34,7 +34,7 @@ sentryTest( [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', }); - expect(traceContextData!._sentry_span_name_set_by_user).toBeUndefined(); + expect(traceContextData![SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined(); expect(eventData.contexts?.trace?.op).toBe('pageload'); expect(eventData.spans?.length).toBeGreaterThan(0); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts index 2dad90486b73..2113c26c549d 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import type { Event } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, type Event } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -32,7 +32,7 @@ sentryTest( [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', }); - expect(traceContextData!._sentry_span_name_set_by_user).toBeUndefined(); + expect(traceContextData![SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined(); expect(eventData.transaction).toBe('new name'); 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 a9ef1fa2a1d0..e092f200265b 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 @@ -1,5 +1,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node'; import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core'; describe('express tracing', () => { afterAll(() => { @@ -58,7 +59,7 @@ describe('express tracing', () => { }, }); // ensure we delete the internal attribute once we're done with it - expect(txnEvent.contexts?.trace?.data?.['_sentry_span_name_set_by_user']).toBeUndefined(); + expect(txnEvent.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined(); }, }) .start(done) @@ -84,7 +85,7 @@ describe('express tracing', () => { }, }); // ensure we delete the internal attribute once we're done with it - expect(txnEvent.contexts?.trace?.data?.['_sentry_span_name_set_by_user']).toBeUndefined(); + expect(txnEvent.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]).toBeUndefined(); }, }) .start(done) diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index 080a3c118d55..dce0ca15bf35 100644 --- a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs +++ b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs @@ -140,8 +140,6 @@ export function makeTerserPlugin() { '_isShim', // This is used in metadata integration '_sentryModuleMetadata', - // _sentry_span_name_set_by_user is set by the user and is used to store the original span name - '_sentry_span_name_set_by_user', ], }, }, diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 82800b0179c5..d8e43a37d40f 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,3 +1,4 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from './semanticAttributes'; import { getDynamicSamplingContextFromSpan } from './tracing/dynamicSamplingContext'; import type { SentrySpan } from './tracing/sentrySpan'; import type { @@ -97,9 +98,11 @@ export function createEventEnvelope( delete event.sdkProcessingMetadata; try { // @ts-expect-error - for bundle size we try/catch the access to this property - delete event.contexts.trace.data._sentry_span_name_set_by_user; + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; // @ts-expect-error - for bundle size we try/catch the access to this property - event.spans.forEach(span => delete span.data._sentry_span_name_set_by_user); + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + event.spans.forEach(span => delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]); } catch { // Do nothing } diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts index 2896bd81f93f..b799f5321a0e 100644 --- a/packages/core/src/semanticAttributes.ts +++ b/packages/core/src/semanticAttributes.ts @@ -29,6 +29,15 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_UNIT = 'sentry.measurement_un /** The value of a measurement, which may be stored as a TimedEvent. */ export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE = 'sentry.measurement_value'; +/** + * A custom span name set by users guaranteed to be taken over any automatically + * inferred name. This attribute is removed before the span is sent. + * + * @internal only meant for internal SDK usage + * @hidden + */ +export const SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME = 'sentry.custom_span_name'; + /** * The id of the profile that this span occurred in. */ diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index b69f7597796c..09ba9d729449 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -4,6 +4,7 @@ import { getCurrentScope } from '../currentScopes'; import { getMetricSummaryJsonForSpan, updateMetricSummaryOnSpan } from '../metrics/metric-summary'; import type { MetricType } from '../metrics/types'; import { + SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -335,6 +336,6 @@ export function updateSpanName(span: Span, name: string): void { span.updateName(name); span.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ['_sentry_span_name_set_by_user']: name, + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: name, }); } diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index e800d6a48109..6e197366cd6e 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -15,6 +15,7 @@ import { } from '@opentelemetry/semantic-conventions'; import type { SpanAttributes, TransactionSource } from '@sentry/core'; import { + SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -115,10 +116,11 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription { // 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') { + const userDefinedName = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; + if (typeof userDefinedName === 'string') { return { op: 'db', - description: attributes['_sentry_span_name_set_by_user'], + description: userDefinedName, source: (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || 'custom', }; } @@ -202,11 +204,10 @@ export function descriptionForHttpMethod( // 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 customSpanName = attributes[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; const useInferredDescription = - !alreadyHasCustomSource && - attributes['_sentry_span_name_set_by_user'] == null && - (isClientOrServerKind || !isManualSpan); + !alreadyHasCustomSource && customSpanName == null && (isClientOrServerKind || !isManualSpan); const { description, source } = useInferredDescription ? { description: inferredDescription, source: inferredSource } @@ -302,12 +303,14 @@ export function getUserUpdatedNameAndSource( source: TransactionSource; } { const source = (attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource) || fallbackSource; + const description = attributes[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; - if (attributes['_sentry_span_name_set_by_user'] && typeof attributes['_sentry_span_name_set_by_user'] === 'string') + if (description && typeof description === 'string') { return { - description: attributes['_sentry_span_name_set_by_user'], + description, source, }; + } return { description: originalName, source }; } diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index 2e87808568a5..d43dfcd9f587 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -15,7 +15,7 @@ import { SEMATTRS_RPC_SERVICE, } from '@opentelemetry/semantic-conventions'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import { descriptionForHttpMethod, getSanitizedUrl, @@ -108,7 +108,7 @@ describe('parseSpanDescription', () => { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', [SEMATTRS_DB_SYSTEM]: 'mysql', [SEMATTRS_DB_STATEMENT]: 'SELECT * from users', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', SpanKind.CLIENT, @@ -124,7 +124,7 @@ describe('parseSpanDescription', () => { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', [SEMATTRS_DB_SYSTEM]: 'mysql', [SEMATTRS_DB_STATEMENT]: 'SELECT * from users', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', SpanKind.CLIENT, @@ -179,7 +179,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', [SEMATTRS_RPC_SERVICE]: 'rpc-test-service', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -194,7 +194,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', [SEMATTRS_RPC_SERVICE]: 'rpc-test-service', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -236,7 +236,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', [SEMATTRS_MESSAGING_SYSTEM]: 'test-messaging-system', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -251,7 +251,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', [SEMATTRS_MESSAGING_SYSTEM]: 'test-messaging-system', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -293,7 +293,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', [SEMATTRS_FAAS_TRIGGER]: 'test-faas-trigger', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -308,7 +308,7 @@ describe('parseSpanDescription', () => { { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', [SEMATTRS_FAAS_TRIGGER]: 'test-faas-trigger', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', undefined, @@ -465,7 +465,7 @@ describe('descriptionForHttpMethod', () => { [SEMATTRS_HTTP_TARGET]: '/my-path/123', [ATTR_HTTP_ROUTE]: '/my-path/:id', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ['_sentry_span_name_set_by_user']: 'custom name', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', SpanKind.CLIENT, @@ -487,7 +487,7 @@ describe('descriptionForHttpMethod', () => { [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', + [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 'custom name', }, 'test name', SpanKind.CLIENT, @@ -655,30 +655,30 @@ describe('getSanitizedUrl', () => { }); describe('getUserUpdatedNameAndSource', () => { - it('returns param name if `_sentry_span_name_set_by_user` attribute is not set', () => { + it('returns param name if `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` attribute is not set', () => { expect(getUserUpdatedNameAndSource('base name', {})).toEqual({ description: 'base name', source: 'custom' }); }); - it('returns param name with custom fallback source if `_sentry_span_name_set_by_user` attribute is not set', () => { + it('returns param name with custom fallback source if `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` 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(getUserUpdatedNameAndSource('base name', { ['_sentry_span_name_set_by_user']: 123 })).toEqual({ + it('returns param name if `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` attribute is not a string', () => { + expect(getUserUpdatedNameAndSource('base name', { [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: 123 })).toEqual({ description: 'base name', source: 'custom', }); }); it.each(['custom', 'task', 'url', 'route'])( - 'returns `_sentry_span_name_set_by_user` attribute if is a string and source is %s', + 'returns `SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME` 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_CUSTOM_SPAN_NAME]: 'custom name', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, }), ).toEqual({ From 1772a77a589b81d33bb3769ed1558fd6932654d1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 10:20:49 +0100 Subject: [PATCH 13/18] remove impl detail in Span JSDoc --- packages/core/src/types-hoist/span.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/core/src/types-hoist/span.ts b/packages/core/src/types-hoist/span.ts index 08178aaf689f..cf0c3086bf88 100644 --- a/packages/core/src/types-hoist/span.ts +++ b/packages/core/src/types-hoist/span.ts @@ -243,9 +243,6 @@ export interface Span { * * You can ensure that your name is kept and not overwritten by calling `Sentry.updateSpanName(span, name)` * - * If you want to update a span name in a browser-only app, `span.updateName` and `Sentry.updateSpanName` - * are identical: In both cases, the name will not be overwritten by the Sentry SDK. - * * @param name the new name of the span */ updateName(name: string): this; From 24e83207dceaecb4a8858db99792f649c609dcdf Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 10:33:01 +0100 Subject: [PATCH 14/18] remove span attributes separately for otel and sentry spans --- packages/core/src/tracing/sentrySpan.ts | 9 +++++++++ packages/opentelemetry/src/spanExporter.ts | 2 ++ 2 files changed, 11 insertions(+) diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index 126702dfad2b..9965261970f2 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -5,6 +5,7 @@ import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary'; import { SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, SEMANTIC_ATTRIBUTE_PROFILE_ID, + SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -355,6 +356,14 @@ export class SentrySpan implements Span { const source = this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] as TransactionSource | undefined; + // remove internal root span attributes we don't need to send. + /* eslint-disable @typescript-eslint/no-dynamic-delete */ + delete this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; + spans.forEach(span => { + span.data && delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; + }); + // eslint-enabled-next-line @typescript-eslint/no-dynamic-delete + const transaction: TransactionEvent = { contexts: { trace: spanToTransactionTraceContext(this), diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index a8d5affa4646..bff6518eb27d 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -12,6 +12,7 @@ import type { TransactionSource, } from '@sentry/core'; import { + SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, @@ -392,6 +393,7 @@ function removeSentryAttributes(data: Record): Record Date: Fri, 13 Dec 2024 10:43:10 +0100 Subject: [PATCH 15/18] biome --- .../browserTracingIntegration/pageload-update-txn-name/test.ts | 2 +- .../browserTracingIntegration/pageload-updateSpanName/test.ts | 2 +- .../suites/express/tracing/updateName/test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts index a957418852db..7d2d949898c2 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, type Event } from '@sentry/core'; +import { type Event, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts index 2113c26c549d..69094b38e4dd 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-updateSpanName/test.ts @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, type Event } from '@sentry/core'; +import { type Event, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, 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 e092f200265b..c6345713fd7e 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 @@ -1,6 +1,6 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/node'; import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; -import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from '@sentry/core'; describe('express tracing', () => { afterAll(() => { From f4a3e89f234552a48359de88c829329a2f9eaa88 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 11:47:38 +0100 Subject: [PATCH 16/18] fix wrong attribute used --- packages/opentelemetry/src/utils/parseSpanDescription.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 6e197366cd6e..1592591072d4 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -116,7 +116,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription { function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription { // if we already have a custom name, we don't overwrite it but only set the op - const userDefinedName = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; + const userDefinedName = attributes[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; if (typeof userDefinedName === 'string') { return { op: 'db', From f8b614c3462a7124a6154ec33ad7022ed22306c2 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 15:14:59 +0100 Subject: [PATCH 17/18] remove leftover deletion and logs --- packages/core/src/envelope.ts | 10 ---------- .../opentelemetry/src/utils/parseSpanDescription.ts | 6 +----- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index d8e43a37d40f..8c6a81533325 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -96,16 +96,6 @@ export function createEventEnvelope( // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid // of this `delete`, lest we miss putting it back in the next time the property is in use.) delete event.sdkProcessingMetadata; - try { - // @ts-expect-error - for bundle size we try/catch the access to this property - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; - // @ts-expect-error - for bundle size we try/catch the access to this property - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - event.spans.forEach(span => delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]); - } catch { - // Do nothing - } const eventItem: EventItem = [{ type: eventType }, event]; return createEnvelope(envelopeHeaders, [eventItem]); diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 1592591072d4..3136b94e6bf7 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -106,12 +106,8 @@ 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 }); - const res = inferSpanData(name, attributes, kind); - - // console.log('x parseSpanDesc res', res); - return res; + return inferSpanData(name, attributes, kind); } function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription { From 87a4d930177059271c61ae336705fff8b49b1e4e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 13 Dec 2024 16:10:48 +0100 Subject: [PATCH 18/18] biome :( --- packages/core/src/envelope.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 8c6a81533325..999fb0681cf0 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,4 +1,3 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME } from './semanticAttributes'; import { getDynamicSamplingContextFromSpan } from './tracing/dynamicSamplingContext'; import type { SentrySpan } from './tracing/sentrySpan'; import type {