From b2284c58eb798ab00a141ce08f4072584795c728 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Mon, 20 May 2024 14:03:20 -0400 Subject: [PATCH] feat: Added support for redis v4 legacyMode `client.v4.` (#2200) --- lib/instrumentation/@node-redis/client.js | 4 + test/versioned/redis/package.json | 3 +- .../redis/redis-v4-legacy-mode.tap.js | 257 ++++++++++++++++++ 3 files changed, 263 insertions(+), 1 deletion(-) create mode 100644 test/versioned/redis/redis-v4-legacy-mode.tap.js diff --git a/lib/instrumentation/@node-redis/client.js b/lib/instrumentation/@node-redis/client.js index b2f3ea109b..a77ce64064 100644 --- a/lib/instrumentation/@node-redis/client.js +++ b/lib/instrumentation/@node-redis/client.js @@ -21,6 +21,10 @@ module.exports = function initialize(_agent, redis, _moduleName, shim) { 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)) + } return client } }) diff --git a/test/versioned/redis/package.json b/test/versioned/redis/package.json index 4152508731..72374be169 100644 --- a/test/versioned/redis/package.json +++ b/test/versioned/redis/package.json @@ -56,7 +56,8 @@ "redis": ">=4.0.0" }, "files": [ - "redis-v4.tap.js" + "redis-v4.tap.js", + "redis-v4-legacy-mode.tap.js" ] } ] diff --git a/test/versioned/redis/redis-v4-legacy-mode.tap.js b/test/versioned/redis/redis-v4-legacy-mode.tap.js new file mode 100644 index 0000000000..8abffe4ce3 --- /dev/null +++ b/test/versioned/redis/redis-v4-legacy-mode.tap.js @@ -0,0 +1,257 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +const tap = require('tap') +const test = tap.test +const helper = require('../../lib/agent_helper') +const { removeMatchedModules } = require('../../lib/cache-buster') +const params = require('../../lib/params') +const urltils = require('../../../lib/util/urltils') + +// Indicates unique database in Redis. 0-15 supported. +const DB_INDEX = 2 + +test('Redis instrumentation', function (t) { + t.autoend() + + let METRIC_HOST_NAME = null + let HOST_ID = null + + let agent + let client + + t.beforeEach(async function () { + agent = helper.instrumentMockedAgent() + + const redis = require('redis') + client = redis.createClient({ + legacyMode: true, + socket: { port: params.redis_port, host: params.redis_host } + }) + + await client.connect() + await client.v4.flushAll() + + await client.v4.select(DB_INDEX) + // eslint-disable-next-line new-cap + + METRIC_HOST_NAME = urltils.isLocalhost(params.redis_host) + ? agent.config.getHostnameSafe() + : params.redis_host + HOST_ID = METRIC_HOST_NAME + '/' + params.redis_port + + // need to capture attributes + agent.config.attributes.enabled = true + }) + + t.afterEach(async function () { + agent && helper.unloadAgent(agent) + if (client) { + await client.v4.flushAll() + await client.v4.quit() + } + // must purge require cache of redis related instrumentation + // otherwise it will not re-register on subsequent test runs + removeMatchedModules(/redis/) + }) + + t.test('should find Redis calls in the transaction trace', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + helper.runInTransaction(agent, async function transactionInScope() { + const transaction = agent.getTransaction() + t.ok(transaction, 'transaction should be visible') + + const ok = await client.v4.set('testkey', 'arglbargle') + t.ok(agent.getTransaction(), 'transaction should still be visible') + t.ok(ok, 'everything should be peachy after setting') + + const value = await client.v4.get('testkey') + t.ok(agent.getTransaction(), 'transaction should still still be visible') + t.equal(value, 'arglbargle', 'redis client should still work') + + const trace = transaction.trace + t.ok(trace, 'trace should exist') + t.ok(trace.root, 'root element should exist') + t.equal(trace.root.children.length, 2, 'there should be only two children of the root') + + const setSegment = trace.root.children[0] + const setAttributes = setSegment.getAttributes() + t.ok(setSegment, 'trace segment for set should exist') + t.equal(setSegment.name, 'Datastore/operation/Redis/set', 'should register the set') + t.equal(setAttributes.key, '"testkey"', 'should have the set key as a attribute') + t.equal(setSegment.children.length, 0, 'set should have no children') + + const getSegment = trace.root.children[1] + const getAttributes = getSegment.getAttributes() + t.ok(getSegment, 'trace segment for get should exist') + + t.equal(getSegment.name, 'Datastore/operation/Redis/get', 'should register the get') + + t.equal(getAttributes.key, '"testkey"', 'should have the get key as a attribute') + + t.ok(getSegment.timer.hrDuration, 'trace segment should have ended') + t.end() + }) + }) + + t.test('should create correct metrics', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + helper.runInTransaction(agent, async function transactionInScope() { + const transaction = agent.getTransaction() + await client.v4.set('testkey', 'arglbargle') + await client.v4.get('testkey') + transaction.end() + const unscoped = transaction.metrics.unscoped + const expected = { + 'Datastore/all': 2, + 'Datastore/allWeb': 2, + 'Datastore/Redis/all': 2, + 'Datastore/Redis/allWeb': 2, + 'Datastore/operation/Redis/set': 1, + 'Datastore/operation/Redis/get': 1 + } + 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 + + helper.runInTransaction(agent, async function () { + await client.v4.set('saveme', 'foobar') + + const segment = agent.tracer.getSegment().children[0] + t.equal(segment.getAttributes().key, '"saveme"', 'should have `key` attribute') + t.end() + }) + }) + + t.test('should not add `key` attribute to trace segment', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + agent.config.attributes.enabled = false + + helper.runInTransaction(agent, async function () { + await client.v4.set('saveme', 'foobar') + + const segment = agent.tracer.getSegment().children[0] + t.notOk(segment.getAttributes().key, 'should not have `key` attribute') + t.end() + }) + }) + + t.test('should add datastore instance attributes to trace segments', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + // Enable. + agent.config.datastore_tracer.instance_reporting.enabled = true + agent.config.datastore_tracer.database_name_reporting.enabled = true + + helper.runInTransaction(agent, async function transactionInScope() { + const transaction = agent.getTransaction() + await client.v4.set('testkey', 'arglbargle') + + const trace = transaction.trace + const setSegment = trace.root.children[0] + const attributes = setSegment.getAttributes() + t.equal(attributes.host, METRIC_HOST_NAME, 'should have host as attribute') + t.equal( + attributes.port_path_or_id, + String(params.redis_port), + 'should have port as attribute' + ) + t.equal(attributes.database_name, String(DB_INDEX), 'should have database id as attribute') + t.equal(attributes.product, 'Redis', 'should have product attribute') + t.end() + }) + }) + + t.test('should not add instance attributes/metrics when disabled', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + // disable + agent.config.datastore_tracer.instance_reporting.enabled = false + agent.config.datastore_tracer.database_name_reporting.enabled = false + + helper.runInTransaction(agent, async function transactionInScope() { + const transaction = agent.getTransaction() + await client.v4.set('testkey', 'arglbargle') + + const setSegment = transaction.trace.root.children[0] + const attributes = setSegment.getAttributes() + t.equal(attributes.host, undefined, 'should not have host attribute') + t.equal(attributes.port_path_or_id, undefined, 'should not have port attribute') + t.equal(attributes.database_name, undefined, 'should not have db name attribute') + + transaction.end() + const unscoped = transaction.metrics.unscoped + t.equal( + unscoped['Datastore/instance/Redis/' + HOST_ID], + undefined, + 'should not have instance metric' + ) + t.end() + }) + }) + + t.test('should follow selected database', function (t) { + t.notOk(agent.getTransaction(), 'no transaction should be in play') + let transaction = null + const SELECTED_DB = 3 + helper.runInTransaction(agent, async function (tx) { + transaction = tx + await client.v4.set('select:test:key', 'foo') + t.ok(agent.getTransaction(), 'should not lose transaction state') + + await client.v4.select(SELECTED_DB) + t.ok(agent.getTransaction(), 'should not lose transaction state') + + await client.v4.set('select:test:key:2', 'bar') + t.ok(agent.getTransaction(), 'should not lose transaction state') + transaction.end() + verify() + t.end() + }) + + function verify() { + const setSegment1 = transaction.trace.root.children[0] + const selectSegment = transaction.trace.root.children[1] + const setSegment2 = transaction.trace.root.children[2] + + t.equal(setSegment1.name, 'Datastore/operation/Redis/set', 'should register the first set') + t.equal( + setSegment1.getAttributes().database_name, + String(DB_INDEX), + 'should have the starting database id as attribute for the first set' + ) + t.equal(selectSegment.name, 'Datastore/operation/Redis/select', 'should register the select') + t.equal( + selectSegment.getAttributes().database_name, + String(DB_INDEX), + 'should have the starting database id as attribute for the select' + ) + t.equal(setSegment2.name, 'Datastore/operation/Redis/set', 'should register the second set') + t.equal( + setSegment2.getAttributes().database_name, + String(SELECTED_DB), + 'should have the selected database id as attribute for the second set' + ) + } + }) +}) + +function checkMetrics(t, metrics, expected) { + Object.keys(expected).forEach(function (name) { + t.ok(metrics[name], 'should have metric ' + name) + if (metrics[name]) { + t.equal( + metrics[name].callCount, + expected[name], + 'should have ' + expected[name] + ' calls for ' + name + ) + } + }) +}