From 8d9d9aa65ee60f658e9b1cdcc475ec456c6b9111 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Mon, 26 Apr 2021 09:29:16 +0300 Subject: [PATCH 01/10] feat: add responseHook config to redis instrumentation --- .../src/instrumentation.ts | 11 +- .../src/types.ts | 42 +++++- .../src/utils.ts | 113 +++++++++------- .../test/redis.test.ts | 126 ++++++++++++++++++ 4 files changed, 245 insertions(+), 47 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts index 9cf437eecb..f480ffb713 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts @@ -29,6 +29,10 @@ import { import { RedisInstrumentationConfig } from './types'; import { VERSION } from './version'; +const DEFAULT_CONFIG: RedisInstrumentationConfig = { + requireParentSpan: false +}; + export class RedisInstrumentation extends InstrumentationBase< typeof redisTypes > { @@ -38,6 +42,10 @@ export class RedisInstrumentation extends InstrumentationBase< super('@opentelemetry/instrumentation-redis', VERSION, _config); } + setConfig(config: RedisInstrumentationConfig = {}) { + this._config = Object.assign({}, DEFAULT_CONFIG, config); + } + protected init() { return [ new InstrumentationNodeModuleDefinition( @@ -100,8 +108,9 @@ export class RedisInstrumentation extends InstrumentationBase< */ private _getPatchInternalSendCommand() { const tracer = this.tracer; + const config = this._config; return function internal_send_command(original: Function) { - return getTracedInternalSendCommand(tracer, original); + return getTracedInternalSendCommand(tracer, original, config); }; } diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/types.ts b/plugins/node/opentelemetry-instrumentation-redis/src/types.ts index 4465c0e131..6eb6164523 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/types.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Span } from '@opentelemetry/api' import { InstrumentationConfig } from '@opentelemetry/instrumentation'; import type * as redisTypes from 'redis'; @@ -36,4 +37,43 @@ export interface RedisPluginClientTypes { address?: string; } -export type RedisInstrumentationConfig = InstrumentationConfig; +/** + * Function that can be used to serialize db.statement tag + * @param cmdName - The name of the command (eg. set, get, mset) + * @param cmdArgs - Array of arguments passed to the command + * + * @returns serialized string that will be used as the db.statement attribute. + */ + export type DbStatementSerializer = ( + cmdName: RedisCommand['command'], + cmdArgs: RedisCommand['args'] +) => string; + +/** + * 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 + * @param cmdName - The name of the command (eg. set, get, mset) + * @param cmdArgs - Array of arguments passed to the command + * @param response - The response object which is returned to the user who called this command. + * Can be used to set custom attributes on the span. + * The type of the response varies depending on the specific command. + */ +export interface RedisResponseCustomAttributeFunction { + ( + span: Span, + cmdName: RedisCommand['command'], + cmdArgs: RedisCommand['args'], + response: unknown + ): void; +} + +export interface RedisInstrumentationConfig extends InstrumentationConfig { + /** Custom serializer function for the db.statement tag */ + dbStatementSerializer?: DbStatementSerializer; + + /** Function for adding custom attributes on db response */ + responseHook?: RedisResponseCustomAttributeFunction; + + /** Require parent to create redis span, default when unset is false */ + requireParentSpan?: boolean; +} diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts index f6eb60cc01..4e08ca6796 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts @@ -21,11 +21,14 @@ import { SpanKind, Span, SpanStatusCode, + getSpan, + diag, } from '@opentelemetry/api'; -import { RedisCommand, RedisPluginClientTypes } from './types'; +import { DbStatementSerializer, RedisCommand, RedisInstrumentationConfig, RedisPluginClientTypes } from './types'; import { EventEmitter } from 'events'; import { RedisInstrumentation } from './'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; +import { safeExecuteInTheMiddle } from '@opentelemetry/instrumentation'; const endSpan = (span: Span, err?: Error | null) => { if (err) { @@ -64,9 +67,12 @@ export const getTracedCreateStreamTrace = ( }; }; +const defaultDbStatementSerializer: DbStatementSerializer = (cmdName, _cmdArgs) => cmdName + export const getTracedInternalSendCommand = ( tracer: Tracer, - original: Function + original: Function, + config?: RedisInstrumentationConfig ) => { return function internal_send_command_trace( this: RedisPluginClientTypes, @@ -74,53 +80,70 @@ export const getTracedInternalSendCommand = ( ) { // New versions of redis (2.4+) use a single options object // instead of named arguments - if (arguments.length === 1 && typeof cmd === 'object') { - const span = tracer.startSpan( - `${RedisInstrumentation.COMPONENT}-${cmd.command}`, - { - kind: SpanKind.CLIENT, - attributes: { - [SemanticAttributes.DB_SYSTEM]: RedisInstrumentation.COMPONENT, - [SemanticAttributes.DB_STATEMENT]: cmd.command, - }, - } - ); + if (arguments.length !== 1 || typeof cmd !== 'object') { + // We don't know how to trace this call, so don't start/stop a span + return original.apply(this, arguments); + } - // Set attributes for not explicitly typed RedisPluginClientTypes - if (this.options) { - span.setAttributes({ - [SemanticAttributes.NET_PEER_NAME]: this.options.host, - [SemanticAttributes.NET_PEER_PORT]: this.options.port, - }); - } - if (this.address) { - span.setAttribute( - SemanticAttributes.NET_PEER_IP, - `redis://${this.address}` - ); - } + const hasNoParentSpan = getSpan(context.active()) === undefined; + if (config?.requireParentSpan === true && hasNoParentSpan) { + return original.apply(this, arguments); + } - const originalCallback = arguments[0].callback; - if (originalCallback) { - (arguments[0] as RedisCommand).callback = function callback( - this: unknown, - err: Error | null, - _reply: T - ) { - endSpan(span, err); - return originalCallback.apply(this, arguments); - }; - } - try { - // Span will be ended in callback - return original.apply(this, arguments); - } catch (rethrow) { - endSpan(span, rethrow); - throw rethrow; // rethrow after ending span + const dbStatementSerializer = config?.dbStatementSerializer || defaultDbStatementSerializer; + const span = tracer.startSpan( + `${RedisInstrumentation.COMPONENT}-${cmd.command}`, + { + kind: SpanKind.CLIENT, + attributes: { + [SemanticAttributes.DB_SYSTEM]: RedisInstrumentation.COMPONENT, + [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer(cmd.command, cmd.args), + }, } + ); + + // Set attributes for not explicitly typed RedisPluginClientTypes + if (this.options) { + span.setAttributes({ + [SemanticAttributes.NET_PEER_NAME]: this.options.host, + [SemanticAttributes.NET_PEER_PORT]: this.options.port, + }); + } + if (this.address) { + span.setAttribute( + SemanticAttributes.NET_PEER_IP, + `redis://${this.address}` + ); } - // We don't know how to trace this call, so don't start/stop a span - return original.apply(this, arguments); + const originalCallback = arguments[0].callback; + if (originalCallback) { + (arguments[0] as RedisCommand).callback = function callback( + this: unknown, + err: Error | null, + reply: T + ) { + if (config?.responseHook) { + const responseHook = config.responseHook; + safeExecuteInTheMiddle(() => { + responseHook(span, cmd.command, cmd.args, reply) + }, + (err) => { + diag.error('Error executing responseHook', err) + }, + true) + } + + endSpan(span, err); + return originalCallback.apply(this, arguments); + }; + } + try { + // Span will be ended in callback + return original.apply(this, arguments); + } catch (rethrow) { + endSpan(span, rethrow); + throw rethrow; // rethrow after ending span + } }; }; diff --git a/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts b/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts index 4013f9213c..769f293d96 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts @@ -21,6 +21,7 @@ import { SpanStatus, getSpan, setSpan, + Span, } from '@opentelemetry/api'; import { NodeTracerProvider } from '@opentelemetry/node'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; @@ -38,6 +39,7 @@ instrumentation.enable(); instrumentation.disable(); import * as redisTypes from 'redis'; +import { RedisResponseCustomAttributeFunction } from '../src/types'; const memoryExporter = new InMemorySpanExporter(); @@ -128,23 +130,27 @@ describe('redis@2.x', () => { const REDIS_OPERATIONS: Array<{ description: string; command: string; + args: string[]; method: (cb: redisTypes.Callback) => unknown; }> = [ { description: 'insert', command: 'hset', + args: ['hash', 'random', 'random'], method: (cb: redisTypes.Callback) => client.hset('hash', 'random', 'random', cb), }, { description: 'get', command: 'get', + args: ['test'], method: (cb: redisTypes.Callback) => client.get('test', cb), }, { description: 'delete', command: 'del', + args: ['test'], method: (cb: redisTypes.Callback) => client.del('test', cb), }, ]; @@ -231,5 +237,125 @@ describe('redis@2.x', () => { }); }); }); + + describe('dbStatementSerializer config', () => { + const dbStatementSerializer = (cmdName: string, cmdArgs: string[]) => { + return Array.isArray(cmdArgs) && cmdArgs.length ? `${cmdName} ${cmdArgs.join(' ')}` : cmdName; + }; + + before(() => { + instrumentation.disable(); + instrumentation.setConfig({ dbStatementSerializer }) + instrumentation.enable(); + }); + + REDIS_OPERATIONS.forEach(operation => { + it(`should properly execute the db statement serializer for operation ${operation.description}`, (done) => { + const span = tracer.startSpan('test span'); + context.with(setSpan(context.active(), span), () => { + operation.method((err, _) => { + assert.ifError(err); + span.end(); + const endedSpans = memoryExporter.getFinishedSpans(); + assert.strictEqual(endedSpans.length, 2); + const expectedStatement = dbStatementSerializer(operation.command, operation.args); + assert.strictEqual(endedSpans[0].attributes[SemanticAttributes.DB_STATEMENT], expectedStatement); + done(); + }); + }); + }); + }) + }); + + describe('responseHook config', () => { + describe('valid responseHook', () => { + const dataFieldName = 'redis.data'; + + const responseHook: RedisResponseCustomAttributeFunction = ( + span: Span, + _cmdName: string, + _cmdArgs: string[], + response: unknown + ) => { + span.setAttribute(dataFieldName, new String(response).toString()); + } + + before(() => { + instrumentation.disable(); + instrumentation.setConfig({ responseHook }) + instrumentation.enable(); + }); + + REDIS_OPERATIONS.forEach(operation => { + it(`should apply responseHook for operation ${operation.description}`, (done) => { + operation.method((err, reply) => { + assert.ifError(err); + const endedSpans = memoryExporter.getFinishedSpans(); + assert.strictEqual(endedSpans[0].attributes[dataFieldName], new String(reply).toString()); + done(); + }); + }); + }); + }) + + describe('invalid responseHook', () => { + const badResponseHook: RedisResponseCustomAttributeFunction = ( + _span: Span, + _cmdName: string, + _cmdArgs: string[], + _response: unknown + ) => { + throw 'Some kind of error'; + } + + before(() => { + instrumentation.disable(); + instrumentation.setConfig({ responseHook: badResponseHook }) + instrumentation.enable(); + }); + + REDIS_OPERATIONS.forEach(operation => { + it(`should not fail because of responseHook error for operation ${operation.description}`, (done) => { + operation.method((err, _reply) => { + assert.ifError(err); + const endedSpans = memoryExporter.getFinishedSpans(); + assert.strictEqual(endedSpans.length, 1); + done(); + }); + }); + }); + }) + }); + + describe('requireParentSpan config', () => { + before(() => { + instrumentation.disable(); + instrumentation.setConfig({ requireParentSpan: true }) + instrumentation.enable(); + }); + + REDIS_OPERATIONS.forEach(operation => { + it(`should not create span without parent span for operation ${operation.description}`, (done) => { + operation.method((err, _) => { + assert.ifError(err); + const endedSpans = memoryExporter.getFinishedSpans(); + assert.strictEqual(endedSpans.length, 0); + done(); + }); + }); + + it(`should create span when a parent span exists for operation ${operation.description}`, (done) => { + const span = tracer.startSpan('test span'); + context.with(setSpan(context.active(), span), () => { + operation.method((err, _) => { + assert.ifError(err); + const endedSpans = memoryExporter.getFinishedSpans(); + assert.strictEqual(endedSpans.length, 1); + done(); + }); + }); + }); + }) + }); }); }); From 314f20372d3e39e09f547846f3430e21466f4df0 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Wed, 12 May 2021 07:56:33 +0300 Subject: [PATCH 02/10] fix: lint fix --- .../src/instrumentation.ts | 2 +- .../src/types.ts | 4 +- .../src/utils.ts | 33 +++++++---- .../test/redis.test.ts | 59 +++++++++++-------- 4 files changed, 60 insertions(+), 38 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts index f480ffb713..17e3adeec4 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/instrumentation.ts @@ -30,7 +30,7 @@ import { RedisInstrumentationConfig } from './types'; import { VERSION } from './version'; const DEFAULT_CONFIG: RedisInstrumentationConfig = { - requireParentSpan: false + requireParentSpan: false, }; export class RedisInstrumentation extends InstrumentationBase< diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/types.ts b/plugins/node/opentelemetry-instrumentation-redis/src/types.ts index 6eb6164523..245e7611e8 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Span } from '@opentelemetry/api' +import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; import type * as redisTypes from 'redis'; @@ -44,7 +44,7 @@ export interface RedisPluginClientTypes { * * @returns serialized string that will be used as the db.statement attribute. */ - export type DbStatementSerializer = ( +export type DbStatementSerializer = ( cmdName: RedisCommand['command'], cmdArgs: RedisCommand['args'] ) => string; diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts index 4e08ca6796..3bbf92bd5a 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts @@ -24,7 +24,12 @@ import { getSpan, diag, } from '@opentelemetry/api'; -import { DbStatementSerializer, RedisCommand, RedisInstrumentationConfig, RedisPluginClientTypes } from './types'; +import { + DbStatementSerializer, + RedisCommand, + RedisInstrumentationConfig, + RedisPluginClientTypes, +} from './types'; import { EventEmitter } from 'events'; import { RedisInstrumentation } from './'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; @@ -67,7 +72,7 @@ export const getTracedCreateStreamTrace = ( }; }; -const defaultDbStatementSerializer: DbStatementSerializer = (cmdName, _cmdArgs) => cmdName +const defaultDbStatementSerializer: DbStatementSerializer = cmdName => cmdName; export const getTracedInternalSendCommand = ( tracer: Tracer, @@ -90,14 +95,18 @@ export const getTracedInternalSendCommand = ( return original.apply(this, arguments); } - const dbStatementSerializer = config?.dbStatementSerializer || defaultDbStatementSerializer; + const dbStatementSerializer = + config?.dbStatementSerializer || defaultDbStatementSerializer; const span = tracer.startSpan( `${RedisInstrumentation.COMPONENT}-${cmd.command}`, { kind: SpanKind.CLIENT, attributes: { [SemanticAttributes.DB_SYSTEM]: RedisInstrumentation.COMPONENT, - [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer(cmd.command, cmd.args), + [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer( + cmd.command, + cmd.args + ), }, } ); @@ -125,13 +134,15 @@ export const getTracedInternalSendCommand = ( ) { if (config?.responseHook) { const responseHook = config.responseHook; - safeExecuteInTheMiddle(() => { - responseHook(span, cmd.command, cmd.args, reply) - }, - (err) => { - diag.error('Error executing responseHook', err) - }, - true) + safeExecuteInTheMiddle( + () => { + responseHook(span, cmd.command, cmd.args, reply); + }, + err => { + diag.error('Error executing responseHook', err); + }, + true + ); } endSpan(span, err); diff --git a/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts b/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts index 769f293d96..7d91068f47 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/test/redis.test.ts @@ -240,17 +240,19 @@ describe('redis@2.x', () => { describe('dbStatementSerializer config', () => { const dbStatementSerializer = (cmdName: string, cmdArgs: string[]) => { - return Array.isArray(cmdArgs) && cmdArgs.length ? `${cmdName} ${cmdArgs.join(' ')}` : cmdName; + return Array.isArray(cmdArgs) && cmdArgs.length + ? `${cmdName} ${cmdArgs.join(' ')}` + : cmdName; }; before(() => { instrumentation.disable(); - instrumentation.setConfig({ dbStatementSerializer }) + instrumentation.setConfig({ dbStatementSerializer }); instrumentation.enable(); }); REDIS_OPERATIONS.forEach(operation => { - it(`should properly execute the db statement serializer for operation ${operation.description}`, (done) => { + it(`should properly execute the db statement serializer for operation ${operation.description}`, done => { const span = tracer.startSpan('test span'); context.with(setSpan(context.active(), span), () => { operation.method((err, _) => { @@ -258,13 +260,19 @@ describe('redis@2.x', () => { span.end(); const endedSpans = memoryExporter.getFinishedSpans(); assert.strictEqual(endedSpans.length, 2); - const expectedStatement = dbStatementSerializer(operation.command, operation.args); - assert.strictEqual(endedSpans[0].attributes[SemanticAttributes.DB_STATEMENT], expectedStatement); + const expectedStatement = dbStatementSerializer( + operation.command, + operation.args + ); + assert.strictEqual( + endedSpans[0].attributes[SemanticAttributes.DB_STATEMENT], + expectedStatement + ); done(); }); - }); + }); }); - }) + }); }); describe('responseHook config', () => { @@ -278,25 +286,28 @@ describe('redis@2.x', () => { response: unknown ) => { span.setAttribute(dataFieldName, new String(response).toString()); - } + }; before(() => { instrumentation.disable(); - instrumentation.setConfig({ responseHook }) + instrumentation.setConfig({ responseHook }); instrumentation.enable(); }); REDIS_OPERATIONS.forEach(operation => { - it(`should apply responseHook for operation ${operation.description}`, (done) => { + it(`should apply responseHook for operation ${operation.description}`, done => { operation.method((err, reply) => { assert.ifError(err); const endedSpans = memoryExporter.getFinishedSpans(); - assert.strictEqual(endedSpans[0].attributes[dataFieldName], new String(reply).toString()); + assert.strictEqual( + endedSpans[0].attributes[dataFieldName], + new String(reply).toString() + ); done(); - }); + }); }); }); - }) + }); describe('invalid responseHook', () => { const badResponseHook: RedisResponseCustomAttributeFunction = ( @@ -306,45 +317,45 @@ describe('redis@2.x', () => { _response: unknown ) => { throw 'Some kind of error'; - } + }; before(() => { instrumentation.disable(); - instrumentation.setConfig({ responseHook: badResponseHook }) + instrumentation.setConfig({ responseHook: badResponseHook }); instrumentation.enable(); }); REDIS_OPERATIONS.forEach(operation => { - it(`should not fail because of responseHook error for operation ${operation.description}`, (done) => { + it(`should not fail because of responseHook error for operation ${operation.description}`, done => { operation.method((err, _reply) => { assert.ifError(err); const endedSpans = memoryExporter.getFinishedSpans(); assert.strictEqual(endedSpans.length, 1); done(); - }); + }); }); }); - }) + }); }); describe('requireParentSpan config', () => { before(() => { instrumentation.disable(); - instrumentation.setConfig({ requireParentSpan: true }) + instrumentation.setConfig({ requireParentSpan: true }); instrumentation.enable(); }); REDIS_OPERATIONS.forEach(operation => { - it(`should not create span without parent span for operation ${operation.description}`, (done) => { + it(`should not create span without parent span for operation ${operation.description}`, done => { operation.method((err, _) => { assert.ifError(err); const endedSpans = memoryExporter.getFinishedSpans(); assert.strictEqual(endedSpans.length, 0); done(); - }); + }); }); - it(`should create span when a parent span exists for operation ${operation.description}`, (done) => { + it(`should create span when a parent span exists for operation ${operation.description}`, done => { const span = tracer.startSpan('test span'); context.with(setSpan(context.active(), span), () => { operation.method((err, _) => { @@ -352,10 +363,10 @@ describe('redis@2.x', () => { const endedSpans = memoryExporter.getFinishedSpans(); assert.strictEqual(endedSpans.length, 1); done(); - }); + }); }); }); - }) + }); }); }); }); From 8c36085244be30038d842b31314a69679bd4aa2f Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Mon, 26 Apr 2021 09:29:16 +0300 Subject: [PATCH 03/10] feat: add responseHook config to redis instrumentation --- .../node/opentelemetry-instrumentation-redis/src/utils.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts index 3bbf92bd5a..9a01db67f7 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts @@ -24,12 +24,7 @@ import { getSpan, diag, } from '@opentelemetry/api'; -import { - DbStatementSerializer, - RedisCommand, - RedisInstrumentationConfig, - RedisPluginClientTypes, -} from './types'; +import { DbStatementSerializer, RedisCommand, RedisInstrumentationConfig, RedisPluginClientTypes } from './types'; import { EventEmitter } from 'events'; import { RedisInstrumentation } from './'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; From 3b2288b58e0cf624c2d07f614c9245f42a5e7f5c Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Wed, 12 May 2021 07:56:33 +0300 Subject: [PATCH 04/10] fix: lint fix --- .../node/opentelemetry-instrumentation-redis/src/utils.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts index 9a01db67f7..3bbf92bd5a 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/utils.ts @@ -24,7 +24,12 @@ import { getSpan, diag, } from '@opentelemetry/api'; -import { DbStatementSerializer, RedisCommand, RedisInstrumentationConfig, RedisPluginClientTypes } from './types'; +import { + DbStatementSerializer, + RedisCommand, + RedisInstrumentationConfig, + RedisPluginClientTypes, +} from './types'; import { EventEmitter } from 'events'; import { RedisInstrumentation } from './'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; From d4925bc5a98f273d9897c90cf9d1d3247cabb277 Mon Sep 17 00:00:00 2001 From: WhiteSource Renovate Date: Fri, 14 May 2021 17:54:30 +0200 Subject: [PATCH 05/10] chore(deps): update all non-major dependencies (#483) From 92979f723dc48606c116f242ab8ef44b6db4b7b5 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Mon, 17 May 2021 23:22:22 +0300 Subject: [PATCH 06/10] chore: generalize the instrumentation file name (#479) From adcc0a5071797e9e30706205dfe360a2471d7764 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Thu, 22 Apr 2021 11:14:15 +0300 Subject: [PATCH 07/10] feat: add responseHook config to graphql instrumentation --- .../src/instrumentation.ts | 44 ++++++++++++++- .../src/types.ts | 12 +++++ .../test/graphql.test.ts | 53 +++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts index 043e9997c9..b5b1fd7094 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts @@ -229,8 +229,8 @@ export class GraphQLInstrumentation extends InstrumentationBase { processedArgs, ]); }, - err => { - endSpan(span, err); + (err, result) => { + instrumentation.handleExecutionResult(span, err, result); } ); }); @@ -238,6 +238,46 @@ export class GraphQLInstrumentation extends InstrumentationBase { }; } + private handleExecutionResult( + span: api.Span, + err: Error | undefined, + result: PromiseOrValue | undefined + ) { + const config = this._getConfig(); + if (config.responseHook === undefined || result === undefined || err) { + endSpan(span, err); + return; + } + + if (result.constructor.name === 'Promise') { + (result as Promise).then(resultData => { + this.executeResponseHook(span, resultData); + }); + } else { + this.executeResponseHook(span, result as graphqlTypes.ExecutionResult); + } + } + + private executeResponseHook( + span: api.Span, + result: graphqlTypes.ExecutionResult + ) { + const config = this._getConfig(); + safeExecuteInTheMiddle( + () => { + config.responseHook(span, result); + }, + err => { + if (err) { + api.diag.error('Error running response hook', err); + } + + endSpan(span, undefined); + }, + true + ); + } + private _patchParse(): (original: parseType) => parseType { const instrumentation = this; return function parse(original) { diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts index 39bbebf17f..f7df9ba32a 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts @@ -29,6 +29,10 @@ import { OTEL_GRAPHQL_DATA_SYMBOL, OTEL_PATCHED_SYMBOL } from './symbols'; export const OPERATION_NOT_SUPPORTED = 'Operation$operationName$not' + ' supported'; +export interface GraphQLInstrumentationExecutionResponseHook { + (span: api.Span, data: graphqlTypes.ExecutionResult): void; +} + export interface GraphQLInstrumentationConfig extends InstrumentationConfig { /** * When set to true it will not remove attributes values from schema source. @@ -53,6 +57,14 @@ export interface GraphQLInstrumentationConfig extends InstrumentationConfig { * @default false */ mergeItems?: boolean; + + /** + * Hook that allows adding custom span attributes based on the data + * returned from "execute" GraphQL actions. + * + * @default undefined + */ + responseHook?: GraphQLInstrumentationExecutionResponseHook; } /** diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts index 2fe6851ab9..a99f78253c 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts @@ -20,7 +20,9 @@ import { ReadableSpan, SimpleSpanProcessor, } from '@opentelemetry/tracing'; +import { Span } from '@opentelemetry/api'; import * as assert from 'assert'; +import type * as graphqlTypes from 'graphql'; import { GraphQLInstrumentation } from '../src'; import { SpanNames } from '../src/enum'; import { AttributeNames } from '../src/enums/AttributeNames'; @@ -971,6 +973,57 @@ describe('graphql', () => { }); }); + describe('when specifying a responseHook configuration', () => { + let spans: ReadableSpan[]; + let graphqlResult: graphqlTypes.ExecutionResult; + const dataAttributeName = 'graphql_data'; + + afterEach(() => { + exporter.reset(); + graphQLInstrumentation.disable(); + spans = []; + }); + + describe('AND valid responseHook', () => { + beforeEach(async () => { + create({ + responseHook: (span: Span, data: graphqlTypes.ExecutionResult) => { + span.setAttribute(dataAttributeName, JSON.stringify(data)); + }, + }); + graphqlResult = await graphql(schema, sourceList1); + spans = exporter.getFinishedSpans(); + }); + + it('should attach response hook data to the resulting spans', () => { + const querySpan = spans.find( + span => span.attributes['graphql.operation.name'] == 'query' + ); + const instrumentationResult = querySpan?.attributes[dataAttributeName]; + assert.deepStrictEqual( + instrumentationResult, + JSON.stringify(graphqlResult) + ); + }); + }); + + describe('AND invalid responseHook', () => { + beforeEach(async () => { + create({ + responseHook: (_span: Span, _data: graphqlTypes.ExecutionResult) => { + throw 'some kind of failure!'; + }, + }); + graphqlResult = await graphql(schema, sourceList1); + spans = exporter.getFinishedSpans(); + }); + + it('should not do any harm when throwing an exception', () => { + assert.deepStrictEqual(graphqlResult.data?.books?.length, 13); + }); + }); + }); + describe('when query operation is not supported', () => { let spans: ReadableSpan[]; From 815b694857b5331d00b9c2422b1f66edb0c73825 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Tue, 8 Jun 2021 19:06:20 +0300 Subject: [PATCH 08/10] Apply suggestions from code review Co-authored-by: Bartlomiej Obecny --- .../src/instrumentation.ts | 12 ++++++------ .../src/types.ts | 2 +- .../test/graphql.test.ts | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts index 4de5973a4c..d25c3c77b7 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts @@ -230,7 +230,7 @@ export class GraphQLInstrumentation extends InstrumentationBase { ]); }, (err, result) => { - instrumentation.handleExecutionResult(span, err, result); + instrumentation._handleExecutionResult(span, err, result); } ); }); @@ -238,13 +238,13 @@ export class GraphQLInstrumentation extends InstrumentationBase { }; } - private handleExecutionResult( + private _handleExecutionResult( span: api.Span, - err: Error | undefined, - result: PromiseOrValue | undefined + err?: Error, + result?: PromiseOrValue ) { const config = this._getConfig(); - if (config.responseHook === undefined || result === undefined || err) { + if (typeof config.responseHook !== 'function' || result === undefined || err) { endSpan(span, err); return; } @@ -258,7 +258,7 @@ export class GraphQLInstrumentation extends InstrumentationBase { } } - private executeResponseHook( + private _executeResponseHook( span: api.Span, result: graphqlTypes.ExecutionResult ) { diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts index f7df9ba32a..26265cc372 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/types.ts @@ -60,7 +60,7 @@ export interface GraphQLInstrumentationConfig extends InstrumentationConfig { /** * Hook that allows adding custom span attributes based on the data - * returned from "execute" GraphQL actions. + * returned from "execute" GraphQL action. * * @default undefined */ diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts index c731ac1c7e..c18a4be8e1 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts @@ -973,7 +973,7 @@ describe('graphql', () => { }); }); - describe('when specifying a responseHook configuration', () => { + describe('responseHook', () => { let spans: ReadableSpan[]; let graphqlResult: graphqlTypes.ExecutionResult; const dataAttributeName = 'graphql_data'; @@ -984,7 +984,7 @@ describe('graphql', () => { spans = []; }); - describe('AND valid responseHook', () => { + describe('when responseHook is valid', () => { beforeEach(async () => { create({ responseHook: (span: Span, data: graphqlTypes.ExecutionResult) => { @@ -1007,7 +1007,7 @@ describe('graphql', () => { }); }); - describe('AND invalid responseHook', () => { + describe('when responseHook throws an error', () => { beforeEach(async () => { create({ responseHook: (_span: Span, _data: graphqlTypes.ExecutionResult) => { @@ -1018,7 +1018,7 @@ describe('graphql', () => { spans = exporter.getFinishedSpans(); }); - it('should not do any harm when throwing an exception', () => { + it('should not do any harm', () => { assert.deepStrictEqual(graphqlResult.data?.books?.length, 13); }); }); From 8a6e7f1c7c867102e37d56897ca05238bdde7df1 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Tue, 8 Jun 2021 19:17:17 +0300 Subject: [PATCH 09/10] test: add a test for a responseHook that isn't a function --- .../src/instrumentation.ts | 4 ++-- .../test/graphql.test.ts | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts index d25c3c77b7..86e0d5c77b 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts @@ -251,10 +251,10 @@ export class GraphQLInstrumentation extends InstrumentationBase { if (result.constructor.name === 'Promise') { (result as Promise).then(resultData => { - this.executeResponseHook(span, resultData); + this._executeResponseHook(span, resultData); }); } else { - this.executeResponseHook(span, result as graphqlTypes.ExecutionResult); + this._executeResponseHook(span, result as graphqlTypes.ExecutionResult); } } diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts index c18a4be8e1..bb9ab92728 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts @@ -26,7 +26,7 @@ import type * as graphqlTypes from 'graphql'; import { GraphQLInstrumentation } from '../src'; import { SpanNames } from '../src/enum'; import { AttributeNames } from '../src/enums/AttributeNames'; -import { GraphQLInstrumentationConfig } from '../src/types'; +import { GraphQLInstrumentationConfig, GraphQLInstrumentationExecutionResponseHook } from '../src/types'; import { assertResolveSpan } from './helper'; const defaultConfig: GraphQLInstrumentationConfig = {}; @@ -1022,6 +1022,22 @@ describe('graphql', () => { assert.deepStrictEqual(graphqlResult.data?.books?.length, 13); }); }); + + describe('when responseHook is not a function', () => { + beforeEach(async () => { + // Cast to unknown so that it's possible to cast to GraphQLInstrumentationExecutionResponseHook later + const invalidTypeHook = 1234 as unknown; + create({ + responseHook: invalidTypeHook as GraphQLInstrumentationExecutionResponseHook + }); + graphqlResult = await graphql(schema, sourceList1); + spans = exporter.getFinishedSpans(); + }); + + it('should not do any harm', () => { + assert.deepStrictEqual(graphqlResult.data?.books?.length, 13); + }); + }) }); describe('when query operation is not supported', () => { From a3ce07b202448ae6014d3dcf08ff2d5b88055c21 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Tue, 8 Jun 2021 19:28:52 +0300 Subject: [PATCH 10/10] fix: lint fix --- .../src/instrumentation.ts | 6 +++++- .../test/graphql.test.ts | 12 ++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts index 86e0d5c77b..a5819b1e87 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts @@ -244,7 +244,11 @@ export class GraphQLInstrumentation extends InstrumentationBase { result?: PromiseOrValue ) { const config = this._getConfig(); - if (typeof config.responseHook !== 'function' || result === undefined || err) { + if ( + typeof config.responseHook !== 'function' || + result === undefined || + err + ) { endSpan(span, err); return; } diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts index bb9ab92728..51ac5a9bee 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts @@ -26,7 +26,10 @@ import type * as graphqlTypes from 'graphql'; import { GraphQLInstrumentation } from '../src'; import { SpanNames } from '../src/enum'; import { AttributeNames } from '../src/enums/AttributeNames'; -import { GraphQLInstrumentationConfig, GraphQLInstrumentationExecutionResponseHook } from '../src/types'; +import { + GraphQLInstrumentationConfig, + GraphQLInstrumentationExecutionResponseHook, +} from '../src/types'; import { assertResolveSpan } from './helper'; const defaultConfig: GraphQLInstrumentationConfig = {}; @@ -1026,9 +1029,10 @@ describe('graphql', () => { describe('when responseHook is not a function', () => { beforeEach(async () => { // Cast to unknown so that it's possible to cast to GraphQLInstrumentationExecutionResponseHook later - const invalidTypeHook = 1234 as unknown; + const invalidTypeHook = 1234 as unknown; create({ - responseHook: invalidTypeHook as GraphQLInstrumentationExecutionResponseHook + responseHook: + invalidTypeHook as GraphQLInstrumentationExecutionResponseHook, }); graphqlResult = await graphql(schema, sourceList1); spans = exporter.getFinishedSpans(); @@ -1037,7 +1041,7 @@ describe('graphql', () => { it('should not do any harm', () => { assert.deepStrictEqual(graphqlResult.data?.books?.length, 13); }); - }) + }); }); describe('when query operation is not supported', () => {