From 1563f93ecd9a97c32f3a9961ce81bfd01620354f Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 15 Jul 2024 15:39:18 -0700 Subject: [PATCH 1/9] fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.get` work when used in ESM code The issue was that the `_wrap`ing of `http.get` was getting the just-wrapped `http.request` by accessing `moduleExports.request`. However, when wrapping an ES module the `moduleExports` object from IITM is a Proxy object that allows setting a property, but *not* re-getting that set property. The fix is to use the wrapped `http.request` from the `this._wrap` call. That required fixing a bug in the IITM code-path of `InstrumentationBase.prototype._wrap` to return the wrapped property. (The previous code was doing `return Object.defineProperty(...)`, which returns the moduleExports, not the defined property.) Fixes: #4857 --- experimental/CHANGELOG.md | 2 + .../package.json | 4 +- .../src/http.ts | 13 +- .../test/integrations/esm.test.mjs | 219 ++++++++++++++++++ .../package.json | 2 +- .../src/platform/node/instrumentation.ts | 4 +- .../test/node/EsmInstrumentation.test.mjs | 13 +- 7 files changed, 244 insertions(+), 13 deletions(-) create mode 100644 experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 725578ab7a..3994f2c26f 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) +* fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.request` work when used in ESM code [#4857](https://github.com/open-telemetry/opentelemetry-js/issues/4857) @trentm + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/experimental/packages/opentelemetry-instrumentation-http/package.json b/experimental/packages/opentelemetry-instrumentation-http/package.json index e311c6dd7b..42a9e05a56 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/package.json +++ b/experimental/packages/opentelemetry-instrumentation-http/package.json @@ -9,7 +9,9 @@ "prepublishOnly": "npm run compile", "compile": "tsc --build", "clean": "tsc --build --clean", - "test": "nyc mocha test/**/*.test.ts", + "test:cjs": "nyc mocha test/**/*.test.ts", + "test:esm": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/.bin/mocha 'test/**/*.test.mjs'", + "test": "npm run test:cjs && npm run test:esm", "tdd": "npm run test -- --watch-extensions ts --watch", "lint": "eslint . --ext .ts", "lint:fix": "eslint . --ext .ts --fix", diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index a42927fff5..94fc1e1d95 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -110,15 +110,15 @@ export class HttpInstrumentation extends InstrumentationBase { - this._wrap( + const patchedRequest = this._wrap( moduleExports, 'request', this._getPatchOutgoingRequestFunction('http') - ); + ) as unknown as Func; this._wrap( moduleExports, 'get', - this._getPatchOutgoingGetFunction(moduleExports.request) + this._getPatchOutgoingGetFunction(patchedRequest) ); this._wrap( moduleExports.Server.prototype, @@ -142,16 +142,17 @@ export class HttpInstrumentation extends InstrumentationBase { - this._wrap( + const patchedRequest = this._wrap( moduleExports, 'request', this._getPatchHttpsOutgoingRequestFunction('https') - ); + ) as unknown as Func; this._wrap( moduleExports, 'get', - this._getPatchHttpsOutgoingGetFunction(moduleExports.request) + this._getPatchHttpsOutgoingGetFunction(patchedRequest) ); + this._wrap( moduleExports.Server.prototype, 'emit', diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs new file mode 100644 index 0000000000..4d9038b2d5 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -0,0 +1,219 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Test that instrumentation-http works when used from ESM code. + +import * as assert from 'assert'; +import * as fs from 'fs'; +import * as http from 'http'; +import * as https from 'https'; + +import { SpanKind } from '@opentelemetry/api'; +import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; +import { + InMemorySpanExporter, + SimpleSpanProcessor, +} from '@opentelemetry/sdk-trace-base'; + +import { assertSpan } from '../../build/test/utils/assertSpan.js'; +import { HttpInstrumentation } from '../../build/src/index.js'; + +const provider = new NodeTracerProvider(); +const memoryExporter = new InMemorySpanExporter(); +provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); +const instrumentation = new HttpInstrumentation(); +instrumentation.setTracerProvider(provider); + +describe('HttpInstrumentation ESM Integration tests', () => { + let port; + let server; + + before(done => { + server = http.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(); + }); + }); + + after(done => { + server.close(done); + }); + + beforeEach(() => { + memoryExporter.reset(); + }); + + it('http.request', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/http.request', + component: 'http', + }; + + await new Promise(resolve => { + const creq = http.request( + `http://127.0.0.1:${port}/http.request`, + cres => { + spanValidations.resHeaders = cres.headers; + cres.resume(); + cres.on('end', resolve); + } + ); + creq.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); + }); + + it('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`, cres => { + spanValidations.resHeaders = cres.headers; + cres.resume(); + cres.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); + }); +}); + +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'); + }); + } + ); + + 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(); + }); + + it('https.request', async () => { + const spanValidations = { + httpStatusCode: 200, + httpMethod: 'GET', + hostname: '127.0.0.1', + pathname: '/https.request', + component: 'https', + }; + + await new Promise(resolve => { + const creq = https.request( + `https://127.0.0.1:${port}/https.request`, + { + rejectUnauthorized: false, + }, + cres => { + spanValidations.resHeaders = cres.headers; + cres.resume(); + cres.on('end', resolve); + } + ); + creq.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); + }); + + it('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, + }, + cres => { + spanValidations.resHeaders = cres.headers; + cres.resume(); + cres.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); + }); +}); diff --git a/experimental/packages/opentelemetry-instrumentation/package.json b/experimental/packages/opentelemetry-instrumentation/package.json index 79a3de0ff3..dfedae305b 100644 --- a/experimental/packages/opentelemetry-instrumentation/package.json +++ b/experimental/packages/opentelemetry-instrumentation/package.json @@ -49,7 +49,7 @@ "tdd:node": "npm run test -- --watch-extensions ts --watch", "tdd:browser": "karma start", "test:cjs": "nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'", - "test:esm": "nyc node --experimental-loader=./hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs' test/node/*.test.mjs", + "test:esm": "nyc node --experimental-loader=./hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs'", "test": "npm run test:cjs && npm run test:esm", "test:browser": "karma start --single-run", "version": "node ../../../scripts/version-update.js", diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 0cd181a1ca..cad74be5a0 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -87,10 +87,10 @@ export abstract class InstrumentationBase< return wrap(moduleExports, name, wrapper); } else { const wrapped = wrap(Object.assign({}, moduleExports), name, wrapper); - - return Object.defineProperty(moduleExports, name, { + Object.defineProperty(moduleExports, name, { value: wrapped, }); + return wrapped; } }; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs index f09097cd79..3f8170d730 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs @@ -26,14 +26,21 @@ class TestInstrumentationWrapFn extends InstrumentationBase { super('test-esm-instrumentation', '0.0.1', config); } init() { - console.log('test-esm-instrumentation initialized!'); return new InstrumentationNodeModuleDefinition( 'test-esm-module', ['*'], moduleExports => { - this._wrap(moduleExports, 'testFunction', () => { - return () => 'patched'; + const wrapRetval = this._wrap(moduleExports, 'testFunction', () => { + return function wrappedTestFunction() { + return 'patched'; + }; }); + assert.strictEqual(typeof wrapRetval, 'function'); + assert.strictEqual( + wrapRetval.name, + 'wrappedTestFunction', + '_wrap(..., "testFunction", ...) return value is the wrapped function' + ); return moduleExports; }, moduleExports => { From 4eb860728f34af3654c315c43a5604915743b66a Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 15 Jul 2024 15:47:20 -0700 Subject: [PATCH 2/9] correct typo in the changelog message --- experimental/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 3994f2c26f..ab188c2752 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -13,7 +13,7 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) -* fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.request` work when used in ESM code [#4857](https://github.com/open-telemetry/opentelemetry-js/issues/4857) @trentm +* fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.get` work when used in ESM code [#4857](https://github.com/open-telemetry/opentelemetry-js/issues/4857) @trentm ### :books: (Refine Doc) From 94933d63666fce15795a5aedc72db95187de46d4 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 15 Jul 2024 15:57:54 -0700 Subject: [PATCH 3/9] does this fix the test:esm script running on windows? --- .../packages/opentelemetry-instrumentation-http/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation-http/package.json b/experimental/packages/opentelemetry-instrumentation-http/package.json index 42a9e05a56..8baa46be19 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/package.json +++ b/experimental/packages/opentelemetry-instrumentation-http/package.json @@ -10,7 +10,7 @@ "compile": "tsc --build", "clean": "tsc --build --clean", "test:cjs": "nyc mocha test/**/*.test.ts", - "test:esm": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/.bin/mocha 'test/**/*.test.mjs'", + "test:esm": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/**/*.test.mjs'", "test": "npm run test:cjs && npm run test:esm", "tdd": "npm run test -- --watch-extensions ts --watch", "lint": "eslint . --ext .ts", From 942aa19729e44616183d6b9fe5cc720007e1fe98 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 16 Jul 2024 22:55:49 -0700 Subject: [PATCH 4/9] remove other console.logs (presumably dev leftovers) from tests in this file --- .../test/node/EsmInstrumentation.test.mjs | 2 -- 1 file changed, 2 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs index 3f8170d730..a46fdc0fd3 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs @@ -56,7 +56,6 @@ class TestInstrumentationMasswrapFn extends InstrumentationBase { super('test-esm-instrumentation', '0.0.1', config); } init() { - console.log('test-esm-instrumentation initialized!'); return new InstrumentationNodeModuleDefinition( 'test-esm-module', ['*'], @@ -86,7 +85,6 @@ class TestInstrumentationSimple extends InstrumentationBase { super('test-esm-instrumentation', '0.0.1', config); } init() { - console.log('test-esm-instrumentation initialized!'); return new InstrumentationNodeModuleDefinition( 'test-esm-module', ['*'], From ed68ff78a892f0481190889b2783f805f910990d Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 21:27:05 -0700 Subject: [PATCH 5/9] test name suggestion Co-authored-by: Jamie Danielson --- .../test/integrations/esm.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4d9038b2d5..e7ec84fa9f 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -187,7 +187,7 @@ describe('HttpsInstrumentation ESM Integration tests', () => { assertSpan(span, SpanKind.CLIENT, spanValidations); }); - it('https.get', async () => { + it('should instrument http requests using https.get', async () => { const spanValidations = { httpStatusCode: 200, httpMethod: 'GET', From 5a40e94318a20bd458ccf99ecf968db6d5b95b67 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 21:27:18 -0700 Subject: [PATCH 6/9] test name suggestion Co-authored-by: Jamie Danielson --- .../test/integrations/esm.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e7ec84fa9f..937476db2c 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -156,7 +156,7 @@ describe('HttpsInstrumentation ESM Integration tests', () => { memoryExporter.reset(); }); - it('https.request', async () => { + it('should instrument https requests using https.request', async () => { const spanValidations = { httpStatusCode: 200, httpMethod: 'GET', From 4ff377160b6e935fe933710d6bfd4ab543f9e123 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 21:27:34 -0700 Subject: [PATCH 7/9] test name suggestion Co-authored-by: Jamie Danielson --- .../test/integrations/esm.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 937476db2c..abbf23abad 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -93,7 +93,7 @@ describe('HttpInstrumentation ESM Integration tests', () => { assertSpan(span, SpanKind.CLIENT, spanValidations); }); - it('http.get', async () => { + it('should instrument http requests using http.get', async () => { const spanValidations = { httpStatusCode: 200, httpMethod: 'GET', From 71811c46b40d8379d4478b0af939be94890d9d4d Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 21:27:44 -0700 Subject: [PATCH 8/9] test name suggestion Co-authored-by: Jamie Danielson --- .../test/integrations/esm.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 abbf23abad..93816077a8 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -65,7 +65,7 @@ describe('HttpInstrumentation ESM Integration tests', () => { memoryExporter.reset(); }); - it('http.request', async () => { + it('should instrument http requests using http.request', async () => { const spanValidations = { httpStatusCode: 200, httpMethod: 'GET', From 8eb6c7dd7170f7911c31a5e3effa4f9aae45e132 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 21:29:45 -0700 Subject: [PATCH 9/9] var naming suggestion: expand cres and creq, the abbrev isn't obvious --- .../test/integrations/esm.test.mjs | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) 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 93816077a8..db35b4ee45 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs +++ b/experimental/packages/opentelemetry-instrumentation-http/test/integrations/esm.test.mjs @@ -75,15 +75,15 @@ describe('HttpInstrumentation ESM Integration tests', () => { }; await new Promise(resolve => { - const creq = http.request( + const clientReq = http.request( `http://127.0.0.1:${port}/http.request`, - cres => { - spanValidations.resHeaders = cres.headers; - cres.resume(); - cres.on('end', resolve); + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); } ); - creq.end(); + clientReq.end(); }); let spans = memoryExporter.getFinishedSpans(); @@ -103,10 +103,10 @@ describe('HttpInstrumentation ESM Integration tests', () => { }; await new Promise(resolve => { - http.get(`http://127.0.0.1:${port}/http.get`, cres => { - spanValidations.resHeaders = cres.headers; - cres.resume(); - cres.on('end', resolve); + http.get(`http://127.0.0.1:${port}/http.get`, clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); }); }); @@ -166,18 +166,18 @@ describe('HttpsInstrumentation ESM Integration tests', () => { }; await new Promise(resolve => { - const creq = https.request( + const clientReq = https.request( `https://127.0.0.1:${port}/https.request`, { rejectUnauthorized: false, }, - cres => { - spanValidations.resHeaders = cres.headers; - cres.resume(); - cres.on('end', resolve); + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); } ); - creq.end(); + clientReq.end(); }); let spans = memoryExporter.getFinishedSpans(); @@ -202,10 +202,10 @@ describe('HttpsInstrumentation ESM Integration tests', () => { { rejectUnauthorized: false, }, - cres => { - spanValidations.resHeaders = cres.headers; - cres.resume(); - cres.on('end', resolve); + clientRes => { + spanValidations.resHeaders = clientRes.headers; + clientRes.resume(); + clientRes.on('end', resolve); } ); });