From 699978c6b41e7616c95d5c616e8ea5b940d02713 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Wed, 16 Oct 2024 13:10:22 -0700 Subject: [PATCH] fix(instrumentation-http): fix http/https ESM instr for 'import defaultExport from' style Fix instrumentation of `http.get`, `http.request`, `https.get`, and `https.request` when used from ESM code and imported via the `import defaultExport from 'http'` style. Fixes: #5024 --- experimental/CHANGELOG.md | 1 + .../src/http.ts | 18 +- .../test/integrations/esm.test.mjs | 366 ++++++++++-------- 3 files changed, 230 insertions(+), 155 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index e0a1a1a5c3a..724a0c32882 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -49,6 +49,7 @@ All notable changes to experimental packages in this project will be documented * `OTEL_EXPORTER_OTLP_LOGS_INSECURE` * fix(sdk-node): use warn instead of error on unknown OTEL_NODE_RESOURCE_DETECTORS values [#5034](https://github.com/open-telemetry/opentelemetry-js/pull/5034) * fix(exporter-logs-otlp-proto): Use correct config type in Node constructor +* fix(instrumentation-http): Fix instrumentation of `http.get`, `http.request`, `https.get`, and `https.request` when used from ESM code and imported via the `import defaultExport from 'http'` style. [#5024](https://github.com/open-telemetry/opentelemetry-js/issues/5024) @trentm ### :books: (Refine Doc) diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index 0d0a63920b2..7c7eb64dada 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -235,17 +235,24 @@ export class HttpInstrumentation extends InstrumentationBase { + const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module'; if (!this.getConfig().disableOutgoingRequestInstrumentation) { const patchedRequest = this._wrap( moduleExports, 'request', this._getPatchOutgoingRequestFunction('http') ) as unknown as Func; - this._wrap( + const patchedGet = this._wrap( moduleExports, 'get', this._getPatchOutgoingGetFunction(patchedRequest) ); + if (isESM) { + // To handle `import http from 'http'`, which returns the default + // export, we need to set `module.default.*`. + (moduleExports as any).default.request = patchedRequest; + (moduleExports as any).default.get = patchedGet; + } } if (!this.getConfig().disableIncomingRequestInstrumentation) { this._wrap( @@ -275,17 +282,24 @@ export class HttpInstrumentation extends InstrumentationBase { + const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module'; if (!this.getConfig().disableOutgoingRequestInstrumentation) { const patchedRequest = this._wrap( moduleExports, 'request', this._getPatchHttpsOutgoingRequestFunction('https') ) as unknown as Func; - this._wrap( + const patchedGet = this._wrap( moduleExports, 'get', this._getPatchHttpsOutgoingGetFunction(patchedRequest) ); + if (isESM) { + // To handle `import https from 'https'`, which returns the default + // export, we need to set `module.default.*`. + (moduleExports as any).default.request = patchedRequest; + (moduleExports as any).default.get = patchedGet; + } } if (!this.getConfig().disableIncomingRequestInstrumentation) { this._wrap( diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs index db35b4ee455..da19460c4eb 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -18,8 +18,22 @@ import * as assert from 'assert'; import * as fs from 'fs'; -import * as http from 'http'; -import * as https from 'https'; + +import httpA from 'http'; // ESM import style A +import * as httpB from 'http'; // ESM import style B +import { + createServer as httpCreateServerC, + request as httpRequestC, + get as httpGetC, +} from 'http'; // ESM import style C + +import httpsA from 'https'; // ESM import style A +import * as httpsB from 'https'; // ESM import style B +import { + createServer as httpsCreateServerC, + request as httpsRequestC, + get as httpsGetC, +} from 'https'; // ESM import style C import { SpanKind } from '@opentelemetry/api'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; @@ -37,183 +51,229 @@ provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); const instrumentation = new HttpInstrumentation(); instrumentation.setTracerProvider(provider); -describe('HttpInstrumentation ESM Integration tests', () => { - let port; - let server; +const httpImports = [ + { + style: 'import http from "http"', + createServer: httpA.createServer, + request: httpA.request, + get: httpA.get, + }, + { + style: 'import * as http from "http"', + createServer: httpB.createServer, + request: httpB.request, + get: httpB.get, + }, + { + style: 'import {...} from "http"', + createServer: httpCreateServerC, + request: httpRequestC, + get: httpGetC, + }, +]; - before(done => { - server = http.createServer((req, res) => { - req.resume(); - req.on('end', () => { - res.writeHead(200); - res.end('pong'); +for (let httpImport of httpImports) { + describe(`HttpInstrumentation ESM Integration tests (${httpImport.style})`, () => { + let port; + let server; + + before(done => { + server = httpImport.createServer((req, res) => { + req.resume(); + req.on('end', () => { + res.writeHead(200); + res.end('pong'); + }); }); - }); - server.listen(0, '127.0.0.1', () => { - port = server.address().port; - assert.ok(Number.isInteger(port)); - done(); + server.listen(0, '127.0.0.1', () => { + port = server.address().port; + assert.ok(Number.isInteger(port)); + done(); + }); }); - }); - - after(done => { - server.close(done); - }); - beforeEach(() => { - memoryExporter.reset(); - }); + after(done => { + server.close(done); + }); - it('should instrument http requests using http.request', async () => { - const spanValidations = { - httpStatusCode: 200, - httpMethod: 'GET', - hostname: '127.0.0.1', - pathname: '/http.request', - component: 'http', - }; - - await new Promise(resolve => { - const clientReq = http.request( - `http://127.0.0.1:${port}/http.request`, - clientRes => { - spanValidations.resHeaders = clientRes.headers; - clientRes.resume(); - clientRes.on('end', resolve); - } - ); - clientReq.end(); + beforeEach(() => { + memoryExporter.reset(); }); - let spans = memoryExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 2); - const span = spans.find(s => s.kind === SpanKind.CLIENT); - assert.strictEqual(span.name, 'GET'); - assertSpan(span, SpanKind.CLIENT, spanValidations); - }); + it('should instrument http requests using http.request', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/http.request', + component: 'http', + }; - it('should instrument http requests using http.get', async () => { - const spanValidations = { - httpStatusCode: 200, - httpMethod: 'GET', - hostname: '127.0.0.1', - pathname: '/http.get', - component: 'http', - }; - - await new Promise(resolve => { - http.get(`http://127.0.0.1:${port}/http.get`, clientRes => { - spanValidations.resHeaders = clientRes.headers; - clientRes.resume(); - clientRes.on('end', resolve); + await new Promise(resolve => { + const clientReq = httpImport.request( + `http://127.0.0.1:${port}/http.request`, + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); + } + ); + clientReq.end(); }); + + let spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2); + const span = spans.find(s => s.kind === SpanKind.CLIENT); + assert.strictEqual(span.name, 'GET'); + assertSpan(span, SpanKind.CLIENT, spanValidations); }); - let spans = memoryExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 2); - const span = spans.find(s => s.kind === SpanKind.CLIENT); - assert.strictEqual(span.name, 'GET'); - assertSpan(span, SpanKind.CLIENT, spanValidations); - }); -}); - -describe('HttpsInstrumentation ESM Integration tests', () => { - let port; - let server; - - before(done => { - server = https.createServer( - { - key: fs.readFileSync( - new URL('../fixtures/server-key.pem', import.meta.url) - ), - cert: fs.readFileSync( - new URL('../fixtures/server-cert.pem', import.meta.url) - ), - }, - (req, res) => { - req.resume(); - req.on('end', () => { - res.writeHead(200); - res.end('pong'); + it('should instrument http requests using http.get', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/http.get', + component: 'http', + }; + + await new Promise(resolve => { + httpImport.get(`http://127.0.0.1:${port}/http.get`, clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); }); - } - ); + }); - server.listen(0, '127.0.0.1', () => { - port = server.address().port; - assert.ok(Number.isInteger(port)); - done(); + let spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2); + const span = spans.find(s => s.kind === SpanKind.CLIENT); + assert.strictEqual(span.name, 'GET'); + assertSpan(span, SpanKind.CLIENT, spanValidations); }); }); +} - after(done => { - server.close(done); - }); +const httpsImports = [ + { + style: 'import https from "https"', + createServer: httpsA.createServer, + request: httpsA.request, + get: httpsA.get, + }, + { + style: 'import * as https from "https"', + createServer: httpsB.createServer, + request: httpsB.request, + get: httpsB.get, + }, + { + style: 'import {...} from "https"', + createServer: httpsCreateServerC, + request: httpsRequestC, + get: httpsGetC, + }, +]; - beforeEach(() => { - memoryExporter.reset(); - }); +for (let httpsImport of httpsImports) { + describe(`HttpsInstrumentation ESM Integration tests (${httpsImport.style})`, () => { + let port; + let server; - it('should instrument https requests using https.request', async () => { - const spanValidations = { - httpStatusCode: 200, - httpMethod: 'GET', - hostname: '127.0.0.1', - pathname: '/https.request', - component: 'https', - }; - - await new Promise(resolve => { - const clientReq = https.request( - `https://127.0.0.1:${port}/https.request`, + before(done => { + server = httpsImport.createServer( { - rejectUnauthorized: false, + key: fs.readFileSync( + new URL('../fixtures/server-key.pem', import.meta.url) + ), + cert: fs.readFileSync( + new URL('../fixtures/server-cert.pem', import.meta.url) + ), }, - clientRes => { - spanValidations.resHeaders = clientRes.headers; - clientRes.resume(); - clientRes.on('end', resolve); + (req, res) => { + req.resume(); + req.on('end', () => { + res.writeHead(200); + res.end('pong'); + }); } ); - clientReq.end(); + + server.listen(0, '127.0.0.1', () => { + port = server.address().port; + assert.ok(Number.isInteger(port)); + done(); + }); }); - let spans = memoryExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 2); - const span = spans.find(s => s.kind === SpanKind.CLIENT); - assert.strictEqual(span.name, 'GET'); - assertSpan(span, SpanKind.CLIENT, spanValidations); - }); + after(done => { + server.close(done); + }); - it('should instrument http requests using https.get', async () => { - const spanValidations = { - httpStatusCode: 200, - httpMethod: 'GET', - hostname: '127.0.0.1', - pathname: '/https.get', - component: 'https', - }; - - await new Promise(resolve => { - https.get( - `https://127.0.0.1:${port}/https.get`, - { - rejectUnauthorized: false, - }, - clientRes => { - spanValidations.resHeaders = clientRes.headers; - clientRes.resume(); - clientRes.on('end', resolve); - } - ); + beforeEach(() => { + memoryExporter.reset(); + }); + + it('should instrument https requests using https.request', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/https.request', + component: 'https', + }; + + await new Promise(resolve => { + const clientReq = httpsImport.request( + `https://127.0.0.1:${port}/https.request`, + { + rejectUnauthorized: false, + }, + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); + } + ); + clientReq.end(); + }); + + let spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2); + const span = spans.find(s => s.kind === SpanKind.CLIENT); + assert.strictEqual(span.name, 'GET'); + assertSpan(span, SpanKind.CLIENT, spanValidations); }); - let spans = memoryExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 2); - const span = spans.find(s => s.kind === SpanKind.CLIENT); - assert.strictEqual(span.name, 'GET'); - assertSpan(span, SpanKind.CLIENT, spanValidations); + it('should instrument http requests using https.get', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/https.get', + component: 'https', + }; + + await new Promise(resolve => { + httpsImport.get( + `https://127.0.0.1:${port}/https.get`, + { + rejectUnauthorized: false, + }, + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); + } + ); + }); + + let spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2); + const span = spans.find(s => s.kind === SpanKind.CLIENT); + assert.strictEqual(span.name, 'GET'); + assertSpan(span, SpanKind.CLIENT, spanValidations); + }); }); -}); +}