diff --git a/MIGRATION.md b/MIGRATION.md index 9140be4fe8ee..a36d03ab747f 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -1001,6 +1001,7 @@ The following span names were adjusted: | `http.client`, `http.client.stream` | The request method and sanitized URL | `GET https://api.example.com/users/123` | The request method and the domain, or just the method if there is no domain | `GET api.example.com`, `GET` | | `router` | Framework-specific, sometimes containing the raw URL | `/users/123`, `SvelteKit Route Change` | The span's `http.route`, or `Router` if the SDK has none | `/users/:id`, `Router` | | `handler` | Framework-specific, often carrying the request method | `GET /users/:id`, `route-handler`, `getUser` | The span's `http.route`, or `Request handler` if the SDK has none | `/users/:id`, `Request handler` | +| `function` | Integration-specific, sometimes the segment span's name | `serverAction/updateUser`, `LOADER routes/users.$id` | The span's `code.function.name`. The previous name is kept as the span description | `updateUser`, `loader` | | `function.gcp` | The request method and path for HTTP functions, otherwise the trigger's event or trigger type | `POST /users`, `google.pubsub.topic.publish`, `firebase.function.http.request` | The function name, or `Serverless function execution` if the SDK cannot resolve one | `myFunction`, `Serverless function execution` | | `function.aws` | The Lambda function name | `my-function` | Unchanged, except that the SDK now falls back to `Serverless function execution` if it cannot resolve the function name | `my-function`, `Serverless function execution` | | `graphql` | The graphql phase and, for operations, the operation name | `query GetUser`, `graphql.parse`, `graphql.resolve user.0.name` | The operation type, or the processing type where there is none | `GraphQL query`, `GraphQL parse`, `GraphQL resolve` | diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/server-transactions.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/server-transactions.test.ts index a4ffb5c41ee4..2f173401ea0d 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/server-transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/server-transactions.test.ts @@ -50,6 +50,7 @@ test('Sends form data with the action span', async ({ page }) => { expect(actionSpan).toBeDefined(); expect(getSpanOp(actionSpan!)).toBe('function'); + expect(actionSpan!.name).toBe('action'); expect(actionSpan!.attributes).toMatchObject({ 'remix.action_form_data.text': { value: 'test', type: 'string' }, 'remix.action_form_data.file': { value: 'file.txt', type: 'string' }, @@ -71,6 +72,32 @@ test('Sends a loader span to Sentry', async ({ page }) => { expect(loaderSpan).toBeDefined(); expect(getSpanOp(loaderSpan!)).toBe('function'); + expect(loaderSpan!.name).toBe('loader'); + // The route id left the span name, so it has to stay reachable as an attribute. + expect(loaderSpan!.attributes['match.route.id']?.value).toEqual(expect.any(String)); +}); + +test('Sends a low cardinality documentRequest span to Sentry', async ({ page }) => { + // Other tests hit `/user/:id` too, and `collectStreamedSpans` evaluates one trace at a time, so a + // unique path is what keeps this assertion on the request under test. + const path = `/user/${crypto.randomUUID()}`; + const spansPromise = collectStreamedSpansUntilSegment( + APP_NAME, + span => getSpanOp(span) === 'http.server' && span.attributes['url.path']?.value === path, + ); + + await page.goto(path); + + const spans = await spansPromise; + const segment = spans.find(span => span.is_segment)!; + const documentRequestSpan = spans.find(span => span.attributes['code.function.name']?.value === 'documentRequest'); + + expect(documentRequestSpan).toBeDefined(); + expect(documentRequestSpan!.name).toBe('documentRequest'); + expect(getSpanOp(documentRequestSpan!)).toBe('function'); + expect(documentRequestSpan!.attributes['sentry.origin']?.value).toBe('auto.function.remix'); + // The span name is low cardinality now, so the description carries the name it used to have. + expect(documentRequestSpan!.attributes['sentry.description']?.value).toBe(segment.name); }); test('Propagates the trace when the ErrorBoundary is triggered', async ({ page }) => { diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2-static/tests/server-transactions.test.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2-static/tests/server-transactions.test.ts index 97d1931f5455..c6b4f5ef0b94 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2-static/tests/server-transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2-static/tests/server-transactions.test.ts @@ -56,3 +56,17 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page expect(pageLoadParentSpanId).toEqual(loaderSpanId); expect(pageLoadSpanId).not.toEqual(httpServerSpanId); }); + +test('Keeps the documentRequest span name unchanged with static spans', async ({ page }) => { + const transactionPromise = waitForTransaction('create-remix-app-v2-static', transactionEvent => { + return transactionEvent.contexts?.trace?.op === 'http.server' && transactionEvent.transaction === 'GET user/:id'; + }); + + await page.goto('/user/123'); + + const transaction = await transactionPromise; + const documentRequestSpan = transaction.spans?.find(span => span.data?.['code.function.name'] === 'documentRequest'); + + expect(documentRequestSpan).toBeDefined(); + expect(documentRequestSpan?.description).toBe('GET user/:id'); +}); diff --git a/packages/remix/src/server/instrumentServer.ts b/packages/remix/src/server/instrumentServer.ts index a336a7f72c2a..4f7a7c76b447 100644 --- a/packages/remix/src/server/instrumentServer.ts +++ b/packages/remix/src/server/instrumentServer.ts @@ -41,8 +41,9 @@ import { extractData, isResponse, json } from '../utils/vendor/response'; import { captureRemixServerException, errorHandleDataFunction } from './errors'; import { generateSentryServerTimingHeader, injectServerTimingHeaderValue } from './serverTimingTracePropagation'; import { - SENTRY_SEGMENT_NAME_SOURCE, CODE_FUNCTION_NAME, + SENTRY_DESCRIPTION, + SENTRY_SEGMENT_NAME_SOURCE, HTTP_ROUTE, SENTRY_OP, URL_FULL, @@ -131,19 +132,22 @@ function makeWrappedDocumentRequestFunction(instrumentTracing?: boolean) { if (instrumentTracing) { const activeSpan = getActiveSpan(); const rootSpan = activeSpan && getRootSpan(activeSpan); - const name = rootSpan ? spanToJSON(rootSpan).name : undefined; + const client = getClient(); + + const description = (rootSpan ? spanToJSON(rootSpan).name : undefined) || ''; + const name = client && hasSpanStreamingEnabled(client) ? 'documentRequest' : description; response = await startSpan( { - // If we don't have a root span, `onlyIfParent` will lead to the span not being created anyhow - // So we don't need to care too much about the fallback name, it's just for typing purposes.... - name: name || '', + name, onlyIfParent: true, attributes: { method: request.method, [URL_FULL]: filterCollectedUrl(request.url), [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.remix', [SENTRY_OP]: FUNCTION, + [SENTRY_DESCRIPTION]: description, + [CODE_FUNCTION_NAME]: 'documentRequest', }, }, () => { @@ -218,14 +222,21 @@ function makeWrappedDataFunction( updateSpanWithRoute(args, build); } + const client = getClient(); + res = await startSpan( { - name: id, + // With span streaming, a `function` span is named after the function it wraps. The route + // module id stays on `router.navigation.route.id`. + name: client && hasSpanStreamingEnabled(client) ? name : id, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.remix', [SENTRY_OP]: FUNCTION, + [SENTRY_DESCRIPTION]: id, [CODE_FUNCTION_NAME]: name, - name, + // TODO: use conventions constant once `router.navigation.route.id` is in + // `@sentry/conventions`. + 'router.navigation.route.id': id, }, }, (span: Span) => { diff --git a/packages/remix/src/server/integrations/tracing-channel.ts b/packages/remix/src/server/integrations/tracing-channel.ts index f55ee00568ad..9efa218828ec 100644 --- a/packages/remix/src/server/integrations/tracing-channel.ts +++ b/packages/remix/src/server/integrations/tracing-channel.ts @@ -20,6 +20,7 @@ import { SENTRY_SEGMENT_NAME_SOURCE, CODE_FUNCTION_NAME, HTTP_ROUTE, + SENTRY_DESCRIPTION, URL_FULL, URL_PATH, SENTRY_KIND, @@ -191,11 +192,16 @@ function subscribeCallRouteLoader(): void { diagnosticsChannel.tracingChannel(remixChannels.REMIX_CALL_ROUTE_LOADER), data => { const params = (data.arguments[0] ?? {}) as RouteCallParams; + const client = getClient(); + const description = `LOADER ${params.routeId}`; return startInactiveSpan({ - name: `LOADER ${params.routeId}`, + // With span streaming, a `function` span is named after the function it wraps. The route id + // stays on `match.route.id`. + name: client && hasSpanStreamingEnabled(client) ? 'loader' : description, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, [SENTRY_OP]: FUNCTION, + [SENTRY_DESCRIPTION]: description, [CODE_FUNCTION_NAME]: 'loader', ...getRequestAttributes(params.request), ...getMatchAttributes(params), @@ -225,11 +231,16 @@ function subscribeCallRouteAction(formDataCapture: FormDataCapture | undefined): formData.catch(() => undefined); data._sentryFormData = formData; } + const client = getClient(); + const description = `ACTION ${params.routeId}`; return startInactiveSpan({ - name: `ACTION ${params.routeId}`, + // With span streaming, a `function` span is named after the function it wraps. The route id + // stays on `match.route.id`. + name: client && hasSpanStreamingEnabled(client) ? 'action' : description, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, [SENTRY_OP]: FUNCTION, + [SENTRY_DESCRIPTION]: description, [CODE_FUNCTION_NAME]: 'action', ...getRequestAttributes(params.request), ...getMatchAttributes(params), diff --git a/packages/remix/test/server/instrumentServer.test.ts b/packages/remix/test/server/instrumentServer.test.ts index 6450daf60d25..2fc313cc0b6b 100644 --- a/packages/remix/test/server/instrumentServer.test.ts +++ b/packages/remix/test/server/instrumentServer.test.ts @@ -1,5 +1,6 @@ import type { LoaderFunctionArgs, ServerBuild } from '@remix-run/server-runtime'; -import { HTTP_ROUTE } from '@sentry/conventions/attributes'; +import { CODE_FUNCTION_NAME, HTTP_ROUTE, SENTRY_DESCRIPTION, SENTRY_OP } from '@sentry/conventions/attributes'; +import { FUNCTION } from '@sentry/conventions/op'; import type { Span } from '@sentry/core'; import * as SentryCore from '@sentry/core'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -43,4 +44,97 @@ describe('instrumentBuild', () => { expect(rootSpan.setAttribute).toHaveBeenCalledWith(HTTP_ROUTE, '/users/:id'); }); + + describe('documentRequest span', () => { + function instrumentDocumentRequest(): (request: Request) => Promise { + const rootSpan = { setAttribute: vi.fn(), updateName: vi.fn() } as unknown as Span; + vi.spyOn(SentryCore, 'getActiveSpan').mockReturnValue(rootSpan); + vi.spyOn(SentryCore, 'getRootSpan').mockReturnValue(rootSpan); + vi.spyOn(SentryCore, 'spanToJSON').mockReturnValue({ name: 'GET /users/:id' }); + + const build = { + entry: { module: { default: vi.fn(async () => new Response('ok')) } }, + routes: {}, + } as unknown as ServerBuild; + + return instrumentBuild(build, { instrumentTracing: true }).entry.module.default as never; + } + + it.each([ + { lifecycle: 'streaming', streamed: true, expectedName: 'documentRequest' }, + { lifecycle: 'static', streamed: false, expectedName: 'GET /users/:id' }, + ])('names the span $expectedName with $lifecycle spans', async ({ streamed, expectedName }) => { + vi.spyOn(SentryCore, 'getClient').mockReturnValue({} as never); + vi.spyOn(SentryCore, 'hasSpanStreamingEnabled').mockReturnValue(streamed); + const startSpan = vi + .spyOn(SentryCore, 'startSpan') + .mockImplementation((_options, callback) => callback(undefined as unknown as Span)); + + await instrumentDocumentRequest()(new Request('https://example.com/users/42')); + + expect(startSpan).toHaveBeenCalledWith( + expect.objectContaining({ + name: expectedName, + attributes: expect.objectContaining({ + [SENTRY_OP]: FUNCTION, + // The description keeps the name the span had before it went low cardinality. + [SENTRY_DESCRIPTION]: 'GET /users/:id', + [CODE_FUNCTION_NAME]: 'documentRequest', + }), + }), + expect.any(Function), + ); + }); + }); + + describe('loader and action spans', () => { + function instrumentRouteLoader(): (args: LoaderFunctionArgs) => Promise { + const build = { + entry: { module: {} }, + routes: { + root: { id: 'root', module: {} }, + 'routes/users.$id': { + id: 'routes/users.$id', + parentId: 'root', + path: 'users/:id', + module: { loader: vi.fn(async () => ({})) }, + }, + }, + } as unknown as ServerBuild; + + return instrumentBuild(build, { instrumentTracing: true }).routes['routes/users.$id']!.module.loader as never; + } + + it.each([ + { lifecycle: 'streaming', streamed: true, expectedName: 'loader' }, + { lifecycle: 'static', streamed: false, expectedName: 'routes/users.$id' }, + ])('names the span $expectedName with $lifecycle spans', async ({ streamed, expectedName }) => { + vi.spyOn(SentryCore, 'getClient').mockReturnValue({} as never); + vi.spyOn(SentryCore, 'hasSpanStreamingEnabled').mockReturnValue(streamed); + const startSpan = vi + .spyOn(SentryCore, 'startSpan') + .mockImplementation((_options, callback) => callback(undefined as unknown as Span)); + + await instrumentRouteLoader()({ + context: {}, + params: { id: '42' }, + request: new Request('https://example.com/users/42'), + } as LoaderFunctionArgs); + + expect(startSpan).toHaveBeenCalledWith( + expect.objectContaining({ + name: expectedName, + attributes: expect.objectContaining({ + [SENTRY_OP]: FUNCTION, + // The description keeps the name the span had before it went low cardinality. + [SENTRY_DESCRIPTION]: 'routes/users.$id', + [CODE_FUNCTION_NAME]: 'loader', + // The route id left the span name, so it has to stay reachable as an attribute. + 'router.navigation.route.id': 'routes/users.$id', + }), + }), + expect.any(Function), + ); + }); + }); }); diff --git a/packages/remix/test/server/tracing-channel.test.ts b/packages/remix/test/server/tracing-channel.test.ts index 22b9e226b52d..1feccdb1ee51 100644 --- a/packages/remix/test/server/tracing-channel.test.ts +++ b/packages/remix/test/server/tracing-channel.test.ts @@ -113,6 +113,7 @@ describe('remixIntegration (Orchestrion-based)', () => { expect.objectContaining({ name: 'LOADER routes/users.$userId', attributes: expect.objectContaining({ + 'sentry.description': 'LOADER routes/users.$userId', 'sentry.origin': 'auto.http.remix', 'sentry.op': 'function', 'code.function.name': 'loader', @@ -153,6 +154,7 @@ describe('remixIntegration (Orchestrion-based)', () => { expect.objectContaining({ name: 'ACTION routes/submit', attributes: expect.objectContaining({ + 'sentry.description': 'ACTION routes/submit', 'sentry.op': 'function', 'code.function.name': 'action', 'http.request.method': 'POST', @@ -164,4 +166,77 @@ describe('remixIntegration (Orchestrion-based)', () => { expect(span.setAttribute).toHaveBeenCalledWith('http.response.status_code', 201); expect(span.setAttribute).toHaveBeenCalledWith('remix.action_form_data.actionType', 'create'); }); + + describe('with span streaming', () => { + // The shared harness mocks `@sentry/node`'s `getClient`, but the instrumentation reads + // `@sentry/core`'s, so the lifecycle has to be stubbed here to reach the streamed branch. + let clientSpy: MockInstance; + let streamingSpy: MockInstance; + + beforeEach(() => { + clientSpy = vi.spyOn(SentryCore, 'getClient').mockReturnValue({ + getOptions: () => ({}), + getDataCollectionOptions: () => ({ httpBodies: [] }), + } as never); + streamingSpy = vi.spyOn(SentryCore, 'hasSpanStreamingEnabled').mockReturnValue(true); + }); + + afterEach(() => { + clientSpy.mockRestore(); + streamingSpy.mockRestore(); + }); + + it('callRouteLoader: names the span after the function and keeps the route id on an attribute', async () => { + const ctx = { + arguments: [ + { + routeId: 'routes/users.$userId', + request: makeRequest({ method: 'GET', url: 'http://localhost/users/123' }), + params: { userId: '123' }, + }, + ], + }; + + await tracingChannel(remixChannels.REMIX_CALL_ROUTE_LOADER).tracePromise(async () => ({ status: 200 }), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'loader', + attributes: expect.objectContaining({ + 'sentry.op': 'function', + 'sentry.description': 'LOADER routes/users.$userId', + 'code.function.name': 'loader', + 'match.route.id': 'routes/users.$userId', + 'match.params.userId': '123', + }), + }), + ); + }); + + it('callRouteAction: names the span after the function and keeps the route id on an attribute', async () => { + const ctx = { + arguments: [ + { + routeId: 'routes/submit', + request: makeRequest({ method: 'POST', url: 'http://localhost/submit' }), + params: {}, + }, + ], + }; + + await tracingChannel(remixChannels.REMIX_CALL_ROUTE_ACTION).tracePromise(async () => ({ status: 201 }), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'action', + attributes: expect.objectContaining({ + 'sentry.op': 'function', + 'sentry.description': 'ACTION routes/submit', + 'code.function.name': 'action', + 'match.route.id': 'routes/submit', + }), + }), + ); + }); + }); });