Uh oh!
There was an error while loading. Please reload this page.
feat(node): Avoid OTEL instrumentation for outgoing requests on Node 22+ - #17355
Conversation
size-limit report 📦
|
67942ee to
b8b33f4Comparea66e597 to
002fb1eComparenode-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
dcb74c7 to
4023787CompareRegisters diagnostics channels for outgoing requests on Node >= 22 that takes care of creating spans, rather than relying on OTEL instrumentation.
4023787 to
1dad574CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| * This is a feature flag that should be enabled by SDKs when the runtime supports it (Node 22+). | ||
| * Individual users should not need to configure this directly. |
There was a problem hiding this comment.
This comment sound very directed to us as SDK maintainers. As this comment is public-facing I would write that a bit differently as it can be confusing how to act on this as a user.
There was a problem hiding this comment.
Do you have a suggestion? The comment already calls out individual users should not set this.
| * | ||
| * @default `true` | ||
| */ | ||
| spans?: boolean; |
There was a problem hiding this comment.
Q: I'm wondering why we need this second option...would someone ever want to set this to false?
There was a problem hiding this comment.
Users with custom OTel setups that add @opentelemetry/instrumentation-http will want to set this, see: https://docs.sentry.io/platforms/javascript/guides/node/opentelemetry/custom-setup/#custom-http-instrumentation
| // In this case, `http.client.response.finish` is not triggered | ||
| subscribe('http.client.request.error', onHttpClientRequestError); | ||
| if (this.getConfig().createSpansForOutgoingRequests) { |
There was a problem hiding this comment.
With the span option, we should also check for this.getConfig().spans (if I understood that correctly).
There was a problem hiding this comment.
This is checked in the handler itself but I agree, we can check this earlier. Currently it can be misleading to set spans: false and then still get the Handling started outgoing request log.
Codecov Results 📊✅ 27 passed | Total: 27 | Pass Rate: 100% | Execution Time: 11.46s All tests are passing successfully. Generated by Codecov Action |
Codecov Results 📊✅ 23 passed | ⏭️ 7 skipped | Total: 30 | Pass Rate: 76.67% | Execution Time: 10.18s All tests are passing successfully. Generated by Codecov Action |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| breadcrumbs: options.breadcrumbs, | ||
| propagateTraceInOutgoingRequests: !useOtelHttpInstrumentation, | ||
| propagateTraceInOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL || !useOtelHttpInstrumentation, | ||
| createSpansForOutgoingRequests: FULLY_SUPPORTS_HTTP_DIAGNOSTICS_CHANNEL, |
There was a problem hiding this comment.
Missing integration test for new diagnostics channel spans
Low Severity
This is a feat PR that introduces diagnostics-channel-based outgoing request span creation, but the diff does not include a new integration or E2E test that specifically exercises this code path. The existing http-basic integration test may cover it implicitly on Node 22.12+ CI runners, but there's no test that explicitly verifies the createSpansForOutgoingRequests / diagnostics channel flow or differentiates it from the OTEL-based flow. Adding a targeted integration test would guard against regressions in this specific feature.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The assertion that both outgoing requests share the same trace ID only holds on Node 22+ (diagnostics channel path). On Node <22, OTEL creates separate spans per request, each with their own trace ID.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing integration or E2E test for new feature
- Added two integration tests that verify span creation, trace propagation, and the interaction between createSpansForOutgoingRequests and the spans option for outgoing HTTP requests using diagnostics channels on Node 22.12+.
Or push these changes by commenting:
@cursor push b5fc75535a
Preview (b5fc75535a)
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/instrument.mjs
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/instrument.mjs@@ -1,0 +1,10 @@+import * as Sentry from '@sentry/node';+import { loggingTransport } from '@sentry-internal/node-integration-tests';++Sentry.init({+ dsn: 'https://public@dsn.ingest.sentry.io/1337',+ release: '1.0',+ tracesSampleRate: 1.0,+ integrations: [Sentry.httpIntegration({ spans: false })],+ transport: loggingTransport,+});diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/scenario.mjs
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/scenario.mjs@@ -1,0 +1,23 @@+import * as Sentry from '@sentry/node';+import * as http from 'http';++Sentry.startSpan({ name: 'test_span' }, async () => {+ await makeHttpRequest(`${process.env.SERVER_URL}/api/test`);+});++function makeHttpRequest(url) {+ return new Promise((resolve, reject) => {+ http+ .request(url, httpRes => {+ httpRes.on('data', () => {+ // we don't care about data+ });+ httpRes.on('end', () => {+ resolve();+ });+ httpRes.on('error', reject);+ })+ .on('error', reject)+ .end();+ });+}diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/test.ts
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans-disabled/test.ts@@ -1,0 +1,41 @@+import { createTestServer } from '@sentry-internal/test-utils';+import { parseSemver } from '@sentry/core';+import { describe, expect } from 'vitest';+import { createEsmAndCjsTests } from '../../../../utils/runner';++const NODE_VERSION = parseSemver(process.versions.node);++const supportsHttpDiagnosticsChannel =+ (NODE_VERSION.major === 22 && NODE_VERSION.minor >= 12) ||+ (NODE_VERSION.major === 23 && NODE_VERSION.minor >= 2) ||+ NODE_VERSION.major >= 24;++const testIfSupported = supportsHttpDiagnosticsChannel ? describe : describe.skip;++testIfSupported('outgoing http with diagnostics channel spans disabled', () => {+ createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {+ test('does not create spans when spans option is false', async () => {+ expect.assertions(3);++ const [SERVER_URL, closeTestServer] = await createTestServer()+ .get('/api/test', headers => {+ expect(headers['sentry-trace']).toEqual(expect.any(String));+ })+ .start();++ await createRunner()+ .withEnv({ SERVER_URL })+ .expect({+ transaction: event => {+ expect(event.transaction).toBe('test_span');++ const httpClientSpans = event.spans?.filter(span => span.op === 'http.client');+ expect(httpClientSpans).toHaveLength(0);+ },+ })+ .start()+ .completed();+ closeTestServer();+ });+ });+});diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/instrument.mjs
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/instrument.mjs@@ -1,0 +1,10 @@+import * as Sentry from '@sentry/node';+import { loggingTransport } from '@sentry-internal/node-integration-tests';++Sentry.init({+ dsn: 'https://public@dsn.ingest.sentry.io/1337',+ release: '1.0',+ tracesSampleRate: 1.0,+ integrations: [],+ transport: loggingTransport,+});diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/scenario.mjs
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/scenario.mjs@@ -1,0 +1,23 @@+import * as Sentry from '@sentry/node';+import * as http from 'http';++Sentry.startSpan({ name: 'test_span' }, async () => {+ await makeHttpRequest(`${process.env.SERVER_URL}/api/test`);+});++function makeHttpRequest(url) {+ return new Promise((resolve, reject) => {+ http+ .request(url, httpRes => {+ httpRes.on('data', () => {+ // we don't care about data+ });+ httpRes.on('end', () => {+ resolve();+ });+ httpRes.on('error', reject);+ })+ .on('error', reject)+ .end();+ });+}diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/test.ts
new file mode 100644
--- /dev/null+++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-diagnostics-channel-spans/test.ts@@ -1,0 +1,65 @@+import { createTestServer } from '@sentry-internal/test-utils';+import { parseSemver } from '@sentry/core';+import { describe, expect } from 'vitest';+import { createEsmAndCjsTests } from '../../../../utils/runner';++const NODE_VERSION = parseSemver(process.versions.node);++// The `http.client.request.created` diagnostics channel was added in Node 22.12.0 and 23.2.0+const supportsHttpDiagnosticsChannel =+ (NODE_VERSION.major === 22 && NODE_VERSION.minor >= 12) ||+ (NODE_VERSION.major === 23 && NODE_VERSION.minor >= 2) ||+ NODE_VERSION.major >= 24;++const testIfSupported = supportsHttpDiagnosticsChannel ? describe : describe.skip;++testIfSupported('outgoing http with diagnostics channel spans', () => {+ createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {+ test('creates spans for outgoing requests and propagates trace context within span', async () => {+ expect.assertions(8);++ let outgoingSpanId: string | undefined;++ const [SERVER_URL, closeTestServer] = await createTestServer()+ .get('/api/test', headers => {+ // Verify trace propagation headers are present+ expect(headers['baggage']).toEqual(expect.any(String));+ expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f\d]{32})-([a-f\d]{16})-1$/));++ // Extract the span ID from the sentry-trace header+ const sentryTrace = headers['sentry-trace'] as string;+ outgoingSpanId = sentryTrace.split('-')[1];++ // Verify we're not propagating all-zero trace IDs+ expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1');+ })+ .start();++ await createRunner()+ .withEnv({ SERVER_URL })+ .expect({+ transaction: event => {+ expect(event.transaction).toBe('test_span');++ // Verify that an http.client span was created+ const httpClientSpans = event.spans?.filter(span => span.op === 'http.client');+ expect(httpClientSpans).toHaveLength(1);++ const httpSpan = httpClientSpans![0];+ expect(httpSpan?.description).toMatch(/^GET /);++ // Verify the propagated span ID matches the created span+ if (outgoingSpanId) {+ expect(httpSpan?.span_id).toBe(outgoingSpanId);+ }++ // Verify span attributes include sentry.origin+ expect(httpSpan?.data?.['sentry.origin']).toBe('auto.http.otel.http');+ },+ })+ .start()+ .completed();+ closeTestServer();+ });+ });+});This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| }); | ||
| return span; | ||
| } |
There was a problem hiding this comment.
Missing integration or E2E test for new feature
Low Severity
This is a feat PR introducing a significant new capability (outgoing request span creation via diagnostics channels). The diff only contains unit tests for _shouldUseOtelHttpInstrumentation and mergeBaggageHeaders, but no integration or E2E test verifying the new span creation flow, trace propagation within span context, or the interaction between createSpansForOutgoingRequests/disableOutgoingRequestInstrumentation. Adding at least one integration test covering end-to-end span creation and propagation for outgoing HTTP requests on Node 22+ would help catch regressions.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Registers diagnostics channels for outgoing requests on Node >= 22 that takes
care of creating spans, rather than relying on OTEL instrumentation.
Closes#18497 (added automatically)