feat(node): Update httpIntegration handling of incoming requests (#17371)

This PR updates the handling of the Node SDK of incoming requests.
Instead of relying on @opentelemetry/instrumentation-http for this, we
now handle this internally, ensuring that we can optimize performance as
much as possible and avoid interop problems. This will also allow us to
extract this out of an OTEL instrumentation, eventually paving the way
for a non-OTEL SDK with basic tracing capabilities.

This change should not affect users, unless they are relying on very
in-depth implementation details. Importantly, this also drops the
`_experimentalConfig` option of the integration - this will no longer do
anything.
Finally, you can still pass
`instrumentation.{requestHook,responseHook,applyCustomAttributesOnSpan}`
options, but they are deprecated and will be removed in v11. Instead,
you can use the new `incomingRequestSpanHook` configuration option if
you want to adjust the incoming request span.

I have tried to ensure the spans look as similar as possible as before,
this should be reflected in the tests.

---------

Co-authored-by: Lukas Stracke <[email protected]>

Author: mydea

.size-limit.js

@@ -224,7 +224,7 @@ module.exports = [
     import: createImport('init'),
     ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
     gzip: true,
-    limit: '116 KB',
+    limit: '51 KB',
   },
   // Node SDK (ESM)
   {
@@ -233,14 +233,14 @@ module.exports = [
     import: createImport('init'),
     ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
     gzip: true,
-    limit: '150 KB',
+    limit: '152 KB',
   },
   {
     name: '@sentry/node - without tracing',
     path: 'packages/node/build/esm/index.js',
     import: createImport('initWithoutDefaultIntegrations', 'getDefaultIntegrationsWithoutPerformance'),
     gzip: true,
-    limit: '110 KB',
+    limit: '95 KB',
     ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
     modifyWebpackConfig: function (config) {
       const webpack = require('webpack');
@@ -263,7 +263,7 @@ module.exports = [
     import: createImport('init'),
     ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
     gzip: true,
-    limit: '135 KB',
+    limit: '107 KB',
   },
 ];
 

CHANGELOG.md

@@ -4,6 +4,15 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+### Important Changes
+
+- **feat(node)**: Update `httpIntegration` handling of incoming requests
+
+This version updates the handling of the Node SDK of incoming requests. Instead of relying on @opentelemetry/instrumentation-http, we now handle incoming request instrumentation internally, ensuring that we can optimize performance as much as possible and avoid interop problems.
+
+This change should not affect users, unless they are relying on very in-depth implementation details. Importantly, this also drops the `_experimentalConfig` option of the integration - this will no longer do anything.
+Finally, you can still pass `instrumentation.{requestHook,responseHook,applyCustomAttributesOnSpan}` options, but they are deprecated and will be removed in v11. Instead, you can use the new `incomingRequestSpanHook` configuration option if you want to adjust the incoming request span.
+
 ## 10.8.0
 
 ### Important Changes

dev-packages/e2e-tests/test-applications/node-express-esm-preload/tests/server.test.ts

@@ -117,9 +117,7 @@ test('Should record a transaction for route with parameters', async ({ request }
   });
 });
 
-// This fails https://github.com/getsentry/sentry-javascript/pull/12587#issuecomment-2181019422
-// Skipping this for now so we don't block releases
-test.skip('Should record spans from http instrumentation', async ({ request }) => {
+test('Should record spans from http instrumentation', async ({ request }) => {
   const transactionEventPromise = waitForTransaction('node-express-esm-preload', transactionEvent => {
     return transactionEvent.contexts?.trace?.data?.['http.target'] === '/http-req';
   });

dev-packages/node-integration-tests/suites/express/tracing/updateName/test.ts

@@ -29,11 +29,11 @@ describe('express tracing', () => {
 
     // Also calling `updateName` AND setting a source doesn't change anything - Otel has no concept of source, this is sentry-internal.
     // Therefore, only the source is updated but the name is still overwritten by Otel.
-    test("calling `span.updateName` and setting attribute source doesn't update the final name in express but it updates the source", async () => {
+    test('calling `span.updateName` and setting attribute source updates the final name in express', async () => {
       const runner = createRunner(__dirname, 'server.js')
         .expect({
           transaction: {
-            transaction: 'GET /test/:id/span-updateName-source',
+            transaction: 'new-name',
             transaction_info: {
               source: 'custom',
             },

dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-options.mjs

@@ -4,13 +4,20 @@ import { loggingTransport } from '@sentry-internal/node-integration-tests';
 Sentry.init({
   dsn: 'https://[email protected]/1337',
   release: '1.0',
-  // disable attaching headers to /test/* endpoints
-  tracePropagationTargets: [/^(?!.*test).*$/],
   tracesSampleRate: 1.0,
   transport: loggingTransport,
 
   integrations: [
     Sentry.httpIntegration({
+      incomingRequestSpanHook: (span, req, res) => {
+        span.setAttribute('incomingRequestSpanHook', 'yes');
+        Sentry.setExtra('incomingRequestSpanHookCalled', {
+          reqUrl: req.url,
+          reqMethod: req.method,
+          resUrl: res.req.url,
+          resMethod: res.req.method,
+        });
+      },
       instrumentation: {
         requestHook: (span, req) => {
           span.setAttribute('attr1', 'yes');

dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-overwrite-server-emit.mjs

@@ -0,0 +1,12 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+  // We deliberately disable dedupe here, to make sure we do not double wrap the emit function
+  // Otherwise, duplicate spans (that we want to test against) may be dropped by dedupe detection
+  integrations: integrations => integrations.filter(integration => integration.name !== 'Dedupe'),
+});

dev-packages/node-integration-tests/suites/tracing/httpIntegration/scenario-overwrite-server-emit.mjs

@@ -0,0 +1,39 @@
+import { sendPortToRunner } from '@sentry-internal/node-integration-tests';
+import http from 'http';
+
+const server = http.createServer(function (request, response) {
+  const useProxy = request.url.includes('proxy');
+  const patchOriginalEmit = request.url.includes('original');
+
+  // Monkey patch it again to ensure it still works later
+  // We cover multiple possible scenarios here:
+  // 1. Use proxy to overwrite server.emit
+  // 2. Use proxy to overwrite server.emit, using initial server.emit
+  // 3. Use classic monkey patching to overwrite server.emit
+  // 4. Use classic monkey patching to overwrite server.emit, using initial server.emit
+  monkeyPatchEmit(server, { useProxy, patchOriginalEmit });
+
+  response.end('Hello Node.js Server!');
+});
+
+const initialServerEmit = server.emit;
+
+server.listen(0, () => {
+  sendPortToRunner(server.address().port);
+});
+
+function monkeyPatchEmit(server, { useProxy = false, patchOriginalEmit = false }) {
+  const originalEmit = patchOriginalEmit ? initialServerEmit : server.emit;
+
+  if (useProxy) {
+    server.emit = new Proxy(originalEmit, {
+      apply(target, thisArg, args) {
+        return target.apply(thisArg, args);
+      },
+    });
+  } else {
+    server.emit = function () {
+      return originalEmit.apply(server, arguments);
+    };
+  }
+}