ref(node): Streamline generic-pool

dev-packages/node-integration-tests/suites/tracing/genericPool-v2/instrument.mjs

@@ -0,0 +1,9 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+});

dev-packages/node-integration-tests/suites/tracing/genericPool-v2/scenario.mjs

@@ -0,0 +1,42 @@
+import * as Sentry from '@sentry/node';
+import genericPool from 'generic-pool';
+
+// generic-pool v2 uses the callback-based API: `new Pool(factory)` with `factory.create(callback)`
+// and `pool.acquire((err, client) => ...)`.
+const Pool = genericPool.Pool;
+
+const pool = new Pool({
+  name: 'test',
+  create: callback => callback(null, { id: Math.random() }),
+  destroy: () => {},
+  max: 10,
+  min: 2,
+});
+
+function acquire() {
+  return new Promise((resolve, reject) => {
+    pool.acquire((err, client) => {
+      if (err) {
+        reject(err);
+        return;
+      }
+      pool.release(client);
+      resolve();
+    });
+  });
+}
+
+async function run() {
+  await Sentry.startSpan(
+    {
+      op: 'transaction',
+      name: 'Test Transaction',
+    },
+    async () => {
+      await acquire();
+      await acquire();
+    },
+  );
+}
+
+run();

dev-packages/node-integration-tests/suites/tracing/genericPool-v2/test.ts

@@ -0,0 +1,43 @@
+import { afterAll, describe, expect } from 'vitest';
+import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner';
+
+describe('genericPool v2 auto instrumentation', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  createEsmAndCjsTests(
+    __dirname,
+    'scenario.mjs',
+    'instrument.mjs',
+    (createRunner, test) => {
+      test('should auto-instrument `generic-pool` v2 when calling pool.acquire()', async () => {
+        const EXPECTED_TRANSACTION = {
+          transaction: 'Test Transaction',
+          spans: expect.arrayContaining([
+            expect.objectContaining({
+              description: 'generic-pool.acquire',
+              origin: 'auto.db.otel.generic_pool',
+              data: {
+                'sentry.origin': 'auto.db.otel.generic_pool',
+              },
+              status: 'ok',
+            }),
+
+            expect.objectContaining({
+              description: 'generic-pool.acquire',
+              origin: 'auto.db.otel.generic_pool',
+              data: {
+                'sentry.origin': 'auto.db.otel.generic_pool',
+              },
+              status: 'ok',
+            }),
+          ]),
+        };
+
+        await createRunner().expect({ transaction: EXPECTED_TRANSACTION }).start().completed();
+      });
+    },
+    { additionalDependencies: { 'generic-pool': '^2.5.0' } },
+  );
+});

dev-packages/node-integration-tests/suites/tracing/genericPool/test.ts

