From dbd4ed83195ccb33c1ce6fe84210c2469cb2a87d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 27 Aug 2024 23:00:25 +0100 Subject: [PATCH 01/15] feat(node): Use `@opentelemetry/instrumentation-undici` for fetch tracing --- .github/dependabot.yml | 1 - .../tracing/requests/fetch-no-tracing/test.ts | 2 +- .../fetch-sampled-no-active-span/scenario.ts | 2 - .../fetch-sampled-no-active-span/test.ts | 6 +- .../requests/fetch-unsampled/scenario.ts | 2 - .../tracing/requests/fetch-unsampled/test.ts | 2 +- packages/node/package.json | 6 +- packages/node/src/integrations/node-fetch.ts | 90 +++++-------------- yarn.lock | 45 +++------- 9 files changed, 40 insertions(+), 116 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 33d8c9ed27c0..4c4085da37b5 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -17,7 +17,6 @@ updates: - dependency-name: "@sentry/vite-plugin" - dependency-name: "@opentelemetry/*" - dependency-name: "@prisma/instrumentation" - - dependency-name: "opentelemetry-instrumentation-fetch-node" versioning-strategy: increase commit-message: prefix: feat diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts index 9c732d899cde..827acbd95028 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 18 })('outgoing fetch', () => { +conditionalTest({ min: 16 })('outgoing fetch', () => { test('outgoing fetch requests are correctly instrumented with tracing disabled', done => { expect.assertions(11); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts index 191797a10c15..9bbd9c9c1aeb 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts @@ -11,8 +11,6 @@ Sentry.init({ }); async function run(): Promise { - // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented - await new Promise(resolve => setTimeout(resolve, 100)); await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text()); await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text()); await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text()); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts index fde1c787829a..7a9bcdf60ae7 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts @@ -2,19 +2,19 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 18 })('outgoing fetch', () => { +conditionalTest({ min: 16 })('outgoing fetch', () => { test('outgoing sampled fetch requests without active span are correctly instrumented', done => { expect.assertions(11); createTestServer(done) .get('/api/v0', headers => { expect(headers['baggage']).toEqual(expect.any(String)); - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); }) .get('/api/v1', headers => { expect(headers['baggage']).toEqual(expect.any(String)); - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); }) .get('/api/v2', headers => { diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts index 91c38bf2b23a..9ab4c58967f2 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts @@ -13,8 +13,6 @@ Sentry.init({ async function run(): Promise { // Wrap in span that is not sampled await Sentry.startSpan({ name: 'outer' }, async () => { - // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented - await new Promise(resolve => setTimeout(resolve, 100)); await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text()); await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text()); await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text()); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts index d288e9a03fbf..e96395466a05 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 18 })('outgoing fetch', () => { +conditionalTest({ min: 16 })('outgoing fetch', () => { test('outgoing fetch requests are correctly instrumented when not sampled', done => { expect.assertions(11); diff --git a/packages/node/package.json b/packages/node/package.json index fa8fa1a797c7..dc1dd1811cd7 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -79,12 +79,13 @@ "@opentelemetry/instrumentation-ioredis": "0.42.0", "@opentelemetry/instrumentation-koa": "0.42.0", "@opentelemetry/instrumentation-mongodb": "0.46.0", - "@opentelemetry/instrumentation-mongoose": "0.40.0", + "@opentelemetry/instrumentation-mongoose": "0.41.0", "@opentelemetry/instrumentation-mysql": "0.40.0", "@opentelemetry/instrumentation-mysql2": "0.40.0", "@opentelemetry/instrumentation-nestjs-core": "0.39.0", "@opentelemetry/instrumentation-pg": "0.43.0", "@opentelemetry/instrumentation-redis-4": "0.41.0", + "@opentelemetry/instrumentation-undici": "0.5.0", "@opentelemetry/resources": "^1.25.1", "@opentelemetry/sdk-trace-base": "^1.25.1", "@opentelemetry/semantic-conventions": "^1.25.1", @@ -98,9 +99,6 @@ "devDependencies": { "@types/node": "^14.18.0" }, - "optionalDependencies": { - "opentelemetry-instrumentation-fetch-node": "1.2.3" - }, "scripts": { "build": "run-p build:transpile build:types", "build:dev": "yarn build", diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index 093b314a6138..cf65a4efa984 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -1,6 +1,7 @@ -import type { Span } from '@opentelemetry/api'; import { trace } from '@opentelemetry/api'; import { context, propagation } from '@opentelemetry/api'; +import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; +import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core'; import { addOpenTelemetryInstrumentation, @@ -8,25 +9,7 @@ import { getPropagationContextFromSpan, } from '@sentry/opentelemetry'; import type { IntegrationFn, SanitizedRequestData } from '@sentry/types'; -import { getSanitizedUrlString, logger, parseUrl } from '@sentry/utils'; -import { DEBUG_BUILD } from '../debug-build'; -import { NODE_MAJOR } from '../nodeVersion'; - -import type { FetchInstrumentation } from 'opentelemetry-instrumentation-fetch-node'; - -import { addOriginToSpan } from '../utils/addOriginToSpan'; - -interface FetchRequest { - method: string; - origin: string; - path: string; - headers: string | string[]; -} - -interface FetchResponse { - headers: Buffer[]; - statusCode: number; -} +import { getSanitizedUrlString, parseUrl } from '@sentry/utils'; interface NodeFetchOptions { /** @@ -46,30 +29,11 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { const _breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs; const _ignoreOutgoingRequests = options.ignoreOutgoingRequests; - async function getInstrumentation(): Promise { - // Only add NodeFetch if Node >= 18, as previous versions do not support it - if (NODE_MAJOR < 18) { - DEBUG_BUILD && logger.log('NodeFetch is not supported on Node < 18, skipping instrumentation...'); - return; - } - - try { - const pkg = await import('opentelemetry-instrumentation-fetch-node'); - const { FetchInstrumentation } = pkg; - - class SentryNodeFetchInstrumentation extends FetchInstrumentation { - // We extend this method so we have access to request _and_ response for the breadcrumb - public onHeaders({ request, response }: { request: FetchRequest; response: FetchResponse }): void { - if (_breadcrumbs) { - _addRequestBreadcrumb(request, response); - } - - return super.onHeaders({ request, response }); - } - } - - return new SentryNodeFetchInstrumentation({ - ignoreRequestHook: (request: FetchRequest) => { + return { + name: 'NodeFetch', + setupOnce() { + const instrumentation = new UndiciInstrumentation({ + ignoreRequestHook: request => { const url = getAbsoluteUrl(request.origin, request.path); const tracingDisabled = !hasTracingEnabled(); const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); @@ -113,39 +77,27 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { return false; }, - onRequest: ({ span }: { span: Span }) => { - _updateSpan(span); + startSpanHook: () => { + return { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'auto.http.otel.node_fetch', + }; + }, + responseHook: (_, { request, response }) => { + if (_breadcrumbs) { + addRequestBreadcrumb(request, response); + } }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - } catch (error) { - // Could not load instrumentation - DEBUG_BUILD && logger.log('Error while loading NodeFetch instrumentation: \n', error); - } - } - - return { - name: 'NodeFetch', - setupOnce() { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - getInstrumentation().then(instrumentation => { - if (instrumentation) { - addOpenTelemetryInstrumentation(instrumentation); - } }); + + addOpenTelemetryInstrumentation(instrumentation); }, }; }) satisfies IntegrationFn; export const nativeNodeFetchIntegration = defineIntegration(_nativeNodeFetchIntegration); -/** Update the span with data we need. */ -function _updateSpan(span: Span): void { - addOriginToSpan(span, 'auto.http.otel.node_fetch'); -} - /** Add a breadcrumb for outgoing requests. */ -function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse): void { +function addRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void { const data = getBreadcrumbData(request); addBreadcrumb( @@ -165,7 +117,7 @@ function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse): ); } -function getBreadcrumbData(request: FetchRequest): Partial { +function getBreadcrumbData(request: UndiciRequest): Partial { try { const url = new URL(request.path, request.origin); const parsedUrl = parseUrl(url.toString()); diff --git a/yarn.lock b/yarn.lock index a1a060119a64..f8dda041c5e0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7166,10 +7166,10 @@ "@opentelemetry/sdk-metrics" "^1.9.1" "@opentelemetry/semantic-conventions" "^1.22.0" -"@opentelemetry/instrumentation-mongoose@0.40.0": - version "0.40.0" - resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-mongoose/-/instrumentation-mongoose-0.40.0.tgz#9c888312e524c381bfdf56a094c799150332dd51" - integrity sha512-niRi5ZUnkgzRhIGMOozTyoZIvJKNJyhijQI4nF4iFSb+FUx2v5fngfR+8XLmdQAO7xmsD8E5vEGdDVYVtKbZew== +"@opentelemetry/instrumentation-mongoose@0.41.0": + version "0.41.0" + resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-mongoose/-/instrumentation-mongoose-0.41.0.tgz#9557245f7fb2a7f4673722a9c597bddb3e3be652" + integrity sha512-ivJg4QnnabFxxoI7K8D+in7hfikjte38sYzJB9v1641xJk9Esa7jM3hmbPB7lxwcgWJLVEDvfPwobt1if0tXxA== dependencies: "@opentelemetry/core" "^1.8.0" "@opentelemetry/instrumentation" "^0.52.0" @@ -7221,6 +7221,14 @@ "@opentelemetry/redis-common" "^0.36.2" "@opentelemetry/semantic-conventions" "^1.22.0" +"@opentelemetry/instrumentation-undici@0.5.0": + version "0.5.0" + resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-undici/-/instrumentation-undici-0.5.0.tgz#50782ff300027d0d0664fb60a3c12227586d5ebd" + integrity sha512-aNTeSrFAVcM9qco5DfZ9DNXu6hpMRe8Kt8nCDHfMWDB3pwgGVUE76jTdohc+H/7eLRqh4L7jqs5NSQoHw7S6ww== + dependencies: + "@opentelemetry/core" "^1.8.0" + "@opentelemetry/instrumentation" "^0.52.0" + "@opentelemetry/instrumentation@0.52.1", "@opentelemetry/instrumentation@^0.49 || ^0.50 || ^0.51 || ^0.52.0", "@opentelemetry/instrumentation@^0.52.0", "@opentelemetry/instrumentation@^0.52.1": version "0.52.1" resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation/-/instrumentation-0.52.1.tgz#2e7e46a38bd7afbf03cf688c862b0b43418b7f48" @@ -7244,17 +7252,6 @@ semver "^7.5.2" shimmer "^1.2.1" -"@opentelemetry/instrumentation@^0.46.0": - version "0.46.0" - resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation/-/instrumentation-0.46.0.tgz#a8a252306f82e2eace489312798592a14eb9830e" - integrity sha512-a9TijXZZbk0vI5TGLZl+0kxyFfrXHhX6Svtz7Pp2/VBlCSKrazuULEyoJQrOknJyFWNMEmbbJgOciHCCpQcisw== - dependencies: - "@types/shimmer" "^1.0.2" - import-in-the-middle "1.7.1" - require-in-the-middle "^7.1.1" - semver "^7.5.2" - shimmer "^1.2.1" - "@opentelemetry/otlp-transformer@^0.50.0": version "0.50.0" resolved "https://registry.yarnpkg.com/@opentelemetry/otlp-transformer/-/otlp-transformer-0.50.0.tgz#211fe512fcce9d76042680f955336dbde3be03ef" @@ -20766,16 +20763,6 @@ import-in-the-middle@1.4.2: cjs-module-lexer "^1.2.2" module-details-from-path "^1.0.3" -import-in-the-middle@1.7.1: - version "1.7.1" - resolved "https://registry.yarnpkg.com/import-in-the-middle/-/import-in-the-middle-1.7.1.tgz#3e111ff79c639d0bde459bd7ba29dd9fdf357364" - integrity sha512-1LrZPDtW+atAxH42S6288qyDFNQ2YCty+2mxEPRtfazH6Z5QwkaBSTS2ods7hnVJioF6rkRfNoA6A/MstpFXLg== - dependencies: - acorn "^8.8.2" - acorn-import-assertions "^1.9.0" - cjs-module-lexer "^1.2.2" - module-details-from-path "^1.0.3" - import-in-the-middle@^1.11.0, import-in-the-middle@^1.8.1: version "1.11.0" resolved "https://registry.yarnpkg.com/import-in-the-middle/-/import-in-the-middle-1.11.0.tgz#a94c4925b8da18256cde3b3b7b38253e6ca5e708" @@ -26304,14 +26291,6 @@ opener@^1.5.2: resolved "https://registry.yarnpkg.com/opener/-/opener-1.5.2.tgz#5d37e1f35077b9dcac4301372271afdeb2a13598" integrity sha512-ur5UIdyw5Y7yEj9wLzhqXiy6GZ3Mwx0yGI+5sMn2r0N0v3cKJvUmFH5yPP+WXh9e0xfyzyJX95D8l088DNFj7A== -opentelemetry-instrumentation-fetch-node@1.2.3: - version "1.2.3" - resolved "https://registry.yarnpkg.com/opentelemetry-instrumentation-fetch-node/-/opentelemetry-instrumentation-fetch-node-1.2.3.tgz#beb24048bdccb1943ba2a5bbadca68020e448ea7" - integrity sha512-Qb11T7KvoCevMaSeuamcLsAD+pZnavkhDnlVL0kRozfhl42dKG5Q3anUklAFKJZjY3twLR+BnRa6DlwwkIE/+A== - dependencies: - "@opentelemetry/instrumentation" "^0.46.0" - "@opentelemetry/semantic-conventions" "^1.17.0" - opentelemetry-instrumentation-remix@0.7.1: version "0.7.1" resolved "https://registry.yarnpkg.com/opentelemetry-instrumentation-remix/-/opentelemetry-instrumentation-remix-0.7.1.tgz#ef90ede718612786f7015e5496bd25cac8c49ce3" From 15b856c320a8c1dace70f36b28f44c3f45a670bb Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 2 Sep 2024 15:00:44 -0400 Subject: [PATCH 02/15] revert mongoose bump --- packages/node/package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 49d1e0644634..c213b57b4a68 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -79,7 +79,7 @@ "@opentelemetry/instrumentation-ioredis": "0.42.0", "@opentelemetry/instrumentation-koa": "0.42.0", "@opentelemetry/instrumentation-mongodb": "0.46.0", - "@opentelemetry/instrumentation-mongoose": "0.41.0", + "@opentelemetry/instrumentation-mongoose": "0.40.0", "@opentelemetry/instrumentation-mysql": "0.40.0", "@opentelemetry/instrumentation-mysql2": "0.40.0", "@opentelemetry/instrumentation-nestjs-core": "0.39.0", diff --git a/yarn.lock b/yarn.lock index e50d52e52c88..c8145b390637 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7166,10 +7166,10 @@ "@opentelemetry/sdk-metrics" "^1.9.1" "@opentelemetry/semantic-conventions" "^1.22.0" -"@opentelemetry/instrumentation-mongoose@0.41.0": - version "0.41.0" - resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-mongoose/-/instrumentation-mongoose-0.41.0.tgz#9557245f7fb2a7f4673722a9c597bddb3e3be652" - integrity sha512-ivJg4QnnabFxxoI7K8D+in7hfikjte38sYzJB9v1641xJk9Esa7jM3hmbPB7lxwcgWJLVEDvfPwobt1if0tXxA== +"@opentelemetry/instrumentation-mongoose@0.40.0": + version "0.40.0" + resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-mongoose/-/instrumentation-mongoose-0.40.0.tgz#9c888312e524c381bfdf56a094c799150332dd51" + integrity sha512-niRi5ZUnkgzRhIGMOozTyoZIvJKNJyhijQI4nF4iFSb+FUx2v5fngfR+8XLmdQAO7xmsD8E5vEGdDVYVtKbZew== dependencies: "@opentelemetry/core" "^1.8.0" "@opentelemetry/instrumentation" "^0.52.0" From f94e18325e624442b80cf1abbe057b96cea0f53a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 2 Sep 2024 15:08:44 -0400 Subject: [PATCH 03/15] Manually set `SEMATTRS_HTTP_URL` (`http.url`) since the instrumentation doesn't set it --- packages/node/src/integrations/node-fetch.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index cf65a4efa984..eccbbfe10d62 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -2,6 +2,7 @@ import { trace } from '@opentelemetry/api'; import { context, propagation } from '@opentelemetry/api'; import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; +import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions'; import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core'; import { addOpenTelemetryInstrumentation, @@ -77,8 +78,11 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { return false; }, - startSpanHook: () => { + startSpanHook: request => { + const url = getAbsoluteUrl(request.origin, request.path); + return { + [SEMATTRS_HTTP_URL]: url, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'auto.http.otel.node_fetch', }; }, From 7342e6144f3fc9a519ebecb9601c518f7c5f539b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 2 Sep 2024 15:11:16 -0400 Subject: [PATCH 04/15] add comments --- packages/node/src/integrations/node-fetch.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index eccbbfe10d62..20834f97796f 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -82,6 +82,8 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { const url = getAbsoluteUrl(request.origin, request.path); return { + // We manually set 'http.url' because the instrumentation doesn't set this and we use it when determining + // whether to propagate W3C baggage headers [SEMATTRS_HTTP_URL]: url, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'auto.http.otel.node_fetch', }; From 5d2d0f22ed408bda4c37f973d32e0d3b9f9f01e2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 2 Sep 2024 15:32:29 -0400 Subject: [PATCH 05/15] fix baggage propagation --- packages/node/src/integrations/node-fetch.ts | 7 +------ packages/opentelemetry/src/propagator.ts | 3 ++- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index 20834f97796f..da90fbb74ccf 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -78,13 +78,8 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { return false; }, - startSpanHook: request => { - const url = getAbsoluteUrl(request.origin, request.path); - + startSpanHook: () => { return { - // We manually set 'http.url' because the instrumentation doesn't set this and we use it when determining - // whether to propagate W3C baggage headers - [SEMATTRS_HTTP_URL]: url, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'auto.http.otel.node_fetch', }; }, diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 40b8a8139b0d..21404d5d8d02 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -292,7 +292,8 @@ function getExistingBaggage(carrier: unknown): string | undefined { * 2. Else, if the active span has no URL attribute (e.g. it is unsampled), we check a special trace state (which we set in our sampler). */ function getCurrentURL(span: Span): string | undefined { - const urlAttribute = spanToJSON(span).data?.[SEMATTRS_HTTP_URL]; + const spanData = spanToJSON(span).data; + const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.['url.full']; if (urlAttribute) { return urlAttribute; } From 4fa38f3888c9efdbff5fcc129d80e0883300bbe2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 3 Sep 2024 14:18:53 +0200 Subject: [PATCH 06/15] Lint --- packages/node/src/integrations/node-fetch.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index da90fbb74ccf..cf65a4efa984 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -2,7 +2,6 @@ import { trace } from '@opentelemetry/api'; import { context, propagation } from '@opentelemetry/api'; import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; -import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions'; import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core'; import { addOpenTelemetryInstrumentation, From 4368628879a8dedbbc42c1a9ef0214907aa390e3 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 3 Sep 2024 18:25:29 +0200 Subject: [PATCH 07/15] Check `url.full` --- packages/opentelemetry/src/sampler.ts | 2 +- packages/opentelemetry/src/utils/isSentryRequest.ts | 2 +- packages/opentelemetry/src/utils/parseSpanDescription.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 446c325f3ac7..25f4dace3509 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -196,7 +196,7 @@ function getBaseTraceState(context: Context, spanAttributes: SpanAttributes): Tr let traceState = parentContext?.traceState || new TraceState(); // We always keep the URL on the trace state, so we can access it in the propagator - const url = spanAttributes[SEMATTRS_HTTP_URL]; + const url = spanAttributes[SEMATTRS_HTTP_URL] || spanAttributes['url.full']; if (url && typeof url === 'string') { traceState = traceState.set(SENTRY_TRACE_STATE_URL, url); } diff --git a/packages/opentelemetry/src/utils/isSentryRequest.ts b/packages/opentelemetry/src/utils/isSentryRequest.ts index 3381b7833cea..28c11111aae4 100644 --- a/packages/opentelemetry/src/utils/isSentryRequest.ts +++ b/packages/opentelemetry/src/utils/isSentryRequest.ts @@ -16,7 +16,7 @@ export function isSentryRequestSpan(span: AbstractSpan): boolean { const { attributes } = span; - const httpUrl = attributes[SEMATTRS_HTTP_URL]; + const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes['url.full']; if (!httpUrl) { return false; diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index b600b81f8aec..3364bcb8c6eb 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -213,7 +213,7 @@ export function getSanitizedUrl( // This is the relative path of the URL, e.g. /sub const httpTarget = attributes[SEMATTRS_HTTP_TARGET]; // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar - const httpUrl = attributes[SEMATTRS_HTTP_URL]; + const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes['url.full']; // This is the normalized route name - may not always be available! const httpRoute = attributes[SEMATTRS_HTTP_ROUTE]; From 2e621b27620a74870f01c558740250bccdad52da Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 4 Sep 2024 09:42:09 +0200 Subject: [PATCH 08/15] Fix attribute --- packages/node/src/integrations/node-fetch.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index cf65a4efa984..e9cdf6aee7ed 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -2,7 +2,13 @@ import { trace } from '@opentelemetry/api'; import { context, propagation } from '@opentelemetry/api'; import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; -import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + addBreadcrumb, + defineIntegration, + getCurrentScope, + hasTracingEnabled, +} from '@sentry/core'; import { addOpenTelemetryInstrumentation, generateSpanContextForPropagationContext, @@ -79,7 +85,7 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { }, startSpanHook: () => { return { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'auto.http.otel.node_fetch', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.node_fetch', }; }, responseHook: (_, { request, response }) => { From df013500a9cfbcc38cb36a85999c6abbdb511589 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 11:42:57 +0200 Subject: [PATCH 09/15] Get tests passing! --- .../tracing/requests/fetch-sampled-no-active-span/test.ts | 4 ++-- packages/cloudflare/src/request.ts | 6 ++++-- packages/core/src/semanticAttributes.ts | 4 ++++ packages/node/src/integrations/node-fetch.ts | 4 ++-- packages/opentelemetry/src/propagator.ts | 3 ++- packages/opentelemetry/src/sampler.ts | 8 +++++--- packages/opentelemetry/src/utils/isSentryRequest.ts | 4 ++-- packages/opentelemetry/src/utils/parseSpanDescription.ts | 8 ++++++-- 8 files changed, 27 insertions(+), 14 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts index 7a9bcdf60ae7..a24fb783aacf 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts @@ -9,12 +9,12 @@ conditionalTest({ min: 16 })('outgoing fetch', () => { createTestServer(done) .get('/api/v0', headers => { expect(headers['baggage']).toEqual(expect.any(String)); - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); }) .get('/api/v1', headers => { expect(headers['baggage']).toEqual(expect.any(String)); - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); }) .get('/api/v2', headers => { diff --git a/packages/cloudflare/src/request.ts b/packages/cloudflare/src/request.ts index 7a474c3b27cb..f030cad8d02e 100644 --- a/packages/cloudflare/src/request.ts +++ b/packages/cloudflare/src/request.ts @@ -4,6 +4,8 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, + SEMANTIC_ATTRIBUTE_URL_FULL, captureException, continueTrace, flush, @@ -45,8 +47,8 @@ export function wrapRequestHandler( [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.cloudflare', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server', - ['http.request.method']: request.method, - ['url.full']: request.url, + [SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method, + [SEMANTIC_ATTRIBUTE_URL_FULL]: request.url, }; const contentLength = request.headers.get('content-length'); diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts index 2c268110854c..2d24c52a15ea 100644 --- a/packages/core/src/semanticAttributes.ts +++ b/packages/core/src/semanticAttributes.ts @@ -41,3 +41,7 @@ export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit'; export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key'; export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size'; + +/** TODO: Remove these once we update to latest semantic conventions */ +export const SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD = 'http.request.method'; +export const SEMANTIC_ATTRIBUTE_URL_FULL = 'url.full'; diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index 3f77d0d4fbc4..7e95f426365f 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -39,9 +39,9 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { name: 'NodeFetch', setupOnce() { const instrumentation = new UndiciInstrumentation({ + requireParentforSpans: false, ignoreRequestHook: request => { const url = getAbsoluteUrl(request.origin, request.path); - const tracingDisabled = !hasTracingEnabled(); const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); if (shouldIgnore) { @@ -50,7 +50,7 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { // If tracing is disabled, we still want to propagate traces // So we do that manually here, matching what the instrumentation does otherwise - if (tracingDisabled) { + if (!hasTracingEnabled()) { const ctx = context.active(); const addedHeaders: Record = {}; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 21404d5d8d02..4ed5a15532d2 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -5,6 +5,7 @@ import { propagation, trace } from '@opentelemetry/api'; import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions'; import type { continueTrace } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_URL_FULL } from '@sentry/core'; import { hasTracingEnabled } from '@sentry/core'; import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; @@ -293,7 +294,7 @@ function getExistingBaggage(carrier: unknown): string | undefined { */ function getCurrentURL(span: Span): string | undefined { const spanData = spanToJSON(span).data; - const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.['url.full']; + const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.[SEMANTIC_ATTRIBUTE_URL_FULL]; if (urlAttribute) { return urlAttribute; } diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 25f4dace3509..506a23e8771d 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -5,8 +5,10 @@ import { TraceState } from '@opentelemetry/core'; import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base'; import { SamplingDecision } from '@opentelemetry/sdk-trace-base'; import { + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, + SEMANTIC_ATTRIBUTE_URL_FULL, hasTracingEnabled, sampleSpan, } from '@sentry/core'; @@ -50,11 +52,11 @@ export class SentrySampler implements Sampler { return wrapSamplingDecision({ decision: undefined, context, spanAttributes }); } - // If we have a http.client span that has no local parent, we never want to sample it + // If we have a http.client or undici span that has no local parent, we never want to sample it // but we want to leave downstream sampling decisions up to the server if ( spanKind === SpanKind.CLIENT && - spanAttributes[SEMATTRS_HTTP_METHOD] && + (spanAttributes[SEMATTRS_HTTP_METHOD] || spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]) && (!parentSpan || parentContext?.isRemote) ) { return wrapSamplingDecision({ decision: undefined, context, spanAttributes }); @@ -196,7 +198,7 @@ function getBaseTraceState(context: Context, spanAttributes: SpanAttributes): Tr let traceState = parentContext?.traceState || new TraceState(); // We always keep the URL on the trace state, so we can access it in the propagator - const url = spanAttributes[SEMATTRS_HTTP_URL] || spanAttributes['url.full']; + const url = spanAttributes[SEMATTRS_HTTP_URL] || spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL]; if (url && typeof url === 'string') { traceState = traceState.set(SENTRY_TRACE_STATE_URL, url); } diff --git a/packages/opentelemetry/src/utils/isSentryRequest.ts b/packages/opentelemetry/src/utils/isSentryRequest.ts index 28c11111aae4..c8b11c9e680c 100644 --- a/packages/opentelemetry/src/utils/isSentryRequest.ts +++ b/packages/opentelemetry/src/utils/isSentryRequest.ts @@ -1,5 +1,5 @@ import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions'; -import { getClient, isSentryRequestUrl } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_URL_FULL, getClient, isSentryRequestUrl } from '@sentry/core'; import type { AbstractSpan } from '../types'; import { spanHasAttributes } from './spanTypes'; @@ -16,7 +16,7 @@ export function isSentryRequestSpan(span: AbstractSpan): boolean { const { attributes } = span; - const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes['url.full']; + const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL]; if (!httpUrl) { return false; diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 3364bcb8c6eb..fb38f87fbfc8 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -14,7 +14,11 @@ import { import type { SpanAttributes, TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_URL_FULL, +} from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes'; import type { AbstractSpan } from '../types'; import { getSpanKind } from './getSpanKind'; @@ -213,7 +217,7 @@ export function getSanitizedUrl( // This is the relative path of the URL, e.g. /sub const httpTarget = attributes[SEMATTRS_HTTP_TARGET]; // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar - const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes['url.full']; + const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL]; // This is the normalized route name - may not always be available! const httpRoute = attributes[SEMATTRS_HTTP_ROUTE]; From 033a154e9281b481e4a00157353ba59540e3839d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 11:54:13 +0200 Subject: [PATCH 10/15] lint --- packages/cloudflare/src/request.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cloudflare/src/request.ts b/packages/cloudflare/src/request.ts index f030cad8d02e..1c51a08c194c 100644 --- a/packages/cloudflare/src/request.ts +++ b/packages/cloudflare/src/request.ts @@ -1,10 +1,10 @@ import type { ExecutionContext, IncomingRequestCfProperties } from '@cloudflare/workers-types'; import { + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, SEMANTIC_ATTRIBUTE_URL_FULL, captureException, continueTrace, From 75455130dd71f573272ca73d3a0e53eec8a59512 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 12:14:35 +0200 Subject: [PATCH 11/15] Dont test Node v16 --- .../suites/tracing/requests/fetch-no-tracing/test.ts | 2 +- .../tracing/requests/fetch-sampled-no-active-span/test.ts | 2 +- .../suites/tracing/requests/fetch-unsampled/test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts index 827acbd95028..9c732d899cde 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 16 })('outgoing fetch', () => { +conditionalTest({ min: 18 })('outgoing fetch', () => { test('outgoing fetch requests are correctly instrumented with tracing disabled', done => { expect.assertions(11); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts index a24fb783aacf..fde1c787829a 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 16 })('outgoing fetch', () => { +conditionalTest({ min: 18 })('outgoing fetch', () => { test('outgoing sampled fetch requests without active span are correctly instrumented', done => { expect.assertions(11); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts index e96395466a05..d288e9a03fbf 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils'; import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -conditionalTest({ min: 16 })('outgoing fetch', () => { +conditionalTest({ min: 18 })('outgoing fetch', () => { test('outgoing fetch requests are correctly instrumented when not sampled', done => { expect.assertions(11); From 88728a15fb27242172d818fe247b6e2f8cf65306 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 12:17:04 +0200 Subject: [PATCH 12/15] more `http.request.method` --- packages/bun/src/integrations/bunserver.ts | 3 ++- packages/opentelemetry/src/utils/parseSpanDescription.ts | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts index 93a3f94dd4a0..193ae6f286ca 100644 --- a/packages/bun/src/integrations/bunserver.ts +++ b/packages/bun/src/integrations/bunserver.ts @@ -1,4 +1,5 @@ import { + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, @@ -65,7 +66,7 @@ function instrumentBunServeOptions(serveOptions: Parameters[0] const parsedUrl = parseUrl(request.url); const attributes: SpanAttributes = { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.bun.serve', - 'http.request.method': request.method || 'GET', + [SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method || 'GET', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', }; if (parsedUrl.search) { diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index fb38f87fbfc8..07354313f331 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -15,6 +15,7 @@ import type { SpanAttributes, TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; import { + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_URL_FULL, @@ -49,10 +50,7 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp } // if http.method exists, this is an http request span - // - // TODO: Referencing `http.request.method` is a temporary workaround until the semantic - // conventions export an attribute key for it. - const httpMethod = attributes['http.request.method'] || attributes[SEMATTRS_HTTP_METHOD]; + const httpMethod = attributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD]; if (httpMethod) { return descriptionForHttpMethod({ attributes, name, kind }, httpMethod); } From 0cb33222f9f45c53807b722399a48d3d77311b5d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 12:56:22 +0200 Subject: [PATCH 13/15] Fix nextjs-14 e2e test --- .../nextjs-14/tests/request-instrumentation.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts index 061c9d3cc5d6..8d8555d8cb11 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts @@ -15,7 +15,7 @@ test('Should send a transaction with a fetch span', async ({ page }) => { expect(transactionEvent.spans).toContainEqual( expect.objectContaining({ data: expect.objectContaining({ - 'http.method': 'GET', + 'http.request.method': 'GET', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.otel.node_fetch', }), @@ -26,7 +26,7 @@ test('Should send a transaction with a fetch span', async ({ page }) => { expect(transactionEvent.spans).toContainEqual( expect.objectContaining({ data: expect.objectContaining({ - 'http.method': 'GET', + 'http.request.method': 'GET', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.otel.http', }), From 4a73857cb166fb8401946a5e0348e75eb136b5eb Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 13:08:28 +0200 Subject: [PATCH 14/15] From PR review --- packages/node/src/integrations/node-fetch.ts | 55 ++------------------ packages/opentelemetry/src/sampler.ts | 2 +- 2 files changed, 4 insertions(+), 53 deletions(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index 7e95f426365f..0726c2c63f9b 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -1,19 +1,7 @@ -import { trace } from '@opentelemetry/api'; -import { context, propagation } from '@opentelemetry/api'; import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; -import { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - addBreadcrumb, - defineIntegration, - getCurrentScope, - hasTracingEnabled, -} from '@sentry/core'; -import { - addOpenTelemetryInstrumentation, - generateSpanContextForPropagationContext, - getPropagationContextFromSpan, -} from '@sentry/opentelemetry'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, addBreadcrumb, defineIntegration } from '@sentry/core'; +import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; import type { IntegrationFn, SanitizedRequestData } from '@sentry/types'; import { getSanitizedUrlString, parseUrl } from '@sentry/utils'; @@ -44,44 +32,7 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { const url = getAbsoluteUrl(request.origin, request.path); const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); - if (shouldIgnore) { - return true; - } - - // If tracing is disabled, we still want to propagate traces - // So we do that manually here, matching what the instrumentation does otherwise - if (!hasTracingEnabled()) { - const ctx = context.active(); - const addedHeaders: Record = {}; - - // We generate a virtual span context from the active one, - // Where we attach the URL to the trace state, so the propagator can pick it up - const activeSpan = trace.getSpan(ctx); - const propagationContext = activeSpan - ? getPropagationContextFromSpan(activeSpan) - : getCurrentScope().getPropagationContext(); - - const spanContext = generateSpanContextForPropagationContext(propagationContext); - // We know that in practice we'll _always_ haven a traceState here - spanContext.traceState = spanContext.traceState?.set('sentry.url', url); - const ctxWithUrlTraceState = trace.setSpanContext(ctx, spanContext); - - propagation.inject(ctxWithUrlTraceState, addedHeaders); - - const requestHeaders = request.headers; - if (Array.isArray(requestHeaders)) { - Object.entries(addedHeaders).forEach(headers => requestHeaders.push(...headers)); - } else { - request.headers += Object.entries(addedHeaders) - .map(([k, v]) => `${k}: ${v}\r\n`) - .join(''); - } - - // Prevent starting a span for this request - return true; - } - - return false; + return !!shouldIgnore; }, startSpanHook: () => { return { diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 506a23e8771d..03d47989ae1d 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -52,7 +52,7 @@ export class SentrySampler implements Sampler { return wrapSamplingDecision({ decision: undefined, context, spanAttributes }); } - // If we have a http.client or undici span that has no local parent, we never want to sample it + // If we have a http.client span that has no local parent, we never want to sample it // but we want to leave downstream sampling decisions up to the server if ( spanKind === SpanKind.CLIENT && From e8e2198b6cdf68cc691224bbbee43cfb2ddca894 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 5 Sep 2024 13:28:56 +0200 Subject: [PATCH 15/15] Fix nextjs-14 test --- .../nextjs-14/tests/request-instrumentation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts index 8d8555d8cb11..65b9c4312d91 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts @@ -26,7 +26,7 @@ test('Should send a transaction with a fetch span', async ({ page }) => { expect(transactionEvent.spans).toContainEqual( expect.objectContaining({ data: expect.objectContaining({ - 'http.request.method': 'GET', + 'http.method': 'GET', 'sentry.op': 'http.client', 'sentry.origin': 'auto.http.otel.http', }),