feat(instrumentation-ioredis): convert module version capturing to hook

plugins/node/opentelemetry-instrumentation-ioredis/README.md

@@ -49,9 +49,9 @@ IORedis instrumentation has few options available to choose from. You can set th
 | Options | Type | Description |
 | ------- | ---- | ----------- |
 | `dbStatementSerializer` | `DbStatementSerializer` | IORedis instrumentation will serialize db.statement using the specified function. |
+| `requestHook` | `RedisRequestCustomAttributeFunction` | Function for adding custom attributes on db request |
 | `responseHook` | `RedisResponseCustomAttributeFunction` | Function for adding custom attributes on db response |
 | `requireParentSpan` | `boolean` | Require parent to create ioredis span, default when unset is true |
-| `moduleVersionAttributeName` | `string` | If passed, a span attribute will be added to all spans with key of the provided "moduleVersionAttributeName" and value of the module version. |
 
 #### Custom db.statement Serializer
 The instrumentation serializes the whole command into a Span attribute called `db.statement`. The standard serialization format is `{cmdName} {cmdArgs.join(',')}`.

plugins/node/opentelemetry-instrumentation-ioredis/src/ioredis.ts

@@ -14,16 +14,16 @@
  * limitations under the License.
  */
 
-import { diag } from "@opentelemetry/api";
-import type * as ioredisTypes from "ioredis";
+import { diag } from '@opentelemetry/api';
+import type * as ioredisTypes from 'ioredis';
 import {
   InstrumentationBase,
   InstrumentationNodeModuleDefinition,
   isWrapped,
-} from "@opentelemetry/instrumentation";
-import { IORedisInstrumentationConfig } from "./types";
-import { traceConnection, traceSendCommand } from "./utils";
-import { VERSION } from "./version";
+} from '@opentelemetry/instrumentation';
+import { IORedisInstrumentationConfig } from './types';
+import { traceConnection, traceSendCommand } from './utils';
+import { VERSION } from './version';
 
 const DEFAULT_CONFIG: IORedisInstrumentationConfig = {
   requireParentSpan: true,
@@ -32,11 +32,11 @@ const DEFAULT_CONFIG: IORedisInstrumentationConfig = {
 export class IORedisInstrumentation extends InstrumentationBase<
   typeof ioredisTypes
 > {
-  static readonly DB_SYSTEM = "redis";
+  static readonly DB_SYSTEM = 'redis';
 
   constructor(_config: IORedisInstrumentationConfig = {}) {
     super(
-      "@opentelemetry/instrumentation-ioredis",
+      '@opentelemetry/instrumentation-ioredis',
       VERSION,
       Object.assign({}, DEFAULT_CONFIG, _config)
     );
@@ -45,33 +45,33 @@ export class IORedisInstrumentation extends InstrumentationBase<
   init(): InstrumentationNodeModuleDefinition<typeof ioredisTypes>[] {
     return [
       new InstrumentationNodeModuleDefinition<typeof ioredisTypes>(
-        "ioredis",
-        [">1 <5"],
+        'ioredis',
+        ['>1 <5'],
         (moduleExports, moduleVersion?: string | undefined) => {
-          diag.debug("Applying patch for ioredis");
+          diag.debug('Applying patch for ioredis');
           if (isWrapped(moduleExports.prototype.sendCommand)) {
-            this._unwrap(moduleExports.prototype, "sendCommand");
+            this._unwrap(moduleExports.prototype, 'sendCommand');
           }
           this._wrap(
             moduleExports.prototype,
-            "sendCommand",
+            'sendCommand',
             this._patchSendCommand(moduleVersion)
           );
           if (isWrapped(moduleExports.prototype.connect)) {
-            this._unwrap(moduleExports.prototype, "connect");
+            this._unwrap(moduleExports.prototype, 'connect');
           }
           this._wrap(
             moduleExports.prototype,
-            "connect",
+            'connect',
             this._patchConnection()
           );
           return moduleExports;
         },
-        (moduleExports) => {
+        moduleExports => {
           if (moduleExports === undefined) return;
-          diag.debug("Removing patch for ioredis");
-          this._unwrap(moduleExports.prototype, "sendCommand");
-          this._unwrap(moduleExports.prototype, "connect");
+          diag.debug('Removing patch for ioredis');
+          this._unwrap(moduleExports.prototype, 'sendCommand');
+          this._unwrap(moduleExports.prototype, 'connect');
         }
       ),
     ];

plugins/node/opentelemetry-instrumentation-ioredis/src/types.ts

@@ -40,6 +40,16 @@ export type DbStatementSerializer = (
   cmdArgs: IORedisCommand['args']
 ) => string;
 
+export interface IoRedisRequestHookInformation {
+  moduleVersion?: string;
+  cmdName: IORedisCommand['name'];
+  cmdArgs: IORedisCommand['args'];
+}
+
+export interface RedisRequestCustomAttributeFunction {
+  (span: Span, requestInfo: IoRedisRequestHookInformation): void;
+}
+
 /**
  * Function that can be used to add custom attributes to span on response from redis server
  * @param span - The span created for the redis command, on which attributes can be set
@@ -66,15 +76,12 @@ export interface IORedisInstrumentationConfig extends InstrumentationConfig {
   /** Custom serializer function for the db.statement tag */
   dbStatementSerializer?: DbStatementSerializer;
 
+  /** Function for adding custom attributes on db request */
+  requestHook?: RedisRequestCustomAttributeFunction;
+
   /** Function for adding custom attributes on db response */
   responseHook?: RedisResponseCustomAttributeFunction;
 
   /** Require parent to create ioredis span, default when unset is true */
   requireParentSpan?: boolean;
-
-  /**
-   * If passed, a span attribute will be added to all spans with key of the provided "moduleVersionAttributeName"
-   * and value of the module version.
-   */
-  moduleVersionAttributeName?: string;
 }

plugins/node/opentelemetry-instrumentation-ioredis/src/utils.ts

@@ -108,8 +108,21 @@ export const traceSendCommand = (
       },
     });
 
-    if (config?.moduleVersionAttributeName && moduleVersion) {
-      span.setAttribute(config?.moduleVersionAttributeName, moduleVersion);
+    if (config?.requestHook) {
+      safeExecuteInTheMiddle(
+        () =>
+          config?.requestHook!(span, {
+            moduleVersion,
+            cmdName: cmd.name,
+            cmdArgs: cmd.args,
+          }),
+        e => {
+          if (e) {
+            diag.error('ioredis instrumentation: request hook failed', e);
+          }
+        },
+        true
+      );
     }
 
     const { host, port } = this.options;
@@ -130,7 +143,7 @@ export const traceSendCommand = (
           () => config?.responseHook?.(span, cmd.name, cmd.args, result),
           e => {
             if (e) {
-              diag.error('ioredis response hook failed', e);
+              diag.error('ioredis instrumentation: response hook failed', e);
             }
           },
           true

plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts

@@ -37,6 +37,7 @@ import { IORedisInstrumentation } from '../src';
 import {
   IORedisInstrumentationConfig,
   DbStatementSerializer,
+  IoRedisRequestHookInformation,
 } from '../src/types';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 
@@ -769,29 +770,73 @@ describe('ioredis', () => {
       });
     });
 
-    it('moduleVersionAttributeName should capture ioredis version', async () => {
-      const VERSION_ATTR = 'instrumentedmodule.version';
-      instrumentation.disable();
-      const config: IORedisInstrumentationConfig = {
-        moduleVersionAttributeName: VERSION_ATTR,
-      };
-      instrumentation = new IORedisInstrumentation(config);
-      instrumentation.setTracerProvider(provider);
-      require('ioredis');
+    describe('Instrumenting with a custom hooks', () => {
+      it('should call requestHook when set in config', async () => {
+        instrumentation.disable();
+        const config: IORedisInstrumentationConfig = {
+          requestHook: (
+            span: Span,
+            requestInfo: IoRedisRequestHookInformation
+          ) => {
+            assert.match(
+              requestInfo.moduleVersion as string,
+              /\d{1,4}\.\d{1,4}\.\d{1,5}.*/
+            );
+            assert.strictEqual(requestInfo.cmdName, 'incr');
+            assert.deepStrictEqual(requestInfo.cmdArgs, ['request-hook-test']);
 
-      const span = provider.getTracer('ioredis-test').startSpan('test span');
-      await context.with(setSpan(context.active(), span), async () => {
-        await client.set(testKeyName, 'data');
-        const endedSpans = memoryExporter.getFinishedSpans();
-        assert.strictEqual(endedSpans.length, 1);
-        assert.match(
-          endedSpans[0].attributes[VERSION_ATTR] as string,
-          /\d{1,4}\.\d{1,4}\.\d{1,5}.*/
-        );
+            span.setAttribute(
+              'attribute key from request hook',
+              'custom value from request hook'
+            );
+          },
+        };
+        instrumentation = new IORedisInstrumentation(config);
+        instrumentation.setTracerProvider(provider);
+        require('ioredis');
+
+        const span = provider.getTracer('ioredis-test').startSpan('test span');
+        await context.with(setSpan(context.active(), span), async () => {
+          await client.incr('request-hook-test');
+          const endedSpans = memoryExporter.getFinishedSpans();
+          assert.strictEqual(endedSpans.length, 1);
+          assert.strictEqual(
+            endedSpans[0].attributes['attribute key from request hook'],
+            'custom value from request hook'
+          );
+        });
+      });
+
+      it('should ignore requestHook which throws exception', async () => {
+        instrumentation.disable();
+        const config: IORedisInstrumentationConfig = {
+          requestHook: (
+            span: Span,
+            _requestInfo: IoRedisRequestHookInformation
+          ) => {
+            span.setAttribute(
+              'attribute key BEFORE exception',
+              'this attribute is added to span BEFORE exception is thrown thus we can expect it'
+            );
+            throw Error('error thrown in requestHook');
+          },
+        };
+        instrumentation = new IORedisInstrumentation(config);
+        instrumentation.setTracerProvider(provider);
+        require('ioredis');
+
+        const span = provider.getTracer('ioredis-test').startSpan('test span');
+        await context.with(setSpan(context.active(), span), async () => {
+          await client.incr('request-hook-throw-test');
+          const endedSpans = memoryExporter.getFinishedSpans();
+          assert.strictEqual(endedSpans.length, 1);
+          assert.strictEqual(
+            endedSpans[0].attributes['attribute key BEFORE exception'],
+            'this attribute is added to span BEFORE exception is thrown thus we can expect it'
+          );
+        });
       });
-    });
 
-    describe('Instrumenting with a custom responseHook', () => {
       it('should call responseHook when set in config', async () => {
         instrumentation.disable();
         const config: IORedisInstrumentationConfig = {
@@ -801,13 +846,17 @@ describe('ioredis', () => {
             _cmdArgs: Array<string | Buffer | number>,
             response: unknown
           ) => {
-            assert.strictEqual(cmdName, 'incr');
-            // the command is 'incr' on a key which does not exist, thus it increase 0 by 1 and respond 1
-            assert.strictEqual(response, 1);
-            span.setAttribute(
-              'attribute key from hook',
-              'custom value from hook'
-            );
+            try {
+              assert.strictEqual(cmdName, 'incr');
+              // the command is 'incr' on a key which does not exist, thus it increase 0 by 1 and respond 1
+              assert.strictEqual(response, 1);
+              span.setAttribute(
+                'attribute key from hook',
+                'custom value from hook'
+              );
+            } catch (err) {
+              console.log(err);
+            }
           },
         };
         instrumentation = new IORedisInstrumentation(config);
@@ -816,7 +865,7 @@ describe('ioredis', () => {
 
         const span = provider.getTracer('ioredis-test').startSpan('test span');
         await context.with(setSpan(context.active(), span), async () => {
-          await client.incr('new-key');
+          await client.incr('response-hook-test');
           const endedSpans = memoryExporter.getFinishedSpans();
           assert.strictEqual(endedSpans.length, 1);
           assert.strictEqual(
@@ -844,7 +893,7 @@ describe('ioredis', () => {
 
         const span = provider.getTracer('ioredis-test').startSpan('test span');
         await context.with(setSpan(context.active(), span), async () => {
-          await client.incr('some-key');
+          await client.incr('response-hook-throw-test');
           const endedSpans = memoryExporter.getFinishedSpans();
 
           // hook throw exception, but span should not be affected