Merge branch 'main' into trentm-lint-examples-express

Author: trentm

.github/workflows/test-all-versions.yml

@@ -163,8 +163,6 @@ jobs:
       - uses: actions/setup-node@v4
         with:
           node-version: ${{ matrix.node }}
-      - name: Set MySQL variables
-        run: mysql --user=root --password=${MYSQL_ROOT_PASSWORD} --host=${MYSQL_HOST} --port=${MYSQL_PORT} -e "SET GLOBAL log_output='TABLE'; SET GLOBAL general_log = 1;" mysql
       - name: Install
         run: npm ci
       - name: Download Build Artifacts

.github/workflows/unit-test.yml

@@ -176,8 +176,6 @@ jobs:
       - uses: actions/setup-node@v4
         with:
           node-version: ${{ matrix.node }}
-      - name: Set MySQL variables
-        run: mysql --user=root --password=${MYSQL_ROOT_PASSWORD} --host=${MYSQL_HOST} --port=${MYSQL_PORT} -e "SET GLOBAL log_output='TABLE'; SET GLOBAL general_log = 1;" mysql
       - name: Install
         run: npm ci
       - name: Download Build Artifacts

CONTRIBUTING.md

@@ -135,6 +135,34 @@ The conventional commit type (in PR title) is very important to automatically bu
 
 There is no need to update the CHANGELOG in a PR because it will be updated as part of the release process (see [RELEASING.md](RELEASING.md) for more details).
 
+### Testing
+
+Most unit tests case be run via:
+
+```sh
+npm test
+```
+
+However, some instrumentations require test-services to be running (e.g. the `instrumentation-mongodb` package requires a MongoDB server). Use the `test-services`-related npm scripts to start all required services in Docker and then run the tests with the appropriate configuration to use those services:
+
+```sh
+npm run test-services:start       # starts services in Docker
+npm run test:with-services-config # runs 'npm test' with envvars from test/test-services.env
+npm run test-services:stop        # stops services in Docker
+```
+
+If you only want to test a single package (e.g. the `instrumentation-mongodb`) you can `cd` into it and run the tests after you started the services.
+
+```sh
+npm run test-services:start                           # starts services in Docker
+cd plugins/node/opentelemetry-instrumentation-mongodb # get into the instrumenation folder
+RUN_MONGODB_TESTS=1 npm test                          # run the test with the proper config (check each package)
+cd ../../..                                           # go back to root folder
+npm run test-services:stop                            # stops services in Docker
+```
+
+NOTE: scripts for each package will be added to avoid extra consumption of resources and improve the development experience.
+
 ### Benchmarks
 
 When two or more approaches must be compared, please write a benchmark in the benchmark/index.js module so that we can keep track of the most efficient algorithm.

package-lock.json

@@ -19,6 +19,9 @@
     "test:browser": "nx run-many -t test:browser",
     "test:ci:changed": "nx affected -t test --base=origin/main --head=HEAD",
     "test-all-versions": "nx run-many -t test-all-versions",
+    "test-services:start": "docker compose -f ./test/docker-compose.yaml up -d --wait",
+    "test-services:stop": "docker compose -f ./test/docker-compose.yaml down",
+    "test:with-services-env": "cross-env NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env npm test",
     "changelog": "lerna-changelog",
     "lint": "nx run-many -t lint && npm run lint:deps && npm run lint:readme && npm run lint:markdown && npm run lint:semconv-deps",
     "lint:fix": "nx run-many -t lint:fix && npm run lint:markdown:fix",
@@ -43,6 +46,7 @@
   "devDependencies": {
     "@typescript-eslint/eslint-plugin": "5.62.0",
     "@typescript-eslint/parser": "5.62.0",
+    "cross-env": "^7.0.3",
     "eslint": "8.7.0",
     "eslint-config-airbnb-base": "15.0.0",
     "eslint-config-prettier": "8.8.0",

package.json

@@ -92,64 +92,75 @@ export class DataloaderInstrumentation extends InstrumentationBase<DataloaderIns
     return `${MODULE_NAME}.${operation} ${dataloaderName}`;
   }
 
