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 <jamieedanielson@gmail.com>

* test name suggestion

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* test name suggestion

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* test name suggestion

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* var naming suggestion: expand cres and creq, the abbrev isn't obvious

---------

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

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)

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",

experimental/packages/opentelemetry-instrumentation-http/src/http.ts

@@ -110,15 +110,15 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
       'http',
       ['*'],
       (moduleExports: Http): Http => {
-        this._wrap(
+        const patchedRequest = this._wrap(
           moduleExports,
           'request',
           this._getPatchOutgoingRequestFunction('http')
-        );
+        ) as unknown as Func<http.ClientRequest>;
         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<HttpInstrumentation
       'https',
       ['*'],
       (moduleExports: Https): Https => {
-        this._wrap(
+        const patchedRequest = this._wrap(
           moduleExports,
           'request',
           this._getPatchHttpsOutgoingRequestFunction('https')
-        );
+        ) as unknown as Func<http.ClientRequest>;
         this._wrap(
           moduleExports,
           'get',
-          this._getPatchHttpsOutgoingGetFunction(moduleExports.request)
+          this._getPatchHttpsOutgoingGetFunction(patchedRequest)
         );
+
         this._wrap(
           moduleExports.Server.prototype,
           'emit',

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);
+  });
+});

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",

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;
     }
   };
 

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',
       ['*'],