diff --git a/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js b/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js new file mode 100644 index 000000000000..3b45591ec4df --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/withError/server.js @@ -0,0 +1,29 @@ +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 { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.get('/test/:id1/:id2', (_req, res) => { + Sentry.captureMessage(new Error('error_1')); + res.send('Success'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/withError/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/withError/test.ts new file mode 100644 index 000000000000..f164bdd0caab --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/withError/test.ts @@ -0,0 +1,28 @@ +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +describe('express tracing experimental', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + describe('CJS', () => { + test('should apply the scope transactionName to error events', done => { + createRunner(__dirname, 'server.js') + .ignore('session', 'sessions', 'transaction') + .expect({ + event: { + exception: { + values: [ + { + value: 'error_1', + }, + ], + }, + transaction: 'GET /test/:id1/:id2', + }, + }) + .start(done) + .makeRequest('get', '/test/123/abc?q=1'); + }); + }); +}); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index ed93ebacdaa5..c01b6dcf871b 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -4,10 +4,11 @@ import { SpanKind } from '@opentelemetry/api'; import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; -import { addBreadcrumb, defineIntegration, getIsolationScope, isSentryRequestUrl } from '@sentry/core'; +import { addBreadcrumb, defineIntegration, getIsolationScope, isSentryRequestUrl, spanToJSON } from '@sentry/core'; import { _INTERNAL, getClient, getSpanKind } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; +import { stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module'; @@ -81,19 +82,35 @@ const _httpIntegration = ((options: HttpOptions = {}) => { requireParentforOutgoingSpans: true, requireParentforIncomingSpans: false, requestHook: (span, req) => { - _updateSpan(span); + addOriginToSpan(span, 'auto.http.otel.http'); + + if (getSpanKind(span) !== SpanKind.SERVER) { + return; + } // Update the isolation scope, isolate this request - if (getSpanKind(span) === SpanKind.SERVER) { - const isolationScope = getIsolationScope().clone(); - isolationScope.setSDKProcessingMetadata({ request: req }); - - const client = getClient(); - if (client && client.getOptions().autoSessionTracking) { - isolationScope.setRequestSession({ status: 'ok' }); - } - setIsolationScope(isolationScope); + const isolationScope = getIsolationScope().clone(); + isolationScope.setSDKProcessingMetadata({ request: req }); + + const client = getClient(); + if (client && client.getOptions().autoSessionTracking) { + isolationScope.setRequestSession({ status: 'ok' }); } + setIsolationScope(isolationScope); + + // attempt to update the scope's `transactionName` based on the request URL + // Ideally, framework instrumentations coming after the HttpInstrumentation + // update the transactionName once we get a parameterized route. + const attributes = spanToJSON(span).data; + if (!attributes) { + return; + } + + const httpMethod = String(attributes['http.method']).toUpperCase() || 'GET'; + const httpTarget = stripUrlQueryAndFragment(String(attributes['http.target'])) || '/'; + const bestEffortTransactionName = `${httpMethod} ${httpTarget}`; + + isolationScope.setTransactionName(bestEffortTransactionName); }, responseHook: (span, res) => { if (_breadcrumbs) { @@ -123,11 +140,6 @@ const _httpIntegration = ((options: HttpOptions = {}) => { */ export const httpIntegration = defineIntegration(_httpIntegration); -/** Update the span with data we need. */ -function _updateSpan(span: Span): void { - addOriginToSpan(span, 'auto.http.otel.http'); -} - /** Add a breadcrumb for outgoing requests. */ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse): void { if (getSpanKind(span) !== SpanKind.CLIENT) { diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 12e44199e16d..57ea1cb75ac0 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -18,6 +18,15 @@ const _expressIntegration = (() => { requestHook(span) { addOriginToSpan(span, 'auto.http.otel.express'); }, + spanNameHook(info, defaultName) { + if (info.layerType === 'request_handler') { + // type cast b/c Otel unfortunately types info.request as any :( + const req = info.request as { method?: string }; + const method = req.method ? req.method.toUpperCase() : 'GET'; + getIsolationScope().setTransactionName(`${method} ${info.route}`); + } + return defaultName; + }, }), ], }); diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 807bc0541c2c..85133521ef76 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -298,7 +298,7 @@ export function addRequestDataToEvent( } } - if (include.transaction && !event.transaction) { + if (include.transaction && !event.transaction && event.type === 'transaction') { // TODO do we even need this anymore? // TODO make this work for nextjs event.transaction = extractTransaction(req, include.transaction); diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts index 3bd5ac507268..7e44f703c62a 100644 --- a/packages/utils/test/requestdata.test.ts +++ b/packages/utils/test/requestdata.test.ts @@ -149,52 +149,63 @@ describe('addRequestDataToEvent', () => { }); describe('transaction property', () => { - test('extracts method and full route path by default`', () => { - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + describe('for transaction events', () => { + beforeEach(() => { + mockEvent.type = 'transaction'; + }); - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); - }); + test('extracts method and full route path by default`', () => { + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - test('extracts method and full path by default when mountpoint is `/`', () => { - mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); - mockReq.baseUrl = ''; + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); + }); - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + test('extracts method and full path by default when mountpoint is `/`', () => { + mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); + mockReq.baseUrl = ''; - // `subpath/` is the full path here, because there's no router mount path - expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); - }); + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - test('fallback to method and `originalUrl` if route is missing', () => { - delete mockReq.route; + // `subpath/` is the full path here, because there's no router mount path + expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); + }); - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + test('fallback to method and `originalUrl` if route is missing', () => { + delete mockReq.route; - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); - }); + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - test('can extract path only instead if configured', () => { - const optionsWithPathTransaction = { - include: { - transaction: 'path', - }, - } as const; + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); + }); - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction); + test('can extract path only instead if configured', () => { + const optionsWithPathTransaction = { + include: { + transaction: 'path', + }, + } as const; - expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); - }); + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction); - test('can extract handler name instead if configured', () => { - const optionsWithHandlerTransaction = { - include: { - transaction: 'handler', - }, - } as const; + expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); + }); - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction); + test('can extract handler name instead if configured', () => { + const optionsWithHandlerTransaction = { + include: { + transaction: 'handler', + }, + } as const; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction); + + expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); + }); + }); + it('transaction is not applied to non-transaction events', () => { + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); + expect(parsedRequest.transaction).toBeUndefined(); }); }); });