From 39eb8421b84f7fe298acf5c9c89de31ee0cc2604 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Thu, 11 Jul 2024 08:35:14 -0400 Subject: [PATCH] fix: Updated redis v4 instrumentation to work with transactions(multi/exec) (#2343) --- lib/instrumentation/@node-redis/client.js | 108 +++++++++------ lib/symbols.js | 1 + test/unit/instrumentation/redis.test.js | 161 ++++++++++++++++++++-- test/versioned/redis/redis-v4.tap.js | 24 ++++ test/versioned/redis/redis.tap.js | 30 ++++ 5 files changed, 270 insertions(+), 54 deletions(-) diff --git a/lib/instrumentation/@node-redis/client.js b/lib/instrumentation/@node-redis/client.js index a77ce64064..7f2f76fccf 100644 --- a/lib/instrumentation/@node-redis/client.js +++ b/lib/instrumentation/@node-redis/client.js @@ -7,63 +7,78 @@ const { OperationSpec, - params: { DatastoreParameters } + params: { DatastoreParameters }, + ClassWrapSpec } = require('../../shim/specs') -const CLIENT_COMMANDS = ['select', 'quit', 'SELECT', 'QUIT'] -const opts = Symbol('clientOptions') +const { redisClientOpts } = require('../../symbols') module.exports = function initialize(_agent, redis, _moduleName, shim) { shim.setDatastore(shim.REDIS) - const COMMANDS = Object.keys(shim.require('dist/lib/client/commands.js').default) - const CMDS_TO_INSTRUMENT = [...COMMANDS, ...CLIENT_COMMANDS] - shim.wrap(redis, 'createClient', function wrapCreateClient(shim, original) { - return function wrappedCreateClient() { - const client = original.apply(this, arguments) - client[opts] = getRedisParams(client.options) - CMDS_TO_INSTRUMENT.forEach(instrumentClientCommand.bind(null, shim, client)) - if (client.options.legacyMode) { - client.v4[opts] = getRedisParams(client.options) - CMDS_TO_INSTRUMENT.forEach(instrumentClientCommand.bind(null, shim, client.v4)) + const commandsQueue = shim.require('dist/lib/client/commands-queue.js') + + shim.wrapClass( + commandsQueue, + 'default', + new ClassWrapSpec({ + post: function postConstructor(shim) { + instrumentAddCommand({ shim, commandsQueue: this }) } + }) + ) + + shim.wrap(redis, 'createClient', function wrapCreateClient(_shim, createClient) { + return function wrappedCreateClient(options) { + // saving connection opts to shim + // since the RedisCommandsQueue gets constructed at createClient + // we can delete the symbol afterwards to ensure the appropriate + // connection options are for the given RedisCommandsQueue + shim[redisClientOpts] = getRedisParams(options) + const client = createClient.apply(this, arguments) + delete shim[redisClientOpts] return client } }) } /** - * Instruments a given command on the client by calling `shim.recordOperation` + * Instruments a given command when added to the command queue by calling `shim.recordOperation` * - * @param {Shim} shim shim instance - * @param {object} client redis client instance - * @param {string} cmd command to instrument + * @param {object} params + * @param {Shim} params.shim shim instance + * @param {object} params.commandsQueue instance */ -function instrumentClientCommand(shim, client, cmd) { +function instrumentAddCommand({ shim, commandsQueue }) { const { agent } = shim + const clientOpts = shim[redisClientOpts] - shim.recordOperation(client, cmd, function wrapCommand(_shim, _fn, _fnName, args) { - const [key, value] = args - const parameters = Object.assign({}, client[opts]) - // If selecting a database, subsequent commands - // will be using said database, update the clientOptions - // but not the current parameters(feature parity with v3) - if (cmd.toLowerCase() === 'select') { - client[opts].database_name = key - } - if (agent.config.attributes.enabled) { - if (key) { - parameters.key = JSON.stringify(key) + shim.recordOperation( + commandsQueue, + 'addCommand', + function wrapAddCommand(_shim, _fn, _fnName, args) { + const [cmd, key, value] = args[0] + const parameters = Object.assign({}, clientOpts) + // If selecting a database, subsequent commands + // will be using said database, update the clientOpts + // but not the current parameters(feature parity with v3) + if (cmd.toLowerCase() === 'select') { + clientOpts.database_name = key } - if (value) { - parameters.value = JSON.stringify(value) + if (agent.config.attributes.enabled) { + if (key) { + parameters.key = JSON.stringify(key) + } + if (value) { + parameters.value = JSON.stringify(value) + } } - } - return new OperationSpec({ - name: (cmd && cmd.toLowerCase()) || 'other', - parameters, - promise: true - }) - }) + return new OperationSpec({ + name: (cmd && cmd.toLowerCase()) || 'other', + parameters, + promise: true + }) + } + ) } /** @@ -73,6 +88,21 @@ function instrumentClientCommand(shim, client, cmd) { * @returns {object} params */ function getRedisParams(clientOpts) { + // need to replicate logic done in RedisClient + // to parse the url to assign to socket.host/port + // see: https://github.com/redis/node-redis/blob/5576a0db492cda2cd88e09881bc330aa956dd0f5/packages/client/lib/client/index.ts#L160 + if (clientOpts?.url) { + const parsedURL = new URL(clientOpts.url) + clientOpts.socket = { host: parsedURL.hostname } + if (parsedURL.port) { + clientOpts.socket.port = parsedURL.port + } + + if (parsedURL.pathname) { + clientOpts.database = parsedURL.pathname.substring(1) + } + } + return new DatastoreParameters({ host: clientOpts?.socket?.host || 'localhost', port_path_or_id: clientOpts?.socket?.path || clientOpts?.socket?.port || '6379', diff --git a/lib/symbols.js b/lib/symbols.js index f5cd6fb734..99bed59f81 100644 --- a/lib/symbols.js +++ b/lib/symbols.js @@ -27,6 +27,7 @@ module.exports = { langchainRunId: Symbol('runId'), prismaConnection: Symbol('prismaConnection'), prismaModelCall: Symbol('modelCall'), + redisClientOpts: Symbol('clientOptions'), segment: Symbol('segment'), shim: Symbol('shim'), storeDatabase: Symbol('storeDatabase'), diff --git a/test/unit/instrumentation/redis.test.js b/test/unit/instrumentation/redis.test.js index 8ee2406bb5..ebe6d20c08 100644 --- a/test/unit/instrumentation/redis.test.js +++ b/test/unit/instrumentation/redis.test.js @@ -6,54 +6,55 @@ 'use strict' const tap = require('tap') -const client = require('../../../lib/instrumentation/@node-redis/client') +const sinon = require('sinon') +const helper = require('../../lib/agent_helper') +const DatastoreShim = require('../../../lib/shim/datastore-shim.js') +const { redisClientOpts } = require('../../../lib/symbols') tap.test('getRedisParams should behave as expected', function (t) { - t.autoend() - + const { getRedisParams } = require('../../../lib/instrumentation/@node-redis/client') t.test('given no opts, should return sensible defaults', function (t) { - t.autoend() - const params = client.getRedisParams() + const params = getRedisParams() const expected = { host: 'localhost', port_path_or_id: '6379', database_name: 0 } t.match(params, expected, 'redis client should be definable without params') + t.end() }) t.test('if host/port are defined incorrectly, should return expected defaults', function (t) { - t.autoend() - const params = client.getRedisParams({ host: 'myLocalHost', port: '1234' }) + const params = getRedisParams({ host: 'myLocalHost', port: '1234' }) const expected = { host: 'localhost', port_path_or_id: '6379', database_name: 0 } t.match(params, expected, 'should return sensible defaults if defined without socket') + t.end() }) t.test('if host/port are defined correctly, we should see them in config', function (t) { - t.autoend() - const params = client.getRedisParams({ socket: { host: 'myLocalHost', port: '1234' } }) + const params = getRedisParams({ socket: { host: 'myLocalHost', port: '1234' } }) const expected = { host: 'myLocalHost', port_path_or_id: '1234', database_name: 0 } t.match(params, expected, 'host/port should be returned when defined correctly') + t.end() }) t.test('path should be used if defined', function (t) { - t.autoend() - const params = client.getRedisParams({ socket: { path: '5678' } }) + const params = getRedisParams({ socket: { path: '5678' } }) const expected = { host: 'localhost', port_path_or_id: '5678', database_name: 0 } t.match(params, expected, 'path should show up in params') + t.end() }) t.test('path should be preferred over port', function (t) { - t.autoend() - const params = client.getRedisParams({ + const params = getRedisParams({ socket: { host: 'myLocalHost', port: '1234', path: '5678' } }) const expected = { @@ -62,15 +63,145 @@ tap.test('getRedisParams should behave as expected', function (t) { database_name: 0 } t.match(params, expected, 'path should show up in params') + t.end() }) t.test('database name should be definable', function (t) { - t.autoend() - const params = client.getRedisParams({ database: 12 }) + const params = getRedisParams({ database: 12 }) const expected = { host: 'localhost', port_path_or_id: '6379', database_name: 12 } t.match(params, expected, 'database should be definable') + t.end() + }) + + t.test('host/port/database should be extracted from url when it exists', function (t) { + const params = getRedisParams({ url: 'redis://host:6369/db' }) + const expected = { + host: 'host', + port_path_or_id: '6369', + database_name: 'db' + } + t.match(params, expected, 'host/port/database should match') + t.end() + }) + + t.test('should default port to 6379 when no port specified in URL', function (t) { + const params = getRedisParams({ url: 'redis://host/db' }) + const expected = { + host: 'host', + port_path_or_id: '6379', + database_name: 'db' + } + t.match(params, expected, 'host/port/database should match') + t.end() + }) + + t.test('should default database to 0 when no db pecified in URL', function (t) { + const params = getRedisParams({ url: 'redis://host' }) + const expected = { + host: 'host', + port_path_or_id: '6379', + database_name: 0 + } + t.match(params, expected, 'host/port/database should match') + t.end() + }) + t.end() +}) + +tap.test('createClient saves connection options', function (t) { + t.beforeEach((t) => { + t.context.sandbox = sinon.createSandbox() + t.context.agent = helper.loadMockedAgent() + t.context.shim = new DatastoreShim(t.context.agent, 'redis') + t.context.instrumentation = require('../../../lib/instrumentation/@node-redis/client') + t.context.clients = { + 1: { socket: { host: '1', port: 2 } }, + 2: { socket: { host: '2', port: 3 } } + } + let i = 0 + class CommandQueueClass { + constructor() { + i++ + this.id = i + const expectedValues = t.context.clients[this.id] + t.match(t.context.shim[redisClientOpts], { + host: expectedValues.socket.host, + port_path_or_id: expectedValues.socket.port + }) + } + + async addCommand() {} + } + + const commandQueueStub = { default: CommandQueueClass } + const redis = Object.create({ + createClient: function () { + const instance = Object.create({}) + // eslint-disable-next-line new-cap + instance.queue = new commandQueueStub.default() + return instance + } + }) + + t.context.sandbox.stub(t.context.shim, 'require').returns(commandQueueStub) + t.context.redis = redis + }) + + t.afterEach((t) => { + helper.unloadAgent(t.context.agent) + t.context.sandbox.restore() + }) + + t.test('should remove connect options after creation', function (t) { + const { agent, redis, shim, instrumentation, clients } = t.context + instrumentation(agent, redis, 'redis', shim) + redis.createClient(clients[1]) + t.notOk(shim[redisClientOpts], 'should remove client options after creation') + redis.createClient(clients[2]) + t.notOk(shim[redisClientOpts], 'should remove client options after creation') + t.end() + }) + + t.test('should keep the connection details per client', function (t) { + const { agent, redis, shim, instrumentation, clients } = t.context + instrumentation(agent, redis, 'redis', shim) + const client = redis.createClient(clients[1]) + const client2 = redis.createClient(clients[2]) + helper.runInTransaction(agent, async function (tx) { + await client.queue.addCommand(['test', 'key', 'value']) + await client2.queue.addCommand(['test2', 'key2', 'value2']) + const [redisSegment, redisSegment2] = tx.trace.root.children + const attrs = redisSegment.getAttributes() + t.same( + attrs, + { + host: '1', + port_path_or_id: 2, + key: '"key"', + value: '"value"', + product: 'Redis', + database_name: '0' + }, + 'should have appropriate segment attrs' + ) + const attrs2 = redisSegment2.getAttributes() + t.same( + attrs2, + { + host: '2', + port_path_or_id: 3, + key: '"key2"', + value: '"value2"', + product: 'Redis', + database_name: '0' + }, + 'should have appropriate segment attrs' + ) + t.end() + }) }) + t.end() }) diff --git a/test/versioned/redis/redis-v4.tap.js b/test/versioned/redis/redis-v4.tap.js index 029700223e..5b43c4225e 100644 --- a/test/versioned/redis/redis-v4.tap.js +++ b/test/versioned/redis/redis-v4.tap.js @@ -116,6 +116,30 @@ test('Redis instrumentation', function (t) { }) }) + t.test('should handle multi commands', function (t) { + helper.runInTransaction(agent, async function transactionInScope() { + const transaction = agent.getTransaction() + const results = await client.multi().set('multi-key', 'multi-value').get('multi-key').exec() + + t.same(results, ['OK', 'multi-value'], 'should return expected results') + transaction.end() + const unscoped = transaction.metrics.unscoped + const expected = { + 'Datastore/all': 4, + 'Datastore/allWeb': 4, + 'Datastore/Redis/all': 4, + 'Datastore/Redis/allWeb': 4, + 'Datastore/operation/Redis/multi': 1, + 'Datastore/operation/Redis/set': 1, + 'Datastore/operation/Redis/get': 1, + 'Datastore/operation/Redis/exec': 1 + } + expected['Datastore/instance/Redis/' + HOST_ID] = 4 + checkMetrics(t, unscoped, expected) + t.end() + }) + }) + t.test('should add `key` attribute to trace segment', function (t) { t.notOk(agent.getTransaction(), 'no transaction should be in play') agent.config.attributes.enabled = true diff --git a/test/versioned/redis/redis.tap.js b/test/versioned/redis/redis.tap.js index 25fc307065..99d3641e2a 100644 --- a/test/versioned/redis/redis.tap.js +++ b/test/versioned/redis/redis.tap.js @@ -185,6 +185,36 @@ test('Redis instrumentation', { timeout: 20000 }, function (t) { }) }) + t.test('should handle multi commands', function (t) { + helper.runInTransaction(agent, function transactionInScope() { + const transaction = agent.getTransaction() + client + .multi() + .set('multi-key', 'multi-value') + .get('multi-key') + .exec(function (error, data) { + t.same(data, ['OK', 'multi-value'], 'should return expected results') + t.error(error) + + transaction.end() + const unscoped = transaction.metrics.unscoped + const expected = { + 'Datastore/all': 4, + 'Datastore/allWeb': 4, + 'Datastore/Redis/all': 4, + 'Datastore/Redis/allWeb': 4, + 'Datastore/operation/Redis/multi': 1, + 'Datastore/operation/Redis/set': 1, + 'Datastore/operation/Redis/get': 1, + 'Datastore/operation/Redis/exec': 1 + } + expected['Datastore/instance/Redis/' + HOST_ID] = 4 + checkMetrics(t, unscoped, expected) + t.end() + }) + }) + }) + t.test('should add `key` attribute to trace segment', function (t) { agent.config.attributes.enabled = true