+  private _wrapBatchLoadFn(
+    batchLoadFn: Dataloader.BatchLoadFn<unknown, unknown>
+  ): Dataloader.BatchLoadFn<unknown, unknown> {
+    const instrumentation = this;
+
+    return function patchedBatchLoadFn(
+      this: DataloaderInternal,
+      ...args: Parameters<Dataloader.BatchLoadFn<unknown, unknown>>
+    ) {
+      if (
+        !instrumentation.isEnabled() ||
+        !instrumentation.shouldCreateSpans()
+      ) {
+        return batchLoadFn.call(this, ...args);
+      }
+
+      const parent = context.active();
+      const span = instrumentation.tracer.startSpan(
+        instrumentation.getSpanName(this, 'batch'),
+        { links: this._batch?.spanLinks as Link[] | undefined },
+        parent
+      );
+
+      return context.with(trace.setSpan(parent, span), () => {
+        return (batchLoadFn.apply(this, args) as Promise<unknown[]>)
+          .then(value => {
+            span.end();
+            return value;
+          })
+          .catch(err => {
+            span.recordException(err);
+            span.setStatus({
+              code: SpanStatusCode.ERROR,
+              message: err.message,
+            });
+            span.end();
+            throw err;
+          });
+      });
+    };
+  }
+
   private _getPatchedConstructor(
     constructor: typeof Dataloader
   ): typeof Dataloader {
-    const prototype = constructor.prototype;
     const instrumentation = this;
+    const prototype = constructor.prototype;
+
+    if (!instrumentation.isEnabled()) {
+      return constructor;
+    }
 
     function PatchedDataloader(
+      this: DataloaderInternal,
       ...args: ConstructorParameters<typeof constructor>
     ) {
-      const inst = new constructor(...args) as DataloaderInternal;
-
-      if (!instrumentation.isEnabled()) {
-        return inst;
-      }
+      // BatchLoadFn is the first constructor argument
+      // https://github.com/graphql/dataloader/blob/77c2cd7ca97e8795242018ebc212ce2487e729d2/src/index.js#L47
+      if (typeof args[0] === 'function') {
+        if (isWrapped(args[0])) {
+          instrumentation._unwrap(args, 0);
+        }
 
-      if (isWrapped(inst._batchLoadFn)) {
-        instrumentation._unwrap(inst, '_batchLoadFn');
+        args[0] = instrumentation._wrapBatchLoadFn(
+          args[0]
+        ) as Dataloader.BatchLoadFn<unknown, unknown>;
       }
 
-      instrumentation._wrap(inst, '_batchLoadFn', original => {
-        return function patchedBatchLoadFn(
-          this: DataloaderInternal,
-          ...args: Parameters<Dataloader.BatchLoadFn<unknown, unknown>>
-        ) {
-          if (
-            !instrumentation.isEnabled() ||
-            !instrumentation.shouldCreateSpans()
-          ) {
-            return original.call(this, ...args);
-          }
-
-          const parent = context.active();
-          const span = instrumentation.tracer.startSpan(
-            instrumentation.getSpanName(inst, 'batch'),
-            { links: this._batch?.spanLinks as Link[] | undefined },
-            parent
-          );
-
-          return context.with(trace.setSpan(parent, span), () => {
-            return (original.apply(this, args) as Promise<unknown[]>)
-              .then(value => {
-                span.end();
-                return value;
-              })
-              .catch(err => {
-                span.recordException(err);
-                span.setStatus({
-                  code: SpanStatusCode.ERROR,
-                  message: err.message,
-                });
-                span.end();
-                throw err;
-              });
-          });
-        };
-      });
-
-      return inst;
+      return constructor.apply(this, args);
     }
 
     PatchedDataloader.prototype = prototype;

plugins/node/instrumentation-dataloader/src/instrumentation.ts

@@ -619,4 +619,25 @@ describe('DataloaderInstrumentation', () => {
     assert.deepStrictEqual(await alternativeDataloader.loadMany(['test']), [1]);
     assert.strictEqual(memoryExporter.getFinishedSpans().length, 5);
   });
+
+  it('should not prune custom methods', async () => {
+    class CustomDataLoader extends Dataloader<string, string> {
+      constructor() {
+        super(async keys => keys.map((_, idx) => getMd5HashFromIdx(idx)));
+      }
+
+      public async customLoad() {
+        return this.load('test');
+      }
+    }
+
+    const customDataloader = new CustomDataLoader();
+    await customDataloader.customLoad();
+
+    assert.strictEqual(memoryExporter.getFinishedSpans().length, 2);
+    const [batchSpan, loadSpan] = memoryExporter.getFinishedSpans();
+
+    assert.strictEqual(loadSpan.name, 'dataloader.load');
+    assert.strictEqual(batchSpan.name, 'dataloader.batch');
+  });
 });

plugins/node/instrumentation-dataloader/test/dataloader.test.ts

@@ -24,7 +24,6 @@ import {
 } from '@opentelemetry/sdk-metrics';
 import * as assert from 'assert';
 import { MySQLInstrumentation } from '../src';
-import * as testUtils from '@opentelemetry/contrib-test-utils';
 import { registerInstrumentationTesting } from '@opentelemetry/contrib-test-utils';
 
 const instrumentation = registerInstrumentationTesting(
@@ -62,9 +61,9 @@ import * as mysqlTypes from 'mysql';
 describe('[email protected]', () => {
   let otelTestingMeterProvider;
   let inMemoryMetricsExporter: InMemoryMetricExporter;
-  const testMysql = process.env.RUN_MYSQL_TESTS; // For CI: assumes local mysql db is already available
-  const testMysqlLocally = process.env.RUN_MYSQL_TESTS_LOCAL; // For local: spins up local mysql db via docker
-  const shouldTest = testMysql || testMysqlLocally; // Skips these tests if false (default)
+  // assumes local mysql db is already available in CI or
+  // using `npm run test-services:start` script at the root folder
+  const shouldTest = process.env.RUN_MYSQL_TESTS;
 
   function initMeterProvider() {
     inMemoryMetricsExporter = new InMemoryMetricExporter(
@@ -90,22 +89,7 @@ describe('[email protected]', () => {
       console.log('Skipping test-mysql for metrics.');
       this.skip();
     }
-
-    if (testMysqlLocally) {
-      testUtils.startDocker('mysql');
-      // wait 15 seconds for docker container to start
-      this.timeout(20000);
-      setTimeout(done, 15000);
-    } else {
-      done();
-    }
-  });
-
-  after(function () {
-    if (testMysqlLocally) {
-      this.timeout(5000);
-      testUtils.cleanUpDocker('mysql');
-    }
+    done();
   });
 
   describe('#Pool - metrics', () => {

plugins/node/opentelemetry-instrumentation-mysql/test/index.metrics.test.ts

@@ -25,7 +25,6 @@ import {
   SEMATTRS_NET_PEER_NAME,
   SEMATTRS_NET_PEER_PORT,
 } from '@opentelemetry/semantic-conventions';
-import * as testUtils from '@opentelemetry/contrib-test-utils';
 import {
   BasicTracerProvider,
   InMemorySpanExporter,
@@ -54,9 +53,9 @@ describe('[email protected]', () => {
   let connection: mysqlTypes.Connection;
   let pool: mysqlTypes.Pool;
   let poolCluster: mysqlTypes.PoolCluster;
-  const testMysql = process.env.RUN_MYSQL_TESTS; // For CI: assumes local mysql db is already available
-  const testMysqlLocally = process.env.RUN_MYSQL_TESTS_LOCAL; // For local: spins up local mysql db via docker
-  const shouldTest = testMysql || testMysqlLocally; // Skips these tests if false (default)
+  // assumes local mysql db is already available in CI or
+  // using `npm run test-services:start` script at the root folder
+  const shouldTest = process.env.RUN_MYSQL_TESTS;
   const memoryExporter = new InMemorySpanExporter();
   const provider = new BasicTracerProvider({
     spanProcessors: [new SimpleSpanProcessor(memoryExporter)],
@@ -68,25 +67,11 @@ describe('[email protected]', () => {
       // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901
       this.test!.parent!.pending = true;
       this.skip();
-    }
-
-    if (testMysqlLocally) {
-      testUtils.startDocker('mysql');
-      // wait 15 seconds for docker container to start
-      this.timeout(20000);
-      setTimeout(done, 15000);
     } else {
       done();
     }
   });
 
-  after(function () {
-    if (testMysqlLocally) {
-      this.timeout(5000);
-      testUtils.cleanUpDocker('mysql');
-    }
-  });
-
   beforeEach(() => {
     instrumentation.disable();
     contextManager = new AsyncLocalStorageContextManager().enable();

plugins/node/opentelemetry-instrumentation-mysql/test/index.test.ts

@@ -64,28 +64,38 @@ interface Result extends RowDataPacket {
   solution: number;
 }
 
-describe('mysql2', () => {
-  const testMysql = process.env.RUN_MYSQL_TESTS; // For CI: assumes local mysql db is already available
-  const testMysqlLocally = process.env.RUN_MYSQL_TESTS_LOCAL; // For local: spins up local mysql db via docker
-  const shouldTest = testMysql || testMysqlLocally; // Skips these tests if false (default)
-
-  before(function (done) {
-    if (testMysqlLocally) {
-      testUtils.startDocker('mysql');
-      // wait 15 seconds for docker container to start
-      this.timeout(20000);
-      setTimeout(done, 15000);
-    } else {
-      done();
-    }
+// Helper function to setup the database
+const execPromise = (conn: Connection, command: string) => {
+  return new Promise<void>((res, rej) => {
+    conn.execute(command, err => {
+      if (err) rej(err);
+      else res();
+    });
   });
+};
 
-  after(function (done) {
-    if (testMysqlLocally) {
-      this.timeout(5000);
-      testUtils.cleanUpDocker('mysql');
+describe('mysql2', () => {
+  // assumes local mysql db is already available in CI or
+  // using `npm run test-services:start` script at the root folder
+  const shouldTest = process.env.RUN_MYSQL_TESTS;
+
+  before(async function () {
+    const connection = createConnection({
+      port,
+      user: 'root',
+      host,
+      password: rootPassword,
+      database,
+    });
+    try {
+      await execPromise(connection, "SET GLOBAL log_output='TABLE'");
+      await execPromise(connection, 'SET GLOBAL general_log = 1');
+    } catch (execErr) {
+      console.error('MySQL seup error: ', execErr);
+      this.skip();
+    } finally {
+      connection.end();
     }
-    done();
   });
 
   describe('callback API', () => {
@@ -358,7 +368,7 @@ describe('mysql2', () => {
       it('should not add comment by default', done => {
         const span = provider.getTracer('default').startSpan('test span');
         context.with(trace.setSpan(context.active(), span), () => {
-          connection.query('SELECT 1+1 as solution', () => {
+          connection.query('SELECT 1+1 as solution', (e, r) => {
             const spans = memoryExporter.getFinishedSpans();
             assert.strictEqual(spans.length, 1);
             getLastQueries(1).then(([query]) => {