From ecb1c9b158c0b399cd2fa9e29d2dd81dfff471ff Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 23 Jul 2024 22:08:41 -0700 Subject: [PATCH] fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.get` work when used in ESM code (#4866) * 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 * correct typo in the changelog message * does this fix the test:esm script running on windows? * remove other console.logs (presumably dev leftovers) from tests in this file * test name suggestion Co-authored-by: Jamie Danielson * test name suggestion Co-authored-by: Jamie Danielson * test name suggestion Co-authored-by: Jamie Danielson * test name suggestion Co-authored-by: Jamie Danielson * var naming suggestion: expand cres and creq, the abbrev isn't obvious --------- Co-authored-by: Jamie Danielson --- 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 | 15 +- 7 files changed, 244 insertions(+), 15 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..ab188c2752 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.get` 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..8baa46be19 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/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", "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..db35b4ee45 --- /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('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(); + }); + + 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.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); + }); + }); + + 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('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`, + { + 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); + }); + + 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); + } + ); + }); + + 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 1edd465ad4..bcd046235a 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..a46fdc0fd3 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 => { @@ -49,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', ['*'], @@ -79,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', ['*'],