From 5751c7cba7e1cae9c057ccd91d47a87574cb7525 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 17 May 2024 14:24:52 +0200 Subject: [PATCH 1/2] feat(node): Ensure express spans have better data --- .../node-express/package.json | 2 +- .../tests/{error.test.ts => errors.test.ts} | 0 .../node-express/tests/transaction.test.ts | 45 ----- .../node-express/tests/transactions.test.ts | 155 ++++++++++++++++++ .../suites/express/tracing/test.ts | 12 +- package.json | 3 +- .../node/src/integrations/tracing/express.ts | 22 ++- 7 files changed, 190 insertions(+), 49 deletions(-) rename dev-packages/e2e-tests/test-applications/node-express/tests/{error.test.ts => errors.test.ts} (100%) delete mode 100644 dev-packages/e2e-tests/test-applications/node-express/tests/transaction.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts diff --git a/dev-packages/e2e-tests/test-applications/node-express/package.json b/dev-packages/e2e-tests/test-applications/node-express/package.json index b3835693d99d..c59865a69266 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/package.json +++ b/dev-packages/e2e-tests/test-applications/node-express/package.json @@ -20,7 +20,7 @@ "@types/node": "18.15.1", "express": "4.19.2", "typescript": "4.9.5", - "zod": "^3.22.4" + "zod": "~3.22.4" }, "devDependencies": { "@sentry-internal/event-proxy-server": "link:../../../event-proxy-server", diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/error.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts similarity index 100% rename from dev-packages/e2e-tests/test-applications/node-express/tests/error.test.ts rename to dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/transaction.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/transaction.test.ts deleted file mode 100644 index 722418977671..000000000000 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/transaction.test.ts +++ /dev/null @@ -1,45 +0,0 @@ -import { expect, test } from '@playwright/test'; -import axios, { AxiosError } from 'axios'; - -const authToken = process.env.E2E_TEST_AUTH_TOKEN; -const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG; -const sentryTestProject = process.env.E2E_TEST_SENTRY_TEST_PROJECT; -const EVENT_POLLING_TIMEOUT = 90_000; - -test('Sends transactions to Sentry', async ({ baseURL }) => { - const { data } = await axios.get(`${baseURL}/test-transaction`); - const { transactionIds } = data; - - console.log(`Polling for transaction eventIds: ${JSON.stringify(transactionIds)}`); - - expect(transactionIds.length).toBeGreaterThan(0); - - await Promise.all( - transactionIds.map(async (transactionId: string) => { - const url = `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionId}/`; - - await expect - .poll( - async () => { - try { - const response = await axios.get(url, { headers: { Authorization: `Bearer ${authToken}` } }); - - return response.status; - } catch (e) { - if (e instanceof AxiosError && e.response) { - if (e.response.status !== 404) { - throw e; - } else { - return e.response.status; - } - } else { - throw e; - } - } - }, - { timeout: EVENT_POLLING_TIMEOUT }, - ) - .toBe(200); - }), - ); -}); diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts new file mode 100644 index 000000000000..fcb3913e1fce --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts @@ -0,0 +1,155 @@ +import { expect, test } from '@playwright/test'; +import { waitForTransaction } from '@sentry-internal/event-proxy-server'; +import axios, { AxiosError } from 'axios'; + +const authToken = process.env.E2E_TEST_AUTH_TOKEN; +const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG; +const sentryTestProject = process.env.E2E_TEST_SENTRY_TEST_PROJECT; +const EVENT_POLLING_TIMEOUT = 90_000; + +test('Sends an API route transaction', async ({ baseURL }) => { + const pageloadTransactionEventPromise = waitForTransaction('node-express', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-transaction' + ); + }); + + await axios.get(`${baseURL}/test-transaction`); + + const transactionEvent = await pageloadTransactionEventPromise; + const transactionEventId = transactionEvent.event_id; + + expect(transactionEvent.contexts?.trace).toEqual({ + data: { + 'sentry.source': 'route', + 'sentry.origin': 'auto.http.otel.http', + 'sentry.op': 'http.server', + 'sentry.sample_rate': 1, + url: 'http://localhost:3030/test-transaction', + 'otel.kind': 'SERVER', + 'http.response.status_code': 200, + 'http.url': 'http://localhost:3030/test-transaction', + 'http.host': 'localhost:3030', + 'net.host.name': 'localhost', + 'http.method': 'GET', + 'http.scheme': 'http', + 'http.target': '/test-transaction', + 'http.user_agent': 'axios/1.6.7', + 'http.flavor': '1.1', + 'net.transport': 'ip_tcp', + 'net.host.ip': expect.any(String), + 'net.host.port': expect.any(Number), + 'net.peer.ip': expect.any(String), + 'net.peer.port': expect.any(Number), + 'http.status_code': 200, + 'http.status_text': 'OK', + 'http.route': '/test-transaction', + }, + op: 'http.server', + span_id: expect.any(String), + status: 'ok', + trace_id: expect.any(String), + origin: 'auto.http.otel.http', + }); + + expect(transactionEvent).toEqual( + expect.objectContaining({ + transaction: 'GET /test-transaction', + type: 'transaction', + transaction_info: { + source: 'route', + }, + }), + ); + + const spans = transactionEvent.spans || []; + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', + 'http.route': '/', + 'express.name': 'query', + 'express.type': 'middleware', + 'otel.kind': 'INTERNAL', + }, + description: 'query', + op: 'middleware.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', + 'http.route': '/', + 'express.name': 'expressInit', + 'express.type': 'middleware', + 'otel.kind': 'INTERNAL', + }, + description: 'expressInit', + op: 'middleware.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); + + expect(spans).toContainEqual({ + data: { + 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'request_handler.express', + 'http.route': '/test-transaction', + 'express.name': '/test-transaction', + 'express.type': 'request_handler', + 'otel.kind': 'INTERNAL', + }, + description: '/test-transaction', + op: 'request_handler.express', + origin: 'auto.http.otel.express', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }); + + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionEventId}/`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + + return response.status; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toBe(200); +}); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts index 337a1166ee64..1c169291f235 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -29,7 +29,17 @@ describe('express tracing experimental', () => { 'express.name': 'corsMiddleware', 'express.type': 'middleware', }), - description: 'middleware - corsMiddleware', + description: 'corsMiddleware', + op: 'middleware.express', + origin: 'auto.http.otel.express', + }), + expect.objectContaining({ + data: expect.objectContaining({ + 'express.name': '/test/express', + 'express.type': 'request_handler', + }), + description: '/test/express', + op: 'request_handler.express', origin: 'auto.http.otel.express', }), ]), diff --git a/package.json b/package.json index 6abdde7f2de2..88d43da80039 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,8 @@ "build:watch": "lerna run build:watch", "build:dev:watch": "lerna run build:dev:watch", "build:types:watch": "ts-node scripts/build-types-watch.ts", - "build:tarball": "lerna run build:tarball", + "build:tarball": "run-s clean:tarballs build:tarballs", + "build:tarballs": "lerna run build:tarball", "circularDepCheck": "lerna run circularDepCheck", "clean": "run-s clean:build clean:caches", "clean:build": "lerna run clean", diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 31abf92aca58..8b8ea56ceddd 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -1,6 +1,12 @@ import type * as http from 'http'; import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express'; -import { defineIntegration, getDefaultIsolationScope, isEnabled } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + defineIntegration, + getDefaultIsolationScope, + isEnabled, + spanToJSON, +} from '@sentry/core'; import { captureException, getClient, getIsolationScope } from '@sentry/core'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; @@ -19,6 +25,20 @@ const _expressIntegration = (() => { new ExpressInstrumentation({ requestHook(span) { addOriginToSpan(span, 'auto.http.otel.express'); + + const attributes = spanToJSON(span).data || {}; + // this is one of: middleware, request_handler, router + const type = attributes['express.type']; + + if (type) { + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, `${type}.express`); + } + + // Also update the name, we don't need to "middleware - " prefix + const name = attributes['express.name']; + if (typeof name === 'string') { + span.updateName(name); + } }, spanNameHook(info, defaultName) { if (getIsolationScope() === getDefaultIsolationScope()) { From ae69acf6854273b73c133b101fc7d75ac724effb Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 17 May 2024 14:52:55 +0200 Subject: [PATCH 2/2] fix tests --- .../node-express-esm-loader/tests/server.test.ts | 12 +++++++++--- .../node-nestjs/tests/transactions.test.ts | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts index 3602a3c54623..410ff414f908 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts @@ -68,8 +68,10 @@ test('Should record a transaction for route with parameters', async ({ request } 'http.route': '/', 'otel.kind': 'INTERNAL', 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', }, - description: 'middleware - query', + op: 'middleware.express', + description: 'query', origin: 'auto.http.otel.express', parent_span_id: expect.any(String), span_id: expect.any(String), @@ -86,8 +88,10 @@ test('Should record a transaction for route with parameters', async ({ request } 'http.route': '/', 'otel.kind': 'INTERNAL', 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'middleware.express', }, - description: 'middleware - expressInit', + op: 'middleware.express', + description: 'expressInit', origin: 'auto.http.otel.express', parent_span_id: expect.any(String), span_id: expect.any(String), @@ -104,8 +108,10 @@ test('Should record a transaction for route with parameters', async ({ request } 'http.route': '/test-transaction/:param', 'otel.kind': 'INTERNAL', 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'request_handler.express', }, - description: 'request handler - /test-transaction/:param', + op: 'request_handler.express', + description: '/test-transaction/:param', origin: 'auto.http.otel.express', parent_span_id: expect.any(String), span_id: expect.any(String), diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs/tests/transactions.test.ts index 08a3998d0ecd..2739dbe20b07 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs/tests/transactions.test.ts @@ -63,8 +63,10 @@ test('Sends an API route transaction', async ({ baseURL }) => { 'http.route': '/test-transaction', 'otel.kind': 'INTERNAL', 'sentry.origin': 'auto.http.otel.express', + 'sentry.op': 'request_handler.express', }, - description: 'request handler - /test-transaction', + op: 'request_handler.express', + description: '/test-transaction', parent_span_id: expect.any(String), span_id: expect.any(String), start_timestamp: expect.any(Number),