@@ -12,7 +12,7 @@ describe('genericPool auto instrumentation', () => {
         transaction: 'Test Transaction',
         spans: expect.arrayContaining([
           expect.objectContaining({
-            description: expect.stringMatching(/^generic-pool\.ac?quire/),
+            description: 'generic-pool.acquire',
             origin: 'auto.db.otel.generic_pool',
             data: {
               'sentry.origin': 'auto.db.otel.generic_pool',
@@ -21,7 +21,7 @@ describe('genericPool auto instrumentation', () => {
           }),
 
           expect.objectContaining({
-            description: expect.stringMatching(/^generic-pool\.ac?quire/),
+            description: 'generic-pool.acquire',
             origin: 'auto.db.otel.generic_pool',
             data: {
               'sentry.origin': 'auto.db.otel.generic_pool',

packages/node/src/integrations/tracing/genericPool/index.ts

@@ -21,12 +21,8 @@ const _genericPoolIntegration = (() => {
       instrumentationWrappedCallback?.(() =>
         client.on('spanStart', span => {
           const spanJSON = spanToJSON(span);
-
           const spanDescription = spanJSON.description;
-
-          // typo in emitted span for version <= 0.38.0 of @opentelemetry/instrumentation-generic-pool
-          const isGenericPoolSpan =
-            spanDescription === 'generic-pool.aquire' || spanDescription === 'generic-pool.acquire';
+          const isGenericPoolSpan = spanDescription === 'generic-pool.acquire';
 
           if (isGenericPoolSpan) {
             span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.generic_pool');

packages/node/src/integrations/tracing/genericPool/vendored/instrumentation.ts

@@ -18,23 +18,24 @@
  * - Upstream version: @opentelemetry/instrumentation-generic-pool@0.61.0
  * - Minor TypeScript strictness adjustments for this repository's compiler settings
  */
-/* eslint-disable */
 
 import * as api from '@opentelemetry/api';
-import {
-  InstrumentationBase,
-  InstrumentationConfig,
-  InstrumentationNodeModuleDefinition,
-  isWrapped,
-} from '@opentelemetry/instrumentation';
-
-import type * as genericPool from './generic-pool-types';
-
+import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
+import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation';
 import { SDK_VERSION } from '@sentry/core';
+import type * as genericPool from './generic-pool-types';
 
 const MODULE_NAME = 'generic-pool';
 const PACKAGE_NAME = '@sentry/instrumentation-generic-pool';
 
+type AcquireFn = (this: unknown, ...args: unknown[]) => unknown;
+interface PoolConstructor {
+  prototype: { acquire: AcquireFn };
+}
+interface GenericPoolModule {
+  Pool: PoolConstructor;
+}
+
 export class GenericPoolInstrumentation extends InstrumentationBase {
   // only used for v2 - v2.3)
   private _isDisabled = false;
@@ -48,49 +49,49 @@ export class GenericPoolInstrumentation extends InstrumentationBase {
       new InstrumentationNodeModuleDefinition(
         MODULE_NAME,
         ['>=3.0.0 <4'],
-        moduleExports => {
-          const Pool: any = moduleExports.Pool;
+        (moduleExports: GenericPoolModule) => {
+          const Pool = moduleExports.Pool;
           if (isWrapped(Pool.prototype.acquire)) {
             this._unwrap(Pool.prototype, 'acquire');
           }
           this._wrap(Pool.prototype, 'acquire', this._acquirePatcher.bind(this));
           return moduleExports;
         },
-        moduleExports => {
-          const Pool: any = moduleExports.Pool;
+        (moduleExports: GenericPoolModule) => {
+          const Pool = moduleExports.Pool;
           this._unwrap(Pool.prototype, 'acquire');
           return moduleExports;
         },
       ),
       new InstrumentationNodeModuleDefinition(
         MODULE_NAME,
         ['>=2.4.0 <3'],
-        moduleExports => {
-          const Pool: any = moduleExports.Pool;
+        (moduleExports: GenericPoolModule) => {
+          const Pool = moduleExports.Pool;
           if (isWrapped(Pool.prototype.acquire)) {
             this._unwrap(Pool.prototype, 'acquire');
           }
           this._wrap(Pool.prototype, 'acquire', this._acquireWithCallbacksPatcher.bind(this));
           return moduleExports;
         },
-        moduleExports => {
-          const Pool: any = moduleExports.Pool;
+        (moduleExports: GenericPoolModule) => {
+          const Pool = moduleExports.Pool;
           this._unwrap(Pool.prototype, 'acquire');
           return moduleExports;
         },
       ),
       new InstrumentationNodeModuleDefinition(
         MODULE_NAME,
         ['>=2.0.0 <2.4'],
-        moduleExports => {
+        (moduleExports: GenericPoolModule) => {
           this._isDisabled = false;
           if (isWrapped(moduleExports.Pool)) {
             this._unwrap(moduleExports, 'Pool');
           }
           this._wrap(moduleExports, 'Pool', this._poolWrapper.bind(this));
           return moduleExports;
         },
-        moduleExports => {
+        (moduleExports: GenericPoolModule) => {
           // since the object is created on the fly every time, we need to use
           // a boolean switch here to disable the instrumentation
           this._isDisabled = true;
@@ -100,14 +101,15 @@ export class GenericPoolInstrumentation extends InstrumentationBase {
     ];
   }
 
-  private _acquirePatcher(original: genericPool.Pool<unknown>['acquire']) {
+  private _acquirePatcher(original: AcquireFn) {
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
     const instrumentation = this;
-    return function wrapped_acquire(this: genericPool.Pool<unknown>, ...args: any[]) {
+    return function wrapped_acquire(this: genericPool.Pool<unknown>, ...args: unknown[]) {
       const parent = api.context.active();
       const span = instrumentation.tracer.startSpan('generic-pool.acquire', {}, parent);
 
       return api.context.with(api.trace.setSpan(parent, span), () => {
-        return original.call(this, ...args).then(
+        return (original.call(this, ...args) as PromiseLike<unknown>).then(
           (value: unknown) => {
             span.end();
             return value;
@@ -122,18 +124,24 @@ export class GenericPoolInstrumentation extends InstrumentationBase {
     };
   }
 
-  private _poolWrapper(original: any) {
+  private _poolWrapper(original: (this: unknown, ...args: unknown[]) => { acquire: AcquireFn }) {
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
     const instrumentation = this;
-    return function wrapped_pool(this: any) {
-      const pool = original.apply(this, arguments);
+    return function wrapped_pool(this: unknown, ...args: unknown[]) {
+      const pool = original.apply(this, args);
       instrumentation._wrap(pool, 'acquire', instrumentation._acquireWithCallbacksPatcher.bind(instrumentation));
       return pool;
     };
   }
 
-  private _acquireWithCallbacksPatcher(original: any) {
+  private _acquireWithCallbacksPatcher(original: AcquireFn) {
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
     const instrumentation = this;
-    return function wrapped_acquire(this: genericPool.Pool<unknown>, cb: Function, priority: number) {
+    return function wrapped_acquire(
+      this: genericPool.Pool<unknown>,
+      cb: (err: unknown, client: unknown) => unknown,
+      priority: number,
+    ) {
       // only used for v2 - v2.3
       if (instrumentation._isDisabled) {
         return original.call(this, cb, priority);
@@ -148,8 +156,9 @@ export class GenericPoolInstrumentation extends InstrumentationBase {
             span.end();
             // Not checking whether cb is a function because
             // the original code doesn't do that either.
+            // The callback's return value is unused by generic-pool, so we don't return it.
             if (cb) {
-              return cb(err, client);
+              cb(err, client);
             }
           },
           priority,

packages/node/test/integrations/tracing/genericPool.test.ts

@@ -0,0 +1,68 @@
+/*
+ * Tests ported from @opentelemetry/instrumentation-generic-pool@0.61.0
+ * Original source: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/instrumentation-generic-pool
+ * Licensed under the Apache License, Version 2.0
+ */
+
+import { context, trace } from '@opentelemetry/api';
+import { BasicTracerProvider, InMemorySpanExporter, SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+import { GenericPoolInstrumentation } from '../../../src/integrations/tracing/genericPool/vendored/instrumentation';
+import { cleanupOtel, mockSdkInit } from '../../helpers/mockSdkInit';
+
+// Create a fake `generic-pool` module.
+function createPoolModule(): { Pool: new () => { acquire: () => Promise<string> } } {
+  class FakePool {
+    public acquire(): Promise<string> {
+      return Promise.resolve('client');
+    }
+  }
+  return { Pool: FakePool };
+}
+
+describe('GenericPoolInstrumentation', () => {
+  const memoryExporter = new InMemorySpanExporter();
+  const provider = new BasicTracerProvider({ spanProcessors: [new SimpleSpanProcessor(memoryExporter)] });
+
+  let instrumentation: GenericPoolInstrumentation;
+
+  beforeEach(() => {
+    mockSdkInit({ tracesSampleRate: 1 });
+    instrumentation = new GenericPoolInstrumentation();
+    instrumentation.setTracerProvider(provider);
+    memoryExporter.reset();
+  });
+
+  afterEach(() => {
+    instrumentation.disable();
+    cleanupOtel();
+  });
+
+  it('should attach it to the parent span', async () => {
+    const moduleExports = createPoolModule();
+    const def = instrumentation.getModuleDefinitions()[0]!;
+    def.patch!(moduleExports);
+
+    const parent = provider.getTracer('test').startSpan('parent');
+    await context.with(trace.setSpan(context.active(), parent), async () => {
+      await new moduleExports.Pool().acquire();
+    });
+    parent.end();
+
+    const acquireSpan = memoryExporter.getFinishedSpans().find(span => span.name === 'generic-pool.acquire');
+    expect(acquireSpan).toBeDefined();
+    expect(acquireSpan!.parentSpanContext?.spanId).toBe(parent.spanContext().spanId);
+  });
+
+  it('should not create anything if disabled', async () => {
+    const moduleExports = createPoolModule();
+    const def = instrumentation.getModuleDefinitions()[0]!;
+    // Patch then unpatch to mimic the instrumentation being disabled.
+    def.patch!(moduleExports);
+    def.unpatch!(moduleExports);
+
+    await new moduleExports.Pool().acquire();
+
+    expect(memoryExporter.getFinishedSpans()).toHaveLength(0);
+  });
+});