Merge pull request #1 from honeycombio/purvi/esm-tests

ESM test & small refactor

Author: JamieDanielson

examples/esm-http-ts/index.ts

@@ -1,5 +1,5 @@
 import { registerInstrumentations } from '@opentelemetry/instrumentation';
-import { trace } from '@opentelemetry/api';
+import { trace, DiagConsoleLogger, DiagLogLevel, diag } from '@opentelemetry/api';
 import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
 import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
 import {
@@ -10,6 +10,7 @@ import { Resource } from '@opentelemetry/resources';
 import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
 import http from 'http';
 
+diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);
 const tracerProvider = new NodeTracerProvider({
   resource: new Resource({
     [SemanticResourceAttributes.SERVICE_NAME]: 'esm-http-ts-example',

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

@@ -61,7 +61,28 @@ export abstract class InstrumentationAbstract<T = any>
   }
 
   /* Api to wrap instrumented method */
-  protected _wrap = shimmer.wrap;
+  protected _wrap = (
+    moduleExports: any,
+    name: string,
+    wrapper: (originalFn: any) => any
+  ) => {
+    try {
+      return shimmer.wrap(moduleExports, name, wrapper);
+    } catch (e) {
+      // shimmer doesn't handle Proxy objects well
+      // if there is an error from import in the middle providing
+      // a Proxy of a Module we have to pass it to shimmer as a regular object
+      const wrapped: any = shimmer.wrap(
+        Object.assign({}, moduleExports),
+        name,
+        wrapper
+      );
+      Object.defineProperty(moduleExports, name, {
+        value: wrapped,
+      });
+      return moduleExports;
+    }
+  };
   /* Api to unwrap instrumented methods */
   protected _unwrap = shimmer.unwrap;
   /* Api to mass wrap instrumented method */

experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

@@ -172,7 +172,7 @@ export abstract class InstrumentationBase<T = any>
       const hookFn: ImportInTheMiddle.HookFn = (exports, name, baseDir) => {
         return this._onRequire<typeof exports>(
           module as unknown as InstrumentationModuleDefinition<typeof exports>,
-          Object.assign({}, exports),
+          exports,
           name,
           baseDir
         );
@@ -197,11 +197,13 @@ export abstract class InstrumentationBase<T = any>
         : this._requireInTheMiddleSingleton.register(module.name, onRequire);
 
       this._hooks.push(hook);
-      new (ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default)(
-        [module.name],
-        { internals: true },
-        <HookFn>hookFn
-      );
+      const esmHook =
+        new (ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default)(
+          [module.name],
+          { internals: false },
+          <HookFn>hookFn
+        );
+      this._hooks.push(esmHook);
     }
   }
 

experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs

@@ -18,32 +18,73 @@ import * as assert from 'assert';
 import {
   InstrumentationBase,
   InstrumentationNodeModuleDefinition,
+  isWrapped,
 } from '../../build/src/index.js';
+import * as exported from 'test-esm-module';
 
-describe('when loading esm module', () => {
-  it('should patch module file', async () => {
-    class TestInstrumentation extends InstrumentationBase {
-      constructor() {
-        super('test-esm-instrumentation', '0.0.1');
+class TestInstrumentationWrapFn extends InstrumentationBase {
+  constructor(config) {
+    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 () => 'a different result';
+        });
+        return moduleExports;
+      },
+      moduleExports => {
+        this._unwrap(moduleExports, 'testFunction');
+        console.log('second');
+        return moduleExports;
       }
-      init() {
-        console.log('test-esm-instrumentation initialized!');
-        return new InstrumentationNodeModuleDefinition(
-          'test-esm-module',
-          ['*'],
-          moduleExports => {
-            console.log('patch');
-            moduleExports.testConstant = 43;
-          }
-        );
+    );
+  }
+}
+
+class TestInstrumentationSimple extends InstrumentationBase {
+  constructor(config) {
+    super('test-esm-instrumentation', '0.0.1', config);
+  }
+  init() {
+    console.log('test-esm-instrumentation initialized!');
+    return new InstrumentationNodeModuleDefinition(
+      'test-esm-module',
+      ['*'],
+      moduleExports => {
+        moduleExports.testConstant = 43;
+        return moduleExports;
       }
-    }
-    const instrumentation = new TestInstrumentation();
+    );
+  }
+}
+describe('when loading esm module', () => {
+  const instrumentationWrap = new TestInstrumentationWrapFn({
+    enabled: false,
+  });
+
+  it('should patch module file directly', async () => {
+    const instrumentation = new TestInstrumentationSimple({
+      enabled: false,
+    });
     instrumentation.enable();
-    // const exported = await import('test-esm-module');
-    // assert.deepEqual(exported.testConstant, 43);
+    assert.deepEqual(exported.testConstant, 43);
+  });
 
-    // error from import-in-the-middle register
-    // TypeError: setters.get(...)[name] is not a function
+  it('should patch a module with the wrap function', async () => {
+    instrumentationWrap.enable();
+    assert.deepEqual(exported.testFunction(), 'a different result');
   });
+
+  // it('should be able to unwrap a patched function', async () => {
+  //   // disable to trigger unwrap
+  //   const exported = await import('test-esm-module');
+  //   instrumentationWrap.enable();
+  //   instrumentationWrap.disable();
+  //   assert.deepEqual(exported.testFunction(), 'test');
+  // });
 });