From 4a1f6adddc3839c9ace405ba2ba3ed431d335c2e Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 15 Nov 2024 09:36:55 +0530 Subject: [PATCH 01/23] feat: added configuration to ignore specific endpoints from tracing --- .../src/announceCycle/unannounced.js | 28 +++++++++ .../test/announceCycle/unannounced_test.js | 44 ++++++++++++++ .../test/tracing/database/redis/app.js | 14 ++++- .../test/tracing/database/redis/legacyApp.js | 13 ++++- .../test/tracing/database/redis/test.js | 58 +++++++++++++++++++ .../core/src/tracing/backend_mappers/index.js | 53 +++++++++++++++++ .../tracing/backend_mappers/redis_mapper.js | 32 ++++++++++ packages/core/src/tracing/index.js | 5 ++ .../tracing/instrumentation/database/redis.js | 5 +- packages/core/src/tracing/spanBuffer.js | 18 ++++++ packages/core/src/util/filterSpan.js | 45 ++++++++++++++ packages/core/src/util/normalizeConfig.js | 40 ++++++++++++- .../core/test/util/normalizeConfig_test.js | 22 +++++++ 13 files changed, 371 insertions(+), 6 deletions(-) create mode 100644 packages/core/src/tracing/backend_mappers/index.js create mode 100644 packages/core/src/tracing/backend_mappers/redis_mapper.js create mode 100644 packages/core/src/util/filterSpan.js diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index dc22d6850a..cc73efec57 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -43,6 +43,7 @@ const maxRetryDelay = 60 * 1000; // one minute * @typedef {Object} TracingConfig * @property {Array.} [extra-http-headers] * @property {KafkaTracingConfig} [kafka] + * @property {IgnoreEndpoints} [ignore-endpoints] * @property {boolean} [span-batching-enabled] */ @@ -51,6 +52,11 @@ const maxRetryDelay = 60 * 1000; // one minute * @property {boolean} [trace-correlation] */ +/** + * @typedef {Object} IgnoreEndpoints + * @property {Object.>} [endpoints] + */ + module.exports = { /** * @param {import('./').AnnounceCycleContext} ctx @@ -126,6 +132,7 @@ function applyAgentConfiguration(agentResponse) { applyExtraHttpHeaderConfiguration(agentResponse); applyKafkaTracingConfiguration(agentResponse); applySpanBatchingConfiguration(agentResponse); + applyIgnoreEndpointsConfiguration(agentResponse); } /** @@ -220,3 +227,24 @@ function applySpanBatchingConfiguration(agentResponse) { agentOpts.config.tracing.spanBatchingEnabled = true; } } +/** + * @param {AgentAnnounceResponse} agentResponse + */ +function applyIgnoreEndpointsConfiguration(agentResponse) { + if (agentResponse.tracing && agentResponse.tracing['ignore-endpoints']) { + const endpointTracingConfigFromAgent = agentResponse.tracing['ignore-endpoints']; + const endpointTracingConfig = {}; + // eslint-disable-next-line no-restricted-syntax + for (const [service, actions] of Object.entries(endpointTracingConfigFromAgent)) { + // From agent, the commands are separated by a pipe ('|'), split the actions into an array. + // @ts-ignore + endpointTracingConfig[service.toLowerCase()] = + // @ts-ignore + actions != null ? actions?.split('|')?.map(action => action?.trim()?.toLowerCase()) : []; + } + + ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); + // @ts-ignore + agentOpts.config.tracing.ignoreEndpoints = endpointTracingConfig; + } +} diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 37fbf9e790..48005f9283 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -213,6 +213,50 @@ describe('unannounced state', () => { } }); }); + it('should apply the configuration to ignore specified endpoints for a single technology', done => { + prepareAnnounceResponse({ + tracing: { + 'ignore-endpoints': { + redis: 'get' + } + } + }); + unannouncedState.enter({ + transitionTo: () => { + expect(agentOptsStub.config).to.deep.equal({ + tracing: { + ignoreEndpoints: { + redis: ['get'] + } + } + }); + done(); + } + }); + }); + it('should apply the tracing configuration to ignore multiple endpoints across different technologies', done => { + prepareAnnounceResponse({ + tracing: { + 'ignore-endpoints': { + REDIS: 'get | type', + dynamodb: 'query' + } + } + }); + unannouncedState.enter({ + transitionTo: () => { + expect(agentOptsStub.config).to.deep.equal({ + tracing: { + ignoreEndpoints: { + redis: ['get', 'type'], + dynamodb: ['query'] + } + } + }); + done(); + } + }); + }); function prepareAnnounceResponse(announceResponse) { agentConnectionStub.announceNodeCollector.callsArgWithAsync(0, null, JSON.stringify(announceResponse)); diff --git a/packages/collector/test/tracing/database/redis/app.js b/packages/collector/test/tracing/database/redis/app.js index 1da94a24ee..6c1c8c2387 100644 --- a/packages/collector/test/tracing/database/redis/app.js +++ b/packages/collector/test/tracing/database/redis/app.js @@ -12,8 +12,18 @@ process.on('SIGTERM', () => { }); require('./mockVersion'); -require('../../../..')(); - +const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; +if (!ignoreEndpointsEnabled) { + require('../../../..')(); +} else { + require('../../../..')({ + tracing: { + ignoreEndpoints: { + redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + } + } + }); +} const redis = require(process.env.REDIS_PKG); const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/redis/legacyApp.js b/packages/collector/test/tracing/database/redis/legacyApp.js index 24cc1e208f..8d201ecd3b 100644 --- a/packages/collector/test/tracing/database/redis/legacyApp.js +++ b/packages/collector/test/tracing/database/redis/legacyApp.js @@ -15,7 +15,18 @@ process.on('SIGTERM', () => { require('./mockVersion'); -require('../../../..')(); +const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; +if (!ignoreEndpointsEnabled) { + require('../../../..')(); +} else { + require('../../../..')({ + tracing: { + ignoreEndpoints: { + redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + } + } + }); +} const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 70b497cde1..ad116826f8 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -199,6 +199,64 @@ const globalAgent = require('../../../globalAgent'); }) ); })); + // TODO + it.skip('should not create spans for ignored commands via ignoredEndpoints', async () => { + const ignoreControls = new ProcessControls({ + useGlobalAgent: true, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + } + }); + + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'price', + value: 42 + } + }) + .then(() => + ignoreControls.sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + ) + .then(async response => { + expect(String(response)).to.equal('42'); + + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); + }) + .finally(async () => { + await ignoreControls.stop(); + }); + }); it('must trace hset/hget calls', () => controls diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js new file mode 100644 index 0000000000..17e9d2fe69 --- /dev/null +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -0,0 +1,53 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +/** + * @type {Record} + */ +const cachedMappers = {}; + +/** + * Dynamically require the mapper only if needed, based on span.n. + * This method will only load the mapper file for the specific technology. + * + * @param {string} technology - The technology name (e.g., 'redis', 'http'). + * @returns {Function|null} - The transformation function for the technology or null if not found. + */ +function loadMapper(technology) { + if (cachedMappers[technology]) { + return cachedMappers[technology]; + } + + try { + const mapper = require(`./${technology}_mapper.js`).transform; + cachedMappers[technology] = mapper; + + return mapper; + } catch (err) { + return null; // Return null if the mapper doesn't exist or failed to load + } +} + +/** + * A function that applies the transformation for the span based on span.n. + * If no mapper is found, the span is returned unmodified without causing failure. + * + * @param {import('../../core').InstanaBaseSpan} span - The span object that needs to be processed. + * @param {import('../../core').InstanaBaseSpan} span - The transformed span. + */ +function transform(span) { + const technology = span.n; + + const transformFunction = loadMapper(technology); + + if (transformFunction) { + return transformFunction(span); + } else { + return span; + } +} + +module.exports = { transform }; diff --git a/packages/core/src/tracing/backend_mappers/redis_mapper.js b/packages/core/src/tracing/backend_mappers/redis_mapper.js new file mode 100644 index 0000000000..5ae4ecc88c --- /dev/null +++ b/packages/core/src/tracing/backend_mappers/redis_mapper.js @@ -0,0 +1,32 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +// Configuration for Redis-specific field mappings for BE +const fieldMappings = { + operation: 'command' +}; + +/** + * Transform the span object by renaming the Redis fields based on defined mappings. + * + * @param {Object} span - The span object containing Redis data to transform. + * @param {boolean} log - Flag to enable/disable logging of transformations. + * @returns {Object} - The transformed span object. + */ +function transform(span) { + if (span.data?.redis) { + Object.entries(fieldMappings).forEach(([oldKey, newKey]) => { + if (span.data.redis[oldKey]) { + span.data.redis[newKey] = span.data.redis[oldKey]; + delete span.data.redis[oldKey]; + } + }); + } + + return span; +} + +module.exports = { transform }; diff --git a/packages/core/src/tracing/index.js b/packages/core/src/tracing/index.js index 8056e5265e..aa9b53e1a1 100644 --- a/packages/core/src/tracing/index.js +++ b/packages/core/src/tracing/index.js @@ -119,6 +119,11 @@ if (customInstrumentations.length > 0) { * @property {boolean} [traceCorrelation] */ +/** + * @typedef {Object} IgnoreEndpoints + * @property {Object.>} [endpoints] + */ + /** @type {Array.} */ let additionalInstrumentationModules = []; /** @type {Object.} */ diff --git a/packages/core/src/tracing/instrumentation/database/redis.js b/packages/core/src/tracing/instrumentation/database/redis.js index a7476b9fb2..945f16ff7e 100644 --- a/packages/core/src/tracing/instrumentation/database/redis.js +++ b/packages/core/src/tracing/instrumentation/database/redis.js @@ -239,9 +239,10 @@ function instrumentCommand(original, command, address, cbStyle) { const span = cls.startSpan(exports.spanName, constants.EXIT); span.stack = tracingUtil.getStackTrace(instrumentCommand); + // Internal property `operation` to hold command span.data.redis = { connection: address || origCtx.address, - command + operation: command }; let userProvidedCallback; @@ -323,7 +324,7 @@ function instrumentMultiExec(origCtx, origArgs, original, address, isAtomic, cbS span.data.redis = { connection: address, // pipeline = batch - command: isAtomic ? 'multi' : 'pipeline' + operation: isAtomic ? 'multi' : 'pipeline' }; const subCommands = (span.data.redis.subCommands = []); diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 6a2e860542..9a3e01fac3 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -6,6 +6,8 @@ 'use strict'; const tracingMetrics = require('./metrics'); +const { filterSpan } = require('../util/filterSpan'); +const { transform } = require('./backend_mappers'); /** @type {import('../core').GenericLogger} */ let logger; @@ -179,6 +181,10 @@ exports.addSpan = function (span) { return; } + // transform the spans + // filter the spans + span = manageSpan(span); + if (span.t == null) { logger.warn('Span of type %s has no trace ID. Not transmitting this span', span.n); return; @@ -483,3 +489,15 @@ function removeSpansIfNecessary() { spans = spans.slice(-maxBufferedSpans); } } +// @ts-ignore +function manageSpan(span) { + if (!span || typeof span.n !== 'string') { + return span; + } + const filteredSpan = filterSpan(span); + + if (!filteredSpan) { + return null; // Return null if span was ignored + } + return transform(filteredSpan); +} diff --git a/packages/core/src/util/filterSpan.js b/packages/core/src/util/filterSpan.js new file mode 100644 index 0000000000..ab9c7222ce --- /dev/null +++ b/packages/core/src/util/filterSpan.js @@ -0,0 +1,45 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +// TODO: read from config +const config = { + tracing: { + ignoreEndpoints: { + redis: ['type'] + } + } +}; + +/** + * To check whether a span should be ignored based on the command. + * + * @param {import('../core').InstanaBaseSpan} span - The span object to check. + * @returns {boolean} - Returns true if the span should be ignored, false otherwise. + */ +function shouldIgnore(span) { + // @ts-ignore + const ignoreEndpoints = config.tracing.ignoreEndpoints[span.n]; + + // @ts-ignore + if (span.data?.[span.n]?.command && ignoreEndpoints) { + return ignoreEndpoints.includes(span.data[span.n].command); + } + + return false; +} + +/** + * @param {Object} span - The span object to check. + * @returns {Object|null} - Returns the span if not ignored, or null if the span should be ignored. + */ +function filterSpan(span) { + if (shouldIgnore(span)) { + return null; + } + return span; +} + +module.exports = { filterSpan }; diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 44f74a65ac..1f56a91cd4 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -27,6 +27,7 @@ const constants = require('../tracing/constants'); * @property {boolean} [disableW3cTraceCorrelation] * @property {KafkaTracingOptions} [kafka] * @property {boolean} [allowRootExitSpan] + * @property {Object} [ignoreEndpoints] */ /** @@ -61,6 +62,11 @@ const constants = require('../tracing/constants'); * @property {number} [timeBetweenHealthcheckCalls] */ +/** + * @typedef {Object} IgnoreEndpoints + * @property {Object.>} [endpoints] + */ + /** * @typedef {Object} AgentConfig * @property {AgentTracingConfig} [tracing] @@ -71,6 +77,7 @@ const constants = require('../tracing/constants'); * @property {AgentTracingHttpConfig} [http] * @property {AgentTracingKafkaConfig} [kafka] * @property {boolean|string} [spanBatchingEnabled] + * @property {Object} [AgentTracingIgnoreEndpoints] */ /** @@ -83,6 +90,11 @@ const constants = require('../tracing/constants'); * @property {boolean} [traceCorrelation] */ +/** + * @typedef {Object} AgentTracingIgnoreEndpoints + * @property {Object.>} [endpoints] + */ + /** @type {import('../core').GenericLogger} */ let logger; logger = require('../logger').getLogger('configuration', newLogger => { @@ -117,7 +129,8 @@ const defaults = { disableW3cTraceCorrelation: false, kafka: { traceCorrelation: true - } + }, + ignoreEndpoints: {} }, secrets: { matcherMode: 'contains-ignore-case', @@ -218,6 +231,7 @@ function normalizeTracingConfig(config) { normalizeDisableW3cTraceCorrelation(config); normalizeTracingKafka(config); normalizeAllowRootExitSpan(config); + normalizeIgnoredEndpoints(config); } /** @@ -674,3 +688,27 @@ function normalizeSingleValue(configValue, defaultValue, configPath, envVarKey) } return configValue; } +/** + * @param {InstanaConfig} config + */ +function normalizeIgnoredEndpoints(config) { + if (!config.tracing.ignoreEndpoints) { + config.tracing.ignoreEndpoints = {}; + } + + for (const [service, methods] of Object.entries(config.tracing.ignoreEndpoints)) { + const normalizedService = service.toLowerCase(); + if (!Array.isArray(methods)) { + console.warn( + `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array, the value will be ignored: ${JSON.stringify( + methods + )}` + ); + // @ts-ignore + config.tracing.ignoreEndpoints[normalizedService] = []; + } else { + // @ts-ignore + config.tracing.ignoreEndpoints[normalizedService] = methods.map(method => method.toLowerCase()); + } + } +} diff --git a/packages/core/test/util/normalizeConfig_test.js b/packages/core/test/util/normalizeConfig_test.js index 3cbc6402b0..df405cf0e3 100644 --- a/packages/core/test/util/normalizeConfig_test.js +++ b/packages/core/test/util/normalizeConfig_test.js @@ -471,6 +471,28 @@ describe('util.normalizeConfig', () => { const config = normalizeConfig(); expect(config.tracing.allowRootExitSpan).to.equal(true); }); + it('should not disable ignore endpoints tracers by default', () => { + const config = normalizeConfig(); + expect(config.tracing.ignoreEndpoints).to.deep.equal({}); + }); + + it('should apply ignore endpoints tracers via config', () => { + const config = normalizeConfig({ + tracing: { + ignoreEndpoints: { redis: ['get'] } + } + }); + expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'] }); + }); + + it('should apply ignore endpoints tracers via config', () => { + const config = normalizeConfig({ + tracing: { + ignoreEndpoints: { redis: ['get'], dynamodb: ['querey'] } + } + }); + expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'], dynamodb: ['querey'] }); + }); function checkDefaults(config) { expect(config).to.be.an('object'); From c01210fe3e8f78f15c4706dc4f8f323598bfb020 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 15 Nov 2024 15:58:39 +0530 Subject: [PATCH 02/23] test: added more tests --- .../test/announceCycle/unannounced_test.js | 4 +- .../core/src/tracing/backend_mappers/index.js | 28 ++++---- packages/core/src/util/filterSpan.js | 15 ++--- .../tracing/backend_mappers/mapper_test.js | 44 +++++++++++++ packages/core/test/util/filterSpan_test.js | 64 +++++++++++++++++++ 5 files changed, 127 insertions(+), 28 deletions(-) create mode 100644 packages/core/test/tracing/backend_mappers/mapper_test.js create mode 100644 packages/core/test/util/filterSpan_test.js diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 48005f9283..50852d6697 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -213,7 +213,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the configuration to ignore specified endpoints for a single technology', done => { + it('should apply the configuration to ignore specified endpoints for a single module', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { @@ -234,7 +234,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the tracing configuration to ignore multiple endpoints across different technologies', done => { + it('should apply the tracing configuration to ignore multiple endpoints across different modules', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js index 17e9d2fe69..801f817849 100644 --- a/packages/core/src/tracing/backend_mappers/index.js +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -10,38 +10,32 @@ const cachedMappers = {}; /** - * Dynamically require the mapper only if needed, based on span.n. - * This method will only load the mapper file for the specific technology. + * Dynamically require the mapper based on span.n. * - * @param {string} technology - The technology name (e.g., 'redis', 'http'). - * @returns {Function|null} - The transformation function for the technology or null if not found. + * @param {string} spanName - The spanName name (e.g., 'redis', 'http'). + * @returns {Function|null} - The BE transformation function for the span.n */ -function loadMapper(technology) { - if (cachedMappers[technology]) { - return cachedMappers[technology]; +function loadMapper(spanName) { + if (cachedMappers[spanName]) { + return cachedMappers[spanName]; } try { - const mapper = require(`./${technology}_mapper.js`).transform; - cachedMappers[technology] = mapper; - + // While adding a new mapper file, always expected in the same format. + const mapper = require(`./${spanName}_mapper.js`).transform; + cachedMappers[spanName] = mapper; return mapper; } catch (err) { - return null; // Return null if the mapper doesn't exist or failed to load + return null; } } /** - * A function that applies the transformation for the span based on span.n. - * If no mapper is found, the span is returned unmodified without causing failure. - * * @param {import('../../core').InstanaBaseSpan} span - The span object that needs to be processed. * @param {import('../../core').InstanaBaseSpan} span - The transformed span. */ function transform(span) { - const technology = span.n; - - const transformFunction = loadMapper(technology); + const transformFunction = loadMapper(span.n); if (transformFunction) { return transformFunction(span); diff --git a/packages/core/src/util/filterSpan.js b/packages/core/src/util/filterSpan.js index ab9c7222ce..217d835cda 100644 --- a/packages/core/src/util/filterSpan.js +++ b/packages/core/src/util/filterSpan.js @@ -5,13 +5,8 @@ 'use strict'; // TODO: read from config -const config = { - tracing: { - ignoreEndpoints: { - redis: ['type'] - } - } -}; +// @ts-ignore +let config; /** * To check whether a span should be ignored based on the command. @@ -35,8 +30,10 @@ function shouldIgnore(span) { * @param {Object} span - The span object to check. * @returns {Object|null} - Returns the span if not ignored, or null if the span should be ignored. */ -function filterSpan(span) { - if (shouldIgnore(span)) { +function filterSpan(span, configuration = {}) { + // @ts-ignore + config = configuration || config; + if (config?.tracing?.ignoreEndpoints && shouldIgnore(span)) { return null; } return span; diff --git a/packages/core/test/tracing/backend_mappers/mapper_test.js b/packages/core/test/tracing/backend_mappers/mapper_test.js new file mode 100644 index 0000000000..3a2ace863c --- /dev/null +++ b/packages/core/test/tracing/backend_mappers/mapper_test.js @@ -0,0 +1,44 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +const expect = require('chai').expect; +const { transform: indexTransform } = require('../../../src/tracing/backend_mappers/index'); + +describe('Transformation Tests', () => { + let span; + + beforeEach(() => { + span = { n: 'redis', t: '1234567803', s: '1234567892', p: '1234567891', data: { redis: { operation: 'GET' } } }; + }); + + describe('index.js transform function', () => { + it('should transform span using the redis mapper for "redis"', () => { + const result = indexTransform(span); + expect(result.data.redis.command).equal('GET'); + expect(result.data.redis).to.not.have.property('operation'); + }); + it('should not modify other fields in the span', () => { + span = { data: { redis: { command: 'SET' }, otherField: 'value' } }; + + const result = indexTransform(span); + + expect(result.data.redis).to.not.have.property('operation'); + expect(result.data.redis.command).to.equal('SET'); + expect(result.data.otherField).to.equal('value'); + }); + + it('should return the span unmodified if no mapper is found', () => { + span.n = 'http'; + const result = indexTransform(span); + expect(result).to.equal(span); + }); + + it('should cache the mapper after the first load', () => { + indexTransform(span); + expect(indexTransform(span)).to.deep.equal(span); + }); + }); +}); diff --git a/packages/core/test/util/filterSpan_test.js b/packages/core/test/util/filterSpan_test.js new file mode 100644 index 0000000000..c187c6863a --- /dev/null +++ b/packages/core/test/util/filterSpan_test.js @@ -0,0 +1,64 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +const expect = require('chai').expect; +const { filterSpan } = require('../../src/util/filterSpan'); + +const span = { + t: '1234567803', + s: '1234567892', + p: '1234567891', + n: 'redis', + k: 2, + data: { + redis: { + command: '' + } + } +}; +let config = { + tracing: { + ignoreEndpoints: { + redis: ['GET', 'SET'] + } + } +}; + +describe('filterSpan', () => { + it('should return null when the span should be ignored', () => { + span.data.redis.command = 'GET'; + expect(filterSpan(span, config)).equal(null); + }); + + it('should return the span when it should not be ignored', () => { + span.data.redis.command = 'DEL'; + expect(filterSpan(span, config)).equal(span); + }); + + it('should return the span when command is not in the ignore list', () => { + span.data.redis.command = 'HGET'; + expect(filterSpan(span, config)).equal(span); + }); + + it('should return the span when span.n does not match any endpoint in config', () => { + const otherSpan = { + n: 'node.http.client', + data: { + http: { + command: 'GET' + } + } + }; + expect(filterSpan(otherSpan)).equal(otherSpan); + }); + it('should return span when no ignoreconfiguration', () => { + config = { + tracing: {} + }; + span.data.redis.command = 'GET'; + expect(filterSpan(span, config)).equal(span); + }); +}); From 984d8149050f3a6d43ea38ab714ebc97f3eb4b6c Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 15 Nov 2024 19:13:38 +0530 Subject: [PATCH 03/23] chore: tracing config available in spanBuffer --- packages/core/src/tracing/spanBuffer.js | 26 +++++++++++--- packages/core/src/util/filterSpan.js | 27 ++++++-------- packages/core/src/util/normalizeConfig.js | 4 +-- packages/core/test/util/filterSpan_test.js | 41 ++++++++-------------- 4 files changed, 47 insertions(+), 51 deletions(-) diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 9a3e01fac3..5bd3406966 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -89,6 +89,10 @@ const batchBucketWidth = 18; /** @type {BatchingBucketMap} */ const batchingBuckets = new Map(); +/** + * @type {Object} + */ +let ignoreEndpoints; /** * @param {import('../util/normalizeConfig').InstanaConfig} config @@ -103,6 +107,7 @@ exports.init = function init(config, _downstreamConnection) { initialDelayBeforeSendingSpans = Math.max(transmissionDelay, minDelayBeforeSendingSpans); isFaaS = false; transmitImmediate = false; + ignoreEndpoints = config.tracing?.ignoreEndpoints; if (config.tracing.activateImmediately) { preActivationCleanupIntervalHandle = setInterval(() => { @@ -132,6 +137,9 @@ exports.activate = function activate(extraConfig) { if (extraConfig && extraConfig.tracing && extraConfig.tracing.spanBatchingEnabled) { batchingEnabled = true; } + if (extraConfig?.tracing?.ignoreEndpoints) { + ignoreEndpoints = extraConfig.tracing.ignoreEndpoints; + } isActive = true; if (activatedAt == null) { @@ -489,15 +497,23 @@ function removeSpansIfNecessary() { spans = spans.slice(-maxBufferedSpans); } } -// @ts-ignore +/** + * @param {import('../core').InstanaBaseSpan} span + * @returns {import('../core').InstanaBaseSpan} span + */ function manageSpan(span) { if (!span || typeof span.n !== 'string') { return span; } - const filteredSpan = filterSpan(span); - if (!filteredSpan) { - return null; // Return null if span was ignored + // Currently only filter if ignoreEndpoint is configured + if (ignoreEndpoints) { + // @ts-ignore + span = filterSpan({ span, ignoreEndpoints }); + + if (!span) { + return null; + } } - return transform(filteredSpan); + return transform(span); } diff --git a/packages/core/src/util/filterSpan.js b/packages/core/src/util/filterSpan.js index 217d835cda..121a85b9f1 100644 --- a/packages/core/src/util/filterSpan.js +++ b/packages/core/src/util/filterSpan.js @@ -4,36 +4,29 @@ 'use strict'; -// TODO: read from config -// @ts-ignore -let config; - /** * To check whether a span should be ignored based on the command. * * @param {import('../core').InstanaBaseSpan} span - The span object to check. + * @param {Array} ignoreEndpoints - An array of endpoints to ignore. * @returns {boolean} - Returns true if the span should be ignored, false otherwise. */ -function shouldIgnore(span) { - // @ts-ignore - const ignoreEndpoints = config.tracing.ignoreEndpoints[span.n]; - - // @ts-ignore - if (span.data?.[span.n]?.command && ignoreEndpoints) { - return ignoreEndpoints.includes(span.data[span.n].command); +function shouldIgnore(span, ignoreEndpoints) { + if (span.data?.[span.n]?.operation && ignoreEndpoints) { + return ignoreEndpoints.includes(span.data[span.n].operation); } return false; } /** - * @param {Object} span - The span object to check. - * @returns {Object|null} - Returns the span if not ignored, or null if the span should be ignored. + * Filters a span based on ignore criteria. + * + * @param {{ span: import('../core').InstanaBaseSpan, ignoreEndpoints: { [key: string]: Array } }} params + * @returns {import('../core').InstanaBaseSpan | null} */ -function filterSpan(span, configuration = {}) { - // @ts-ignore - config = configuration || config; - if (config?.tracing?.ignoreEndpoints && shouldIgnore(span)) { +function filterSpan({ span, ignoreEndpoints }) { + if (ignoreEndpoints && shouldIgnore(span, ignoreEndpoints[span.n])) { return null; } return span; diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 1f56a91cd4..52db06a6b3 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -77,7 +77,7 @@ const constants = require('../tracing/constants'); * @property {AgentTracingHttpConfig} [http] * @property {AgentTracingKafkaConfig} [kafka] * @property {boolean|string} [spanBatchingEnabled] - * @property {Object} [AgentTracingIgnoreEndpoints] + * @property {Object} [ignoreEndpoints] */ /** @@ -91,7 +91,7 @@ const constants = require('../tracing/constants'); */ /** - * @typedef {Object} AgentTracingIgnoreEndpoints + * @typedef {Object} ignoreEndpoints * @property {Object.>} [endpoints] */ diff --git a/packages/core/test/util/filterSpan_test.js b/packages/core/test/util/filterSpan_test.js index c187c6863a..d23de031d9 100644 --- a/packages/core/test/util/filterSpan_test.js +++ b/packages/core/test/util/filterSpan_test.js @@ -15,50 +15,37 @@ const span = { k: 2, data: { redis: { - command: '' + operation: '' } } }; -let config = { - tracing: { - ignoreEndpoints: { - redis: ['GET', 'SET'] - } - } +let ignoreEndpoints = { + redis: ['GET', 'SET'] }; describe('filterSpan', () => { it('should return null when the span should be ignored', () => { - span.data.redis.command = 'GET'; - expect(filterSpan(span, config)).equal(null); + span.data.redis.operation = 'GET'; + expect(filterSpan({ span, ignoreEndpoints })).equal(null); }); it('should return the span when it should not be ignored', () => { - span.data.redis.command = 'DEL'; - expect(filterSpan(span, config)).equal(span); + span.data.redis.operation = 'DEL'; + expect(filterSpan({ span, ignoreEndpoints })).equal(span); }); it('should return the span when command is not in the ignore list', () => { - span.data.redis.command = 'HGET'; - expect(filterSpan(span, config)).equal(span); + span.data.redis.operation = 'HGET'; + expect(filterSpan({ span, ignoreEndpoints })).equal(span); }); it('should return the span when span.n does not match any endpoint in config', () => { - const otherSpan = { - n: 'node.http.client', - data: { - http: { - command: 'GET' - } - } - }; - expect(filterSpan(otherSpan)).equal(otherSpan); + span.n = 'node.http.client'; + expect(filterSpan({ span, ignoreEndpoints })).equal(span); }); it('should return span when no ignoreconfiguration', () => { - config = { - tracing: {} - }; - span.data.redis.command = 'GET'; - expect(filterSpan(span, config)).equal(span); + ignoreEndpoints = {}; + span.data.redis.operation = 'GET'; + expect(filterSpan({ span, ignoreEndpoints })).equal(span); }); }); From d4184a80210805c1da97f07938960da7f620bb97 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 15 Nov 2024 19:40:01 +0530 Subject: [PATCH 04/23] chore: fine tuning --- .../src/announceCycle/unannounced.js | 3 +-- .../core/src/tracing/backend_mappers/index.js | 8 ++++---- .../tracing/backend_mappers/redis_mapper.js | 7 ++----- .../instrumentation/database/ioredis.js | 4 ++-- packages/core/src/tracing/spanBuffer.js | 10 +++++++--- packages/core/src/util/filterSpan.js | 19 ++++++++++++------- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index cc73efec57..d7ecc98547 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -236,7 +236,7 @@ function applyIgnoreEndpointsConfiguration(agentResponse) { const endpointTracingConfig = {}; // eslint-disable-next-line no-restricted-syntax for (const [service, actions] of Object.entries(endpointTracingConfigFromAgent)) { - // From agent, the commands are separated by a pipe ('|'), split the actions into an array. + // From agent, the commands are separated by a pipe ('|') // @ts-ignore endpointTracingConfig[service.toLowerCase()] = // @ts-ignore @@ -244,7 +244,6 @@ function applyIgnoreEndpointsConfiguration(agentResponse) { } ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); - // @ts-ignore agentOpts.config.tracing.ignoreEndpoints = endpointTracingConfig; } } diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js index 801f817849..f14a1cf796 100644 --- a/packages/core/src/tracing/backend_mappers/index.js +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -12,8 +12,8 @@ const cachedMappers = {}; /** * Dynamically require the mapper based on span.n. * - * @param {string} spanName - The spanName name (e.g., 'redis', 'http'). - * @returns {Function|null} - The BE transformation function for the span.n + * @param {string} spanName + * @returns {Function|null} */ function loadMapper(spanName) { if (cachedMappers[spanName]) { @@ -31,8 +31,8 @@ function loadMapper(spanName) { } /** - * @param {import('../../core').InstanaBaseSpan} span - The span object that needs to be processed. - * @param {import('../../core').InstanaBaseSpan} span - The transformed span. + * @param {import('../../core').InstanaBaseSpan} span + * @param {import('../../core').InstanaBaseSpan} span . */ function transform(span) { const transformFunction = loadMapper(span.n); diff --git a/packages/core/src/tracing/backend_mappers/redis_mapper.js b/packages/core/src/tracing/backend_mappers/redis_mapper.js index 5ae4ecc88c..4d103e3772 100644 --- a/packages/core/src/tracing/backend_mappers/redis_mapper.js +++ b/packages/core/src/tracing/backend_mappers/redis_mapper.js @@ -10,11 +10,8 @@ const fieldMappings = { }; /** - * Transform the span object by renaming the Redis fields based on defined mappings. - * - * @param {Object} span - The span object containing Redis data to transform. - * @param {boolean} log - Flag to enable/disable logging of transformations. - * @returns {Object} - The transformed span object. + * @param {Object} span + * @returns {Object} */ function transform(span) { if (span.data?.redis) { diff --git a/packages/core/src/tracing/instrumentation/database/ioredis.js b/packages/core/src/tracing/instrumentation/database/ioredis.js index 204849d5d7..23987cc67d 100644 --- a/packages/core/src/tracing/instrumentation/database/ioredis.js +++ b/packages/core/src/tracing/instrumentation/database/ioredis.js @@ -88,7 +88,7 @@ function instrumentSendCommand(original) { span.data.redis = { connection, - command: command.name.toLowerCase() + operation: command.name.toLowerCase() }; callback = cls.ns.bind(onResult); @@ -157,7 +157,7 @@ function instrumentMultiOrPipelineCommand(commandName, original) { span.stack = tracingUtil.getStackTrace(wrappedInternalMultiOrPipelineCommand); span.data.redis = { connection, - command: commandName + operation: commandName }; const multiOrPipeline = original.apply(this, arguments); diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 5bd3406966..a4969014ec 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -189,9 +189,13 @@ exports.addSpan = function (span) { return; } - // transform the spans - // filter the spans - span = manageSpan(span); + // Transform the span + const processedSpan = manageSpan(span); + if (!processedSpan) { + logger.warn('Span of type %s has no trace ID. Not transmitting this span', span.n); + return; + } + span = processedSpan; if (span.t == null) { logger.warn('Span of type %s has no trace ID. Not transmitting this span', span.n); diff --git a/packages/core/src/util/filterSpan.js b/packages/core/src/util/filterSpan.js index 121a85b9f1..4bf9c816a6 100644 --- a/packages/core/src/util/filterSpan.js +++ b/packages/core/src/util/filterSpan.js @@ -4,12 +4,15 @@ 'use strict'; +/** @type {import('../core').GenericLogger} */ +let logger; +logger = require('../logger').getLogger('tracing/util/filterSpan', newLogger => { + logger = newLogger; +}); /** - * To check whether a span should be ignored based on the command. - * - * @param {import('../core').InstanaBaseSpan} span - The span object to check. - * @param {Array} ignoreEndpoints - An array of endpoints to ignore. - * @returns {boolean} - Returns true if the span should be ignored, false otherwise. + * @param {import('../core').InstanaBaseSpan} span + * @param {Array} ignoreEndpoints + * @returns {boolean} */ function shouldIgnore(span, ignoreEndpoints) { if (span.data?.[span.n]?.operation && ignoreEndpoints) { @@ -20,13 +23,15 @@ function shouldIgnore(span, ignoreEndpoints) { } /** - * Filters a span based on ignore criteria. - * * @param {{ span: import('../core').InstanaBaseSpan, ignoreEndpoints: { [key: string]: Array } }} params * @returns {import('../core').InstanaBaseSpan | null} */ function filterSpan({ span, ignoreEndpoints }) { if (ignoreEndpoints && shouldIgnore(span, ignoreEndpoints[span.n])) { + logger.debug('Span ignored due to matching ignore endpoint', { + spanType: span.n, + ignoredOperation: span.data[span.n]?.operation + }); return null; } return span; From c919b13e78ebe74a4cd5688deb54d761b32b1f43 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 11:01:39 +0530 Subject: [PATCH 05/23] test: added more tests --- packages/collector/test/apps/agentStub.js | 7 +- .../collector/test/apps/agentStubControls.js | 3 + .../test/tracing/database/ioredis/app.js | 13 +- .../test/tracing/database/ioredis/test.js | 54 +++++ .../test/tracing/database/redis/test.js | 226 +++++++++++++----- .../instrumentation/database/ioredis.js | 2 +- 6 files changed, 243 insertions(+), 62 deletions(-) diff --git a/packages/collector/test/apps/agentStub.js b/packages/collector/test/apps/agentStub.js index dd0269484c..d3e612424d 100644 --- a/packages/collector/test/apps/agentStub.js +++ b/packages/collector/test/apps/agentStub.js @@ -38,6 +38,8 @@ const enableSpanBatching = process.env.ENABLE_SPANBATCHING === 'true'; const kafkaTraceCorrelation = process.env.KAFKA_TRACE_CORRELATION ? process.env.KAFKA_TRACE_CORRELATION === 'true' : null; +const ignoreRedisEndpoints = + process.env.INSTANA_IGNORE_ENDPOINTS_REDIS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS_REDIS); let discoveries = {}; let rejectAnnounceAttempts = 0; @@ -104,7 +106,10 @@ app.put('/com.instana.plugin.nodejs.discovery', (req, res) => { response.tracing['span-batching-enabled'] = true; } } - + if (ignoreRedisEndpoints != null) { + response.tracing = {}; + response.tracing['ignore-endpoints'] = ignoreRedisEndpoints; + } res.send(response); }); diff --git a/packages/collector/test/apps/agentStubControls.js b/packages/collector/test/apps/agentStubControls.js index eee7a6c027..23b8ae4690 100644 --- a/packages/collector/test/apps/agentStubControls.js +++ b/packages/collector/test/apps/agentStubControls.js @@ -43,6 +43,9 @@ class AgentStubControls { env.KAFKA_TRACE_CORRELATION = opts.kafkaConfig.traceCorrelation.toString(); } } + if (opts?.ignoreEndpoints?.redis) { + env.INSTANA_IGNORE_ENDPOINTS_REDIS = JSON.stringify(opts.ignoreEndpoints); + } this.agentStub = spawn('node', [path.join(__dirname, 'agentStub.js')], { stdio: config.getAppStdio(), diff --git a/packages/collector/test/tracing/database/ioredis/app.js b/packages/collector/test/tracing/database/ioredis/app.js index 6ed89a6ef7..8db0f2a5d9 100644 --- a/packages/collector/test/tracing/database/ioredis/app.js +++ b/packages/collector/test/tracing/database/ioredis/app.js @@ -15,7 +15,18 @@ process.on('SIGTERM', () => { const agentPort = process.env.INSTANA_AGENT_PORT; -require('../../../..')(); +const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; +if (!ignoreEndpointsEnabled) { + require('../../../..')(); +} else { + require('../../../..')({ + tracing: { + ignoreEndpoints: { + redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + } + } + }); +} const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index 463eacece1..9ab55b54eb 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1238,7 +1238,61 @@ function checkConnection(span, setupType) { expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); + it('should not create Redis spans for commands listed in the ignoredEndpoints', async () => { + const ignoreControls = new ProcessControls({ + useGlobalAgent: true, + dirname: __dirname, + env: { + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + } + }); + + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'price', + value: 42 + } + }) + .then(() => + ignoreControls.sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + ) + .then(async response => { + expect(String(response)).to.equal('42'); + + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); + }) + .finally(async () => { + await ignoreControls.stop(); + }); + }); if (setupType !== 'cluster') { it('call two different hosts/clients', async () => { const response = await controls.sendRequest({ diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index ad116826f8..3b5b724eb4 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -199,65 +199,6 @@ const globalAgent = require('../../../globalAgent'); }) ); })); - // TODO - it.skip('should not create spans for ignored commands via ignoredEndpoints', async () => { - const ignoreControls = new ProcessControls({ - useGlobalAgent: true, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get', 'set']) - } - }); - - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - - await ignoreControls - .sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'price', - value: 42 - } - }) - .then(() => - ignoreControls.sendRequest({ - method: 'GET', - path: '/values', - qs: { - key: 'price' - } - }) - ) - .then(async response => { - expect(String(response)).to.equal('42'); - - return retry(async () => { - const spans = await agentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('POST') - ]); - - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('GET') - ]); - }); - }) - .finally(async () => { - await ignoreControls.stop(); - }); - }); - it('must trace hset/hget calls', () => controls .sendRequest({ @@ -868,6 +809,173 @@ const globalAgent = require('../../../globalAgent'); expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); + it('should not create Redis spans for commands listed in the ignoredEndpoints', async () => { + const ignoreControls = new ProcessControls({ + useGlobalAgent: true, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + } + }); + + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'price', + value: 42 + } + }) + .then(() => + ignoreControls.sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + ) + .then(async response => { + expect(String(response)).to.equal('42'); + + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); + }) + .finally(async () => { + await ignoreControls.stop(); + }); + }); + + it('should not create Redis spans for the ignored "set" command', async () => { + const ignoreControls = new ProcessControls({ + useGlobalAgent: true, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['set']) + } + }); + + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }) + .then(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).to.not.equal('redis'); + }); + }); + }); + + it('should create spans for non-ignored Redis commands', async () => { + const ignoreControls = new ProcessControls({ + useGlobalAgent: true, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + } + }); + + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + + await ignoreControls + .sendRequest({ + method: 'GET', + path: '/hset-hget' + }) + .then(async () => { + const spans = await agentControls.getSpans(); + + expect(spans.some(span => span.n === 'redis')).to.be.true; + }); + }); + describe('ignore-endpoints enabled via agent config', function () { + const { AgentStubControls } = require('../../../apps/agentStubControls'); + const customAgentControls = new AgentStubControls(); + let ignoreControls; + before(async () => { + await customAgentControls.startAgent({ + ignoreEndpoints: { redis: 'type|set' } + }); + + ignoreControls = new ProcessControls({ + agentControls: customAgentControls, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster' + } + }); + + await ignoreControls.start(); + }); + + beforeEach(async () => { + await customAgentControls.clearReceivedTraceData(); + }); + + after(async () => { + await ignoreControls.stop(); + await customAgentControls.stopAgent(); + }); + + it('should ignore redis spans when configured to ignore endpoints', async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }); + + const spans = await customAgentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); + }); // Does not make sense for cluster. if (setupType !== 'cluster') { diff --git a/packages/core/src/tracing/instrumentation/database/ioredis.js b/packages/core/src/tracing/instrumentation/database/ioredis.js index 23987cc67d..bc83cbe082 100644 --- a/packages/core/src/tracing/instrumentation/database/ioredis.js +++ b/packages/core/src/tracing/instrumentation/database/ioredis.js @@ -61,7 +61,7 @@ function instrumentSendCommand(original) { if ( parentSpan && parentSpan.n === exports.spanName && - (parentSpan.data.redis.command === 'multi' || parentSpan.data.redis.command === 'pipeline') && + (parentSpan.data.redis.operation === 'multi' || parentSpan.data.redis.operation === 'pipeline') && command.name !== 'multi' ) { const parentSpanSubCommands = (parentSpan.data.redis.subCommands = parentSpan.data.redis.subCommands || []); From bb2bd3f9f2be06e46590e32cab9785626caaab97 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 13:00:34 +0530 Subject: [PATCH 06/23] test: improved tests --- .../test/announceCycle/unannounced_test.js | 6 +-- packages/collector/test/apps/agentStub.js | 3 +- .../collector/test/apps/agentStubControls.js | 4 +- .../test/tracing/database/ioredis/test.js | 2 +- .../test/tracing/database/redis/test.js | 13 +++--- packages/core/src/tracing/spanBuffer.js | 4 ++ .../tracing/backend_mappers/mapper_test.js | 21 +++++---- packages/core/test/tracing/spanBuffer_test.js | 45 +++++++++++++++++++ packages/core/test/util/filterSpan_test.js | 9 +--- .../core/test/util/normalizeConfig_test.js | 6 +-- 10 files changed, 79 insertions(+), 34 deletions(-) diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 50852d6697..06c644d1a6 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -213,7 +213,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the configuration to ignore specified endpoints for a single module', done => { + it('should apply the configuration to ignore specified endpoints for a single package', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { @@ -234,11 +234,11 @@ describe('unannounced state', () => { } }); }); - it('should apply the tracing configuration to ignore multiple endpoints across different modules', done => { + it('should apply the tracing configuration to ignore multiple endpoints across different packages', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { - REDIS: 'get | type', + REDIS: 'get|type', dynamodb: 'query' } } diff --git a/packages/collector/test/apps/agentStub.js b/packages/collector/test/apps/agentStub.js index d3e612424d..a152666593 100644 --- a/packages/collector/test/apps/agentStub.js +++ b/packages/collector/test/apps/agentStub.js @@ -38,8 +38,7 @@ const enableSpanBatching = process.env.ENABLE_SPANBATCHING === 'true'; const kafkaTraceCorrelation = process.env.KAFKA_TRACE_CORRELATION ? process.env.KAFKA_TRACE_CORRELATION === 'true' : null; -const ignoreRedisEndpoints = - process.env.INSTANA_IGNORE_ENDPOINTS_REDIS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS_REDIS); +const ignoreRedisEndpoints = process.env.INSTANA_IGNORE_ENDPOINTS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); let discoveries = {}; let rejectAnnounceAttempts = 0; diff --git a/packages/collector/test/apps/agentStubControls.js b/packages/collector/test/apps/agentStubControls.js index 23b8ae4690..e3049a9afd 100644 --- a/packages/collector/test/apps/agentStubControls.js +++ b/packages/collector/test/apps/agentStubControls.js @@ -43,8 +43,8 @@ class AgentStubControls { env.KAFKA_TRACE_CORRELATION = opts.kafkaConfig.traceCorrelation.toString(); } } - if (opts?.ignoreEndpoints?.redis) { - env.INSTANA_IGNORE_ENDPOINTS_REDIS = JSON.stringify(opts.ignoreEndpoints); + if (opts?.ignoreEndpoints) { + env.INSTANA_IGNORE_ENDPOINTS = JSON.stringify(opts.ignoreEndpoints); } this.agentStub = spawn('node', [path.join(__dirname, 'agentStub.js')], { diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index 9ab55b54eb..cc65642f2b 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1238,7 +1238,7 @@ function checkConnection(span, setupType) { expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); - it('should not create Redis spans for commands listed in the ignoredEndpoints', async () => { + it('should not create redis spans for commands listed in the ignoredEndpoints', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, dirname: __dirname, diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 3b5b724eb4..2ba3346644 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -809,7 +809,7 @@ const globalAgent = require('../../../globalAgent'); expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); - it('should not create Redis spans for commands listed in the ignoredEndpoints', async () => { + it('should not create redis spans for commands listed in the ignoredEndpoints', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -868,7 +868,7 @@ const globalAgent = require('../../../globalAgent'); }); }); - it('should not create Redis spans for the ignored "set" command', async () => { + it('should not create redis spans for the ignored "set" command', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -928,7 +928,7 @@ const globalAgent = require('../../../globalAgent'); expect(spans.some(span => span.n === 'redis')).to.be.true; }); }); - describe('ignore-endpoints enabled via agent config', function () { + mochaSuiteFn('ignore-endpoints enabled via agent config', function () { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); let ignoreControls; @@ -957,10 +957,13 @@ const globalAgent = require('../../../globalAgent'); after(async () => { await ignoreControls.stop(); - await customAgentControls.stopAgent(); + customAgentControls.stopAgent(); + }); + afterEach(async () => { + await ignoreControls.clearIpcMessages(); }); - it('should ignore redis spans when configured to ignore endpoints', async () => { + it('should ignore redis spans for configured ignore endpoints', async () => { await ignoreControls.sendRequest({ method: 'POST', path: '/values', diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index a4969014ec..402c315ec5 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -521,3 +521,7 @@ function manageSpan(span) { } return transform(span); } +// export the manageSpan function for use in test. +if (process.env.NODE_ENV === 'test') { + module.exports.manageSpan = manageSpan; +} diff --git a/packages/core/test/tracing/backend_mappers/mapper_test.js b/packages/core/test/tracing/backend_mappers/mapper_test.js index 3a2ace863c..69214b15b9 100644 --- a/packages/core/test/tracing/backend_mappers/mapper_test.js +++ b/packages/core/test/tracing/backend_mappers/mapper_test.js @@ -5,26 +5,25 @@ 'use strict'; const expect = require('chai').expect; -const { transform: indexTransform } = require('../../../src/tracing/backend_mappers/index'); +const { transform } = require('../../../src/tracing/backend_mappers/index'); -describe('Transformation Tests', () => { +describe('BE span transformation test', () => { let span; beforeEach(() => { span = { n: 'redis', t: '1234567803', s: '1234567892', p: '1234567891', data: { redis: { operation: 'GET' } } }; }); - describe('index.js transform function', () => { - it('should transform span using the redis mapper for "redis"', () => { - const result = indexTransform(span); + describe('should invoke transform function', () => { + it('should transform redis span using the redis mapper', () => { + const result = transform(span); expect(result.data.redis.command).equal('GET'); expect(result.data.redis).to.not.have.property('operation'); }); - it('should not modify other fields in the span', () => { + it('should not modify fields that need not be transformed in the redis span', () => { span = { data: { redis: { command: 'SET' }, otherField: 'value' } }; - const result = indexTransform(span); - + const result = transform(span); expect(result.data.redis).to.not.have.property('operation'); expect(result.data.redis.command).to.equal('SET'); expect(result.data.otherField).to.equal('value'); @@ -32,13 +31,13 @@ describe('Transformation Tests', () => { it('should return the span unmodified if no mapper is found', () => { span.n = 'http'; - const result = indexTransform(span); + const result = transform(span); expect(result).to.equal(span); }); it('should cache the mapper after the first load', () => { - indexTransform(span); - expect(indexTransform(span)).to.deep.equal(span); + transform(span); + expect(transform(span)).to.deep.equal(span); }); }); }); diff --git a/packages/core/test/tracing/spanBuffer_test.js b/packages/core/test/tracing/spanBuffer_test.js index f31bbb6ce0..4ac0f550cc 100644 --- a/packages/core/test/tracing/spanBuffer_test.js +++ b/packages/core/test/tracing/spanBuffer_test.js @@ -566,6 +566,51 @@ describe('tracing/spanBuffer', () => { verifyNoBatching(span1, span2); }); }); + describe('manageSpan test', () => { + before(() => { + spanBuffer.init( + { + tracing: { + ignoreEndpoints: { redis: ['get'] } + } + }, + { + /* downstreamConnection */ + sendSpans: function () {} + } + ); + }); + + beforeEach(() => spanBuffer.activate()); + + afterEach(() => spanBuffer.deactivate()); + const span = { + t: '1234567803', + s: '1234567892', + p: '1234567891', + n: 'redis', + k: 2, + data: { + redis: { + operation: 'get' + } + } + }; + it('should return the span if span is not valid', () => { + expect(spanBuffer.manageSpan(null)).to.equal(null); + }); + + it('should filter out the span when command is listed in ignoreEndpoints config', () => { + expect(spanBuffer.manageSpan(span)).to.equal(null); + }); + + it('should transform and return the span for command not specified in ignoreEndpoints config', () => { + span.data.redis.operation = 'set'; + const result = spanBuffer.manageSpan(span); + expect(result.data.redis.command).to.equal('set'); + expect(result.data.redis).to.not.have.property('operation'); + }); + }); }); function timestamp(offset) { diff --git a/packages/core/test/util/filterSpan_test.js b/packages/core/test/util/filterSpan_test.js index d23de031d9..0d41932529 100644 --- a/packages/core/test/util/filterSpan_test.js +++ b/packages/core/test/util/filterSpan_test.js @@ -20,20 +20,15 @@ const span = { } }; let ignoreEndpoints = { - redis: ['GET', 'SET'] + redis: ['GET', 'TYPE'] }; -describe('filterSpan', () => { +describe('util.filterSpan', () => { it('should return null when the span should be ignored', () => { span.data.redis.operation = 'GET'; expect(filterSpan({ span, ignoreEndpoints })).equal(null); }); - it('should return the span when it should not be ignored', () => { - span.data.redis.operation = 'DEL'; - expect(filterSpan({ span, ignoreEndpoints })).equal(span); - }); - it('should return the span when command is not in the ignore list', () => { span.data.redis.operation = 'HGET'; expect(filterSpan({ span, ignoreEndpoints })).equal(span); diff --git a/packages/core/test/util/normalizeConfig_test.js b/packages/core/test/util/normalizeConfig_test.js index df405cf0e3..383ebf3bf9 100644 --- a/packages/core/test/util/normalizeConfig_test.js +++ b/packages/core/test/util/normalizeConfig_test.js @@ -471,12 +471,12 @@ describe('util.normalizeConfig', () => { const config = normalizeConfig(); expect(config.tracing.allowRootExitSpan).to.equal(true); }); - it('should not disable ignore endpoints tracers by default', () => { + it('should not set ignore endpoints tracers by default', () => { const config = normalizeConfig(); expect(config.tracing.ignoreEndpoints).to.deep.equal({}); }); - it('should apply ignore endpoints tracers via config', () => { + it('should apply ignore endpoints via config', () => { const config = normalizeConfig({ tracing: { ignoreEndpoints: { redis: ['get'] } @@ -485,7 +485,7 @@ describe('util.normalizeConfig', () => { expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'] }); }); - it('should apply ignore endpoints tracers via config', () => { + it('should apply ignore endpoints via config for multiple packages', () => { const config = normalizeConfig({ tracing: { ignoreEndpoints: { redis: ['get'], dynamodb: ['querey'] } From e5113ac23df1539bd9de1aa9592a238b2dd3b530 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 13:53:02 +0530 Subject: [PATCH 07/23] test: skipping the test --- packages/collector/test/tracing/database/redis/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 2ba3346644..730e10f0aa 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -928,7 +928,7 @@ const globalAgent = require('../../../globalAgent'); expect(spans.some(span => span.n === 'redis')).to.be.true; }); }); - mochaSuiteFn('ignore-endpoints enabled via agent config', function () { + describe.skip('ignore-endpoints enabled via agent config', function () { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); let ignoreControls; From 8b2edc4e62cdad31b9c4a4a8f3e27cfa1af8ed1d Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 16:42:15 +0530 Subject: [PATCH 08/23] test: skip tests --- .../test/tracing/database/redis/test.js | 104 +++++++++--------- 1 file changed, 50 insertions(+), 54 deletions(-) diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 730e10f0aa..a48b0c817f 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -809,7 +809,7 @@ const globalAgent = require('../../../globalAgent'); expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); - it('should not create redis spans for commands listed in the ignoredEndpoints', async () => { + it.skip('should not create redis spans for commands listed in the ignoredEndpoints', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -868,7 +868,7 @@ const globalAgent = require('../../../globalAgent'); }); }); - it('should not create redis spans for the ignored "set" command', async () => { + it.skip('should not create redis spans for the ignored "set" command', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -901,7 +901,7 @@ const globalAgent = require('../../../globalAgent'); }); }); - it('should create spans for non-ignored Redis commands', async () => { + it.skip('should create spans for non-ignored Redis commands', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -928,57 +928,6 @@ const globalAgent = require('../../../globalAgent'); expect(spans.some(span => span.n === 'redis')).to.be.true; }); }); - describe.skip('ignore-endpoints enabled via agent config', function () { - const { AgentStubControls } = require('../../../apps/agentStubControls'); - const customAgentControls = new AgentStubControls(); - let ignoreControls; - before(async () => { - await customAgentControls.startAgent({ - ignoreEndpoints: { redis: 'type|set' } - }); - - ignoreControls = new ProcessControls({ - agentControls: customAgentControls, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster' - } - }); - - await ignoreControls.start(); - }); - - beforeEach(async () => { - await customAgentControls.clearReceivedTraceData(); - }); - - after(async () => { - await ignoreControls.stop(); - customAgentControls.stopAgent(); - }); - afterEach(async () => { - await ignoreControls.clearIpcMessages(); - }); - - it('should ignore redis spans for configured ignore endpoints', async () => { - await ignoreControls.sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'discount', - value: 50 - } - }); - - const spans = await customAgentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); - }); - }); // Does not make sense for cluster. if (setupType !== 'cluster') { @@ -1021,6 +970,53 @@ const globalAgent = require('../../../globalAgent'); }); } }); + mochaSuiteFn('ignore-endpoints enabled via agent config', function () { + const { AgentStubControls } = require('../../../apps/agentStubControls'); + const customAgentControls = new AgentStubControls(); + let ignoreControls; + before(async () => { + await customAgentControls.startAgent({ + ignoreEndpoints: { redis: 'type|set' } + }); + + ignoreControls = new ProcessControls({ + agentControls: customAgentControls, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster' + } + }); + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + }); + + beforeEach(async () => { + await customAgentControls.clearReceivedTraceData(); + }); + + after(async () => { + await customAgentControls.stopAgent(); + await ignoreControls.stop(); + }); + + it('should ignore redis spans for configured ignore endpoints', async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }); + + const spans = await customAgentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); + }); }); function verifyHttpExit(controls, spans, parent) { From 5a0b4f9627d3c3a52563e5b34251848c9a858468 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 17:27:04 +0530 Subject: [PATCH 09/23] test: fine tuning --- packages/collector/test/tracing/database/redis/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index a48b0c817f..77922c83f1 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -868,7 +868,7 @@ const globalAgent = require('../../../globalAgent'); }); }); - it.skip('should not create redis spans for the ignored "set" command', async () => { + it('should not create redis spans for the ignored "set" command', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: From ed458988f1b42c4f3354cc3b9ff1c024889922c0 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 17:31:39 +0530 Subject: [PATCH 10/23] test: fine tuning --- .../core/src/tracing/backend_mappers/index.js | 43 +---------------- .../src/tracing/backend_mappers/mapper.js | 47 +++++++++++++++++++ .../tracing/backend_mappers/mapper_test.js | 2 +- 3 files changed, 50 insertions(+), 42 deletions(-) create mode 100644 packages/core/src/tracing/backend_mappers/mapper.js diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js index f14a1cf796..22fcccfb12 100644 --- a/packages/core/src/tracing/backend_mappers/index.js +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -4,44 +4,5 @@ 'use strict'; -/** - * @type {Record} - */ -const cachedMappers = {}; - -/** - * Dynamically require the mapper based on span.n. - * - * @param {string} spanName - * @returns {Function|null} - */ -function loadMapper(spanName) { - if (cachedMappers[spanName]) { - return cachedMappers[spanName]; - } - - try { - // While adding a new mapper file, always expected in the same format. - const mapper = require(`./${spanName}_mapper.js`).transform; - cachedMappers[spanName] = mapper; - return mapper; - } catch (err) { - return null; - } -} - -/** - * @param {import('../../core').InstanaBaseSpan} span - * @param {import('../../core').InstanaBaseSpan} span . - */ -function transform(span) { - const transformFunction = loadMapper(span.n); - - if (transformFunction) { - return transformFunction(span); - } else { - return span; - } -} - -module.exports = { transform }; +// Entry point for the mappers module. +module.exports = require('./mapper'); diff --git a/packages/core/src/tracing/backend_mappers/mapper.js b/packages/core/src/tracing/backend_mappers/mapper.js new file mode 100644 index 0000000000..0297afd755 --- /dev/null +++ b/packages/core/src/tracing/backend_mappers/mapper.js @@ -0,0 +1,47 @@ +/* + * (c) Copyright IBM Corp. 2024 + */ + +'use strict'; + +/** + * @type {Record} + */ +const cachedMappers = {}; + +/** + * Dynamically require the mapper based on span.n. + * + * @param {string} spanName + * @returns {Function|null} + */ +function loadMapper(spanName) { + if (cachedMappers[spanName]) { + return cachedMappers[spanName]; + } + + try { + // Ensures mapper files follow the same naming convention. + const mapper = require(`./${spanName}_mapper.js`).transform; + cachedMappers[spanName] = mapper; + return mapper; + } catch (err) { + return null; + } +} + +/** + * @param {import('../../core').InstanaBaseSpan} span + * @returns {import('../../core').InstanaBaseSpan} + */ +function transform(span) { + const transformFunction = loadMapper(span.n); + + if (transformFunction) { + return transformFunction(span); + } else { + return span; + } +} + +module.exports = { transform }; diff --git a/packages/core/test/tracing/backend_mappers/mapper_test.js b/packages/core/test/tracing/backend_mappers/mapper_test.js index 69214b15b9..2e1f9bb0d4 100644 --- a/packages/core/test/tracing/backend_mappers/mapper_test.js +++ b/packages/core/test/tracing/backend_mappers/mapper_test.js @@ -5,7 +5,7 @@ 'use strict'; const expect = require('chai').expect; -const { transform } = require('../../../src/tracing/backend_mappers/index'); +const { transform } = require('../../../src/tracing/backend_mappers'); describe('BE span transformation test', () => { let span; From 1a75189a9fc7f697bc0ca4261ba19568b8068de2 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 18:59:30 +0530 Subject: [PATCH 11/23] test: fixing the failing tests --- .../test/tracing/database/redis/test.js | 152 +++++++++++++----- 1 file changed, 116 insertions(+), 36 deletions(-) diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 77922c83f1..a28d50ba86 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -868,7 +868,7 @@ const globalAgent = require('../../../globalAgent'); }); }); - it('should not create redis spans for the ignored "set" command', async () => { + it.skip('should not create redis spans for the ignored "set" command', async () => { const ignoreControls = new ProcessControls({ useGlobalAgent: true, appPath: @@ -970,51 +970,131 @@ const globalAgent = require('../../../globalAgent'); }); } }); - mochaSuiteFn('ignore-endpoints enabled via agent config', function () { - const { AgentStubControls } = require('../../../apps/agentStubControls'); - const customAgentControls = new AgentStubControls(); - let ignoreControls; - before(async () => { - await customAgentControls.startAgent({ - ignoreEndpoints: { redis: 'type|set' } + mochaSuiteFn('ignore-endpoints config', function () { + describe('ignore-endpoints enabled via agent config', () => { + const { AgentStubControls } = require('../../../apps/agentStubControls'); + const customAgentControls = new AgentStubControls(); + let ignoreControls; + before(async () => { + await customAgentControls.startAgent({ + ignoreEndpoints: { redis: 'type|set' } + }); + + ignoreControls = new ProcessControls({ + agentControls: customAgentControls, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster' + } + }); + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); }); - ignoreControls = new ProcessControls({ - agentControls: customAgentControls, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster' - } + beforeEach(async () => { + await customAgentControls.clearReceivedTraceData(); }); - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - }); - beforeEach(async () => { - await customAgentControls.clearReceivedTraceData(); - }); + after(async () => { + await customAgentControls.stopAgent(); + await ignoreControls.stop(); + }); - after(async () => { - await customAgentControls.stopAgent(); - await ignoreControls.stop(); + it('should ignore redis spans for configured ignore endpoints', async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }); + + const spans = await customAgentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); }); + describe('ignore-endpoints enabled via tracing config', async () => { + globalAgent.setUpCleanUpHooks(); + let ignoreControls; - it('should ignore redis spans for configured ignore endpoints', async () => { - await ignoreControls.sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'discount', - value: 50 - } + before(async () => { + ignoreControls = new ProcessControls({ + useGlobalAgent: true, + appPath: + redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), + env: { + REDIS_VERSION: redisVersion, + REDIS_PKG: redisPkg, + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + } + }); + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); }); - const spans = await customAgentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); + beforeEach(async () => { + await agentControls.clearReceivedTraceData(); }); + + before(async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/clearkeys' + }); + }); + + after(async () => { + await ignoreControls.stop(); + }); + + afterEach(async () => { + await ignoreControls.clearIpcMessages(); + }); + + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'price', + value: 42 + } + }) + .then(() => + ignoreControls.sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + ) + .then(async response => { + expect(String(response)).to.equal('42'); + + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); + }); }); }); }); From 7c7eea60b693c13bcd430be965e41a09c972fca8 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 19:50:21 +0530 Subject: [PATCH 12/23] test: improved the redis test --- .../test/tracing/database/redis/test.js | 198 +++++------------- 1 file changed, 47 insertions(+), 151 deletions(-) diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index a28d50ba86..b327cd722a 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -809,125 +809,6 @@ const globalAgent = require('../../../globalAgent'); expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); - it.skip('should not create redis spans for commands listed in the ignoredEndpoints', async () => { - const ignoreControls = new ProcessControls({ - useGlobalAgent: true, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get', 'set']) - } - }); - - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - - await ignoreControls - .sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'price', - value: 42 - } - }) - .then(() => - ignoreControls.sendRequest({ - method: 'GET', - path: '/values', - qs: { - key: 'price' - } - }) - ) - .then(async response => { - expect(String(response)).to.equal('42'); - - return retry(async () => { - const spans = await agentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); - - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('POST') - ]); - - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('GET') - ]); - }); - }) - .finally(async () => { - await ignoreControls.stop(); - }); - }); - - it.skip('should not create redis spans for the ignored "set" command', async () => { - const ignoreControls = new ProcessControls({ - useGlobalAgent: true, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['set']) - } - }); - - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - - await ignoreControls - .sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'discount', - value: 50 - } - }) - .then(async () => { - const spans = await agentControls.getSpans(); - spans.forEach(span => { - expect(span.n).to.not.equal('redis'); - }); - }); - }); - - it.skip('should create spans for non-ignored Redis commands', async () => { - const ignoreControls = new ProcessControls({ - useGlobalAgent: true, - appPath: - redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), - env: { - REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get', 'set']) - } - }); - - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - - await ignoreControls - .sendRequest({ - method: 'GET', - path: '/hset-hget' - }) - .then(async () => { - const spans = await agentControls.getSpans(); - - expect(spans.some(span => span.n === 'redis')).to.be.true; - }); - }); // Does not make sense for cluster. if (setupType !== 'cluster') { @@ -970,7 +851,7 @@ const globalAgent = require('../../../globalAgent'); }); } }); - mochaSuiteFn('ignore-endpoints config', function () { + mochaSuiteFn('ignore-endpoints tests', function () { describe('ignore-endpoints enabled via agent config', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); @@ -1056,45 +937,60 @@ const globalAgent = require('../../../globalAgent'); afterEach(async () => { await ignoreControls.clearIpcMessages(); }); - - await ignoreControls - .sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'price', - value: 42 - } - }) - .then(() => - ignoreControls.sendRequest({ - method: 'GET', + it('should not create redis spans for the ignored commands', async function () { + await ignoreControls + .sendRequest({ + method: 'POST', path: '/values', qs: { - key: 'price' + key: 'price', + value: 42 } }) - ) - .then(async response => { - expect(String(response)).to.equal('42'); + .then(() => + ignoreControls.sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + ) + .then(async response => { + expect(String(response)).to.equal('42'); - return retry(async () => { - const spans = await agentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('POST') - ]); + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('GET') - ]); + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); }); - }); + }); + it('should create spans for non-ignored Redis commands', async () => { + await ignoreControls + .sendRequest({ + method: 'GET', + path: '/hset-hget' + }) + .then(async () => { + return retry(async () => { + const spans = await agentControls.getSpans(); + + expect(spans.some(span => span.n === 'redis')).to.be.true; + }); + }); + }); }); }); }); From 8d661ca06c6d5283d036f72316a2ff3bfc0d1d16 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 18 Nov 2024 19:58:19 +0530 Subject: [PATCH 13/23] test: improved the ioredis test --- .../test/tracing/database/ioredis/test.js | 187 +++++++++++++----- 1 file changed, 133 insertions(+), 54 deletions(-) diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index cc65642f2b..6fba0a32bf 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1238,61 +1238,7 @@ function checkConnection(span, setupType) { expect.fail(`Unexpected spans: ${stringifyItems(spans)}`); } }); - it('should not create redis spans for commands listed in the ignoredEndpoints', async () => { - const ignoreControls = new ProcessControls({ - useGlobalAgent: true, - dirname: __dirname, - env: { - REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get', 'set']) - } - }); - - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); - - await ignoreControls - .sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'price', - value: 42 - } - }) - .then(() => - ignoreControls.sendRequest({ - method: 'GET', - path: '/values', - qs: { - key: 'price' - } - }) - ) - .then(async response => { - expect(String(response)).to.equal('42'); - - return retry(async () => { - const spans = await agentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); - - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('POST') - ]); - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('GET') - ]); - }); - }) - .finally(async () => { - await ignoreControls.stop(); - }); - }); if (setupType !== 'cluster') { it('call two different hosts/clients', async () => { const response = await controls.sendRequest({ @@ -1335,4 +1281,137 @@ function checkConnection(span, setupType) { }); } }); + mochaSuiteFn('ignore-endpoints tests', function () { + this.timeout(config.getTestTimeout() * 4); + describe('ignore-endpoints enabled via agent config', () => { + const { AgentStubControls } = require('../../../apps/agentStubControls'); + const customAgentControls = new AgentStubControls(); + let ignoreControls; + + before(async () => { + await customAgentControls.startAgent({ + ignoreEndpoints: { redis: 'type|set' } + }); + + ignoreControls = new ProcessControls({ + agentControls: customAgentControls, + dirname: __dirname, + env: { + REDIS_CLUSTER: setupType === 'cluster' + } + }); + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + }); + + beforeEach(async () => { + await customAgentControls.clearReceivedTraceData(); + }); + + after(async () => { + await customAgentControls.stopAgent(); + await ignoreControls.stop(); + }); + + it('should ignore redis spans for configured ignore endpoints', async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }); + + const spans = await customAgentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); + }); + describe('ignore-endpoints enabled via tracing config', async () => { + globalAgent.setUpCleanUpHooks(); + const agentControls = globalAgent.instance; + let ignoreControls; + + before(async () => { + ignoreControls = new ProcessControls({ + useGlobalAgent: true, + dirname: __dirname, + env: { + REDIS_CLUSTER: setupType === 'cluster', + IGNORE_ENDPOINTS: true, + IGNORE_COMMANDS: JSON.stringify(['get']) + } + }); + await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + }); + + beforeEach(async () => { + await agentControls.clearReceivedTraceData(); + }); + + before(async () => { + await ignoreControls.sendRequest({ + method: 'POST', + path: '/clearkeys' + }); + }); + + after(async () => { + await ignoreControls.stop(); + }); + + afterEach(async () => { + await ignoreControls.clearIpcMessages(); + }); + it('should not create redis spans for the ignored commands', async function () { + ignoreControls + .sendRequest({ + method: 'GET', + path: '/values', + qs: { + key: 'price' + } + }) + + .then(async response => { + expect(String(response)).to.equal('42'); + + return retry(async () => { + const spans = await agentControls.getSpans(); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); + + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('GET') + ]); + }); + }); + }); + it('should create spans for non-ignored Redis commands', async () => { + await ignoreControls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'price', + value: 42 + } + }) + .then(async () => { + return retry(async () => { + const spans = await agentControls.getSpans(); + expect(spans.some(span => span.n === 'redis')).to.be.true; + }); + }); + }); + }); + }); }); From 808acac7c857cb78c3288bae9c1bfd429a351289 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 25 Nov 2024 17:12:44 +0530 Subject: [PATCH 14/23] test: updated the test description --- packages/collector/test/tracing/database/ioredis/test.js | 6 +++--- packages/collector/test/tracing/database/redis/test.js | 6 +++--- packages/collector/test/tracing/messaging/bull/test.js | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index 6fba0a32bf..02ad317a1f 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1281,7 +1281,7 @@ function checkConnection(span, setupType) { }); } }); - mochaSuiteFn('ignore-endpoints tests', function () { + mochaSuiteFn('ignore-endpoints test:', function () { this.timeout(config.getTestTimeout() * 4); describe('ignore-endpoints enabled via agent config', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); @@ -1364,7 +1364,7 @@ function checkConnection(span, setupType) { afterEach(async () => { await ignoreControls.clearIpcMessages(); }); - it('should not create redis spans for the ignored commands', async function () { + it('should ignore redis spans for configured ignore endpoints', async function () { ignoreControls .sendRequest({ method: 'GET', @@ -1395,7 +1395,7 @@ function checkConnection(span, setupType) { }); }); }); - it('should create spans for non-ignored Redis commands', async () => { + it('should create redis spans for non-ignored redis endpoints', async () => { await ignoreControls .sendRequest({ method: 'POST', diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index b327cd722a..75dae2c416 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -851,7 +851,7 @@ const globalAgent = require('../../../globalAgent'); }); } }); - mochaSuiteFn('ignore-endpoints tests', function () { + mochaSuiteFn('ignore-endpoints test:', function () { describe('ignore-endpoints enabled via agent config', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); @@ -937,7 +937,7 @@ const globalAgent = require('../../../globalAgent'); afterEach(async () => { await ignoreControls.clearIpcMessages(); }); - it('should not create redis spans for the ignored commands', async function () { + it('should ignore redis spans for configured ignore endpoints', async function () { await ignoreControls .sendRequest({ method: 'POST', @@ -977,7 +977,7 @@ const globalAgent = require('../../../globalAgent'); }); }); }); - it('should create spans for non-ignored Redis commands', async () => { + it('should create redis spans for non-ignored redis endpoints', async () => { await ignoreControls .sendRequest({ method: 'GET', diff --git a/packages/collector/test/tracing/messaging/bull/test.js b/packages/collector/test/tracing/messaging/bull/test.js index aa5fb56b7b..bfd573e863 100644 --- a/packages/collector/test/tracing/messaging/bull/test.js +++ b/packages/collector/test/tracing/messaging/bull/test.js @@ -35,8 +35,7 @@ if (process.env.BULL_QUEUE_NAME) { queueName = `${process.env.BULL_QUEUE_NAME}${semver.major(process.versions.node)}`; } -const mochaSuiteFn = - supportedVersion(process.versions.node) ? describe : describe.skip; +const mochaSuiteFn = supportedVersion(process.versions.node) ? describe : describe.skip; const retryTime = 1000; From 056ada1b5f42813f92e8fd1423900bcaa357aae9 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Thu, 28 Nov 2024 13:00:22 +0530 Subject: [PATCH 15/23] chore: fine tuning --- .../src/announceCycle/unannounced.js | 16 +++++-------- .../test/announceCycle/unannounced_test.js | 23 ++++++++++++++++++- packages/core/src/tracing/spanBuffer.js | 17 +++++++------- packages/core/src/util/normalizeConfig.js | 17 +++++--------- .../src/util/{filterSpan.js => spanFilter.js} | 9 ++++---- packages/core/test/tracing/spanBuffer_test.js | 8 +++---- .../core/test/util/normalizeConfig_test.js | 9 +++++++- ...{filterSpan_test.js => spanFilter_test.js} | 12 +++++----- 8 files changed, 65 insertions(+), 46 deletions(-) rename packages/core/src/util/{filterSpan.js => spanFilter.js} (78%) rename packages/core/test/util/{filterSpan_test.js => spanFilter_test.js} (69%) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index d7ecc98547..8d299bb306 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -43,7 +43,7 @@ const maxRetryDelay = 60 * 1000; // one minute * @typedef {Object} TracingConfig * @property {Array.} [extra-http-headers] * @property {KafkaTracingConfig} [kafka] - * @property {IgnoreEndpoints} [ignore-endpoints] + * @property {Object.>} [ignore-endpoints] * @property {boolean} [span-batching-enabled] */ @@ -52,11 +52,6 @@ const maxRetryDelay = 60 * 1000; // one minute * @property {boolean} [trace-correlation] */ -/** - * @typedef {Object} IgnoreEndpoints - * @property {Object.>} [endpoints] - */ - module.exports = { /** * @param {import('./').AnnounceCycleContext} ctx @@ -231,16 +226,17 @@ function applySpanBatchingConfiguration(agentResponse) { * @param {AgentAnnounceResponse} agentResponse */ function applyIgnoreEndpointsConfiguration(agentResponse) { - if (agentResponse.tracing && agentResponse.tracing['ignore-endpoints']) { + if (agentResponse?.tracing?.['ignore-endpoints']) { const endpointTracingConfigFromAgent = agentResponse.tracing['ignore-endpoints']; const endpointTracingConfig = {}; // eslint-disable-next-line no-restricted-syntax for (const [service, actions] of Object.entries(endpointTracingConfigFromAgent)) { // From agent, the commands are separated by a pipe ('|') // @ts-ignore - endpointTracingConfig[service.toLowerCase()] = - // @ts-ignore - actions != null ? actions?.split('|')?.map(action => action?.trim()?.toLowerCase()) : []; + endpointTracingConfig[service.toLowerCase()] = actions + ? // @ts-ignore + actions?.split(/[|,]/).map(action => action?.trim()?.toLowerCase()) + : []; } ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 06c644d1a6..6316a78bbc 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -213,7 +213,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the configuration to ignore specified endpoints for a single package', done => { + it('should apply the configuration to ignore specified endpoint for a single package', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { @@ -234,6 +234,27 @@ describe('unannounced state', () => { } }); }); + it('should apply the configuration to ignore multiple endpoints for a single package', done => { + prepareAnnounceResponse({ + tracing: { + 'ignore-endpoints': { + redis: 'TYPE|GET' + } + } + }); + unannouncedState.enter({ + transitionTo: () => { + expect(agentOptsStub.config).to.deep.equal({ + tracing: { + ignoreEndpoints: { + redis: ['type', 'get'] + } + } + }); + done(); + } + }); + }); it('should apply the tracing configuration to ignore multiple endpoints across different packages', done => { prepareAnnounceResponse({ tracing: { diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 402c315ec5..321303605a 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -6,7 +6,7 @@ 'use strict'; const tracingMetrics = require('./metrics'); -const { filterSpan } = require('../util/filterSpan'); +const { applyFilter } = require('../util/spanFilter'); const { transform } = require('./backend_mappers'); /** @type {import('../core').GenericLogger} */ @@ -189,8 +189,8 @@ exports.addSpan = function (span) { return; } - // Transform the span - const processedSpan = manageSpan(span); + // Process the span, apply any transformations, and implement filtering if necessary. + const processedSpan = processSpan(span); if (!processedSpan) { logger.warn('Span of type %s has no trace ID. Not transmitting this span', span.n); return; @@ -505,15 +505,16 @@ function removeSpansIfNecessary() { * @param {import('../core').InstanaBaseSpan} span * @returns {import('../core').InstanaBaseSpan} span */ -function manageSpan(span) { +function processSpan(span) { if (!span || typeof span.n !== 'string') { return span; } - // Currently only filter if ignoreEndpoint is configured + // Currently, filter only if the ignoreEndpoint is configured. + // In the next phase, this check will be removed. if (ignoreEndpoints) { // @ts-ignore - span = filterSpan({ span, ignoreEndpoints }); + span = applyFilter({ span, ignoreEndpoints }); if (!span) { return null; @@ -521,7 +522,7 @@ function manageSpan(span) { } return transform(span); } -// export the manageSpan function for use in test. +// export the processSpan function for use in test. if (process.env.NODE_ENV === 'test') { - module.exports.manageSpan = manageSpan; + module.exports.processSpan = processSpan; } diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 52db06a6b3..9602d87439 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -27,7 +27,7 @@ const constants = require('../tracing/constants'); * @property {boolean} [disableW3cTraceCorrelation] * @property {KafkaTracingOptions} [kafka] * @property {boolean} [allowRootExitSpan] - * @property {Object} [ignoreEndpoints] + * @property {Object.>} [ignoreEndpoints] */ /** @@ -62,11 +62,6 @@ const constants = require('../tracing/constants'); * @property {number} [timeBetweenHealthcheckCalls] */ -/** - * @typedef {Object} IgnoreEndpoints - * @property {Object.>} [endpoints] - */ - /** * @typedef {Object} AgentConfig * @property {AgentTracingConfig} [tracing] @@ -231,7 +226,7 @@ function normalizeTracingConfig(config) { normalizeDisableW3cTraceCorrelation(config); normalizeTracingKafka(config); normalizeAllowRootExitSpan(config); - normalizeIgnoredEndpoints(config); + normalizeIgnoreEndpoints(config); } /** @@ -691,15 +686,15 @@ function normalizeSingleValue(configValue, defaultValue, configPath, envVarKey) /** * @param {InstanaConfig} config */ -function normalizeIgnoredEndpoints(config) { - if (!config.tracing.ignoreEndpoints) { +function normalizeIgnoreEndpoints(config) { + if (!config.tracing?.ignoreEndpoints) { config.tracing.ignoreEndpoints = {}; } for (const [service, methods] of Object.entries(config.tracing.ignoreEndpoints)) { const normalizedService = service.toLowerCase(); if (!Array.isArray(methods)) { - console.warn( + logger.warn( `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array, the value will be ignored: ${JSON.stringify( methods )}` @@ -708,7 +703,7 @@ function normalizeIgnoredEndpoints(config) { config.tracing.ignoreEndpoints[normalizedService] = []; } else { // @ts-ignore - config.tracing.ignoreEndpoints[normalizedService] = methods.map(method => method.toLowerCase()); + config.tracing.ignoreEndpoints[normalizedService] = methods.map(method => method?.toLowerCase()); } } } diff --git a/packages/core/src/util/filterSpan.js b/packages/core/src/util/spanFilter.js similarity index 78% rename from packages/core/src/util/filterSpan.js rename to packages/core/src/util/spanFilter.js index 4bf9c816a6..8608660a4f 100644 --- a/packages/core/src/util/filterSpan.js +++ b/packages/core/src/util/spanFilter.js @@ -5,8 +5,7 @@ 'use strict'; /** @type {import('../core').GenericLogger} */ -let logger; -logger = require('../logger').getLogger('tracing/util/filterSpan', newLogger => { +let logger = require('../logger').getLogger('util/spanFilter', newLogger => { logger = newLogger; }); /** @@ -26,15 +25,15 @@ function shouldIgnore(span, ignoreEndpoints) { * @param {{ span: import('../core').InstanaBaseSpan, ignoreEndpoints: { [key: string]: Array } }} params * @returns {import('../core').InstanaBaseSpan | null} */ -function filterSpan({ span, ignoreEndpoints }) { +function applyFilter({ span, ignoreEndpoints }) { if (ignoreEndpoints && shouldIgnore(span, ignoreEndpoints[span.n])) { logger.debug('Span ignored due to matching ignore endpoint', { spanType: span.n, - ignoredOperation: span.data[span.n]?.operation + ignoreOperation: span.data[span.n]?.operation }); return null; } return span; } -module.exports = { filterSpan }; +module.exports = { applyFilter }; diff --git a/packages/core/test/tracing/spanBuffer_test.js b/packages/core/test/tracing/spanBuffer_test.js index 4ac0f550cc..3c24d33a3e 100644 --- a/packages/core/test/tracing/spanBuffer_test.js +++ b/packages/core/test/tracing/spanBuffer_test.js @@ -566,7 +566,7 @@ describe('tracing/spanBuffer', () => { verifyNoBatching(span1, span2); }); }); - describe('manageSpan test', () => { + describe('processSpan test', () => { before(() => { spanBuffer.init( { @@ -597,16 +597,16 @@ describe('tracing/spanBuffer', () => { } }; it('should return the span if span is not valid', () => { - expect(spanBuffer.manageSpan(null)).to.equal(null); + expect(spanBuffer.processSpan(null)).to.equal(null); }); it('should filter out the span when command is listed in ignoreEndpoints config', () => { - expect(spanBuffer.manageSpan(span)).to.equal(null); + expect(spanBuffer.processSpan(span)).to.equal(null); }); it('should transform and return the span for command not specified in ignoreEndpoints config', () => { span.data.redis.operation = 'set'; - const result = spanBuffer.manageSpan(span); + const result = spanBuffer.processSpan(span); expect(result.data.redis.command).to.equal('set'); expect(result.data.redis).to.not.have.property('operation'); }); diff --git a/packages/core/test/util/normalizeConfig_test.js b/packages/core/test/util/normalizeConfig_test.js index 383ebf3bf9..d8bfab54fa 100644 --- a/packages/core/test/util/normalizeConfig_test.js +++ b/packages/core/test/util/normalizeConfig_test.js @@ -484,7 +484,14 @@ describe('util.normalizeConfig', () => { }); expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'] }); }); - + it('should apply multiple ignore endpoints via config', () => { + const config = normalizeConfig({ + tracing: { + ignoreEndpoints: { redis: ['GET', 'TYPE'] } + } + }); + expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get', 'type'] }); + }); it('should apply ignore endpoints via config for multiple packages', () => { const config = normalizeConfig({ tracing: { diff --git a/packages/core/test/util/filterSpan_test.js b/packages/core/test/util/spanFilter_test.js similarity index 69% rename from packages/core/test/util/filterSpan_test.js rename to packages/core/test/util/spanFilter_test.js index 0d41932529..4bec4e157f 100644 --- a/packages/core/test/util/filterSpan_test.js +++ b/packages/core/test/util/spanFilter_test.js @@ -5,7 +5,7 @@ 'use strict'; const expect = require('chai').expect; -const { filterSpan } = require('../../src/util/filterSpan'); +const { applyFilter } = require('../../src/util/spanFilter'); const span = { t: '1234567803', @@ -23,24 +23,24 @@ let ignoreEndpoints = { redis: ['GET', 'TYPE'] }; -describe('util.filterSpan', () => { +describe('util.spanFilter', () => { it('should return null when the span should be ignored', () => { span.data.redis.operation = 'GET'; - expect(filterSpan({ span, ignoreEndpoints })).equal(null); + expect(applyFilter({ span, ignoreEndpoints })).equal(null); }); it('should return the span when command is not in the ignore list', () => { span.data.redis.operation = 'HGET'; - expect(filterSpan({ span, ignoreEndpoints })).equal(span); + expect(applyFilter({ span, ignoreEndpoints })).equal(span); }); it('should return the span when span.n does not match any endpoint in config', () => { span.n = 'node.http.client'; - expect(filterSpan({ span, ignoreEndpoints })).equal(span); + expect(applyFilter({ span, ignoreEndpoints })).equal(span); }); it('should return span when no ignoreconfiguration', () => { ignoreEndpoints = {}; span.data.redis.operation = 'GET'; - expect(filterSpan({ span, ignoreEndpoints })).equal(span); + expect(applyFilter({ span, ignoreEndpoints })).equal(span); }); }); From ba2d43f662fc0a4a4a40a2f90c34ddceaf87c15c Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 29 Nov 2024 10:44:22 +0530 Subject: [PATCH 16/23] chore: incorporated review comments --- .../src/announceCycle/unannounced.js | 8 +- .../test/announceCycle/unannounced_test.js | 2 +- packages/collector/test/apps/agentStub.js | 10 +-- .../test/tracing/database/ioredis/app.js | 6 +- .../test/tracing/database/ioredis/test.js | 77 +++++++++---------- .../test/tracing/database/redis/app.js | 6 +- .../test/tracing/database/redis/legacyApp.js | 6 +- .../test/tracing/database/redis/test.js | 74 ++++++++++-------- .../tracing/backend_mappers/redis_mapper.js | 1 + packages/core/src/tracing/index.js | 3 +- .../tracing/instrumentation/database/redis.js | 1 - packages/core/src/tracing/spanBuffer.js | 24 +++--- packages/core/src/util/normalizeConfig.js | 15 +--- packages/core/src/util/spanFilter.js | 29 +++---- packages/core/test/tracing/spanBuffer_test.js | 5 +- 15 files changed, 131 insertions(+), 136 deletions(-) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index 8d299bb306..103ea6f69a 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -43,7 +43,7 @@ const maxRetryDelay = 60 * 1000; // one minute * @typedef {Object} TracingConfig * @property {Array.} [extra-http-headers] * @property {KafkaTracingConfig} [kafka] - * @property {Object.>} [ignore-endpoints] + * @property {Object.} [ignore-endpoints] * @property {boolean} [span-batching-enabled] */ @@ -228,15 +228,15 @@ function applySpanBatchingConfiguration(agentResponse) { function applyIgnoreEndpointsConfiguration(agentResponse) { if (agentResponse?.tracing?.['ignore-endpoints']) { const endpointTracingConfigFromAgent = agentResponse.tracing['ignore-endpoints']; + const endpointTracingConfig = {}; // eslint-disable-next-line no-restricted-syntax for (const [service, actions] of Object.entries(endpointTracingConfigFromAgent)) { // From agent, the commands are separated by a pipe ('|') // @ts-ignore endpointTracingConfig[service.toLowerCase()] = actions - ? // @ts-ignore - actions?.split(/[|,]/).map(action => action?.trim()?.toLowerCase()) - : []; + ? actions?.split(/[|,]/).map(action => action?.trim()?.toLowerCase()) + : null; } ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 6316a78bbc..9d6f14e8b5 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -255,7 +255,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the tracing configuration to ignore multiple endpoints across different packages', done => { + it('should apply the tracing configuration across different packages', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { diff --git a/packages/collector/test/apps/agentStub.js b/packages/collector/test/apps/agentStub.js index a152666593..21bd4d13e6 100644 --- a/packages/collector/test/apps/agentStub.js +++ b/packages/collector/test/apps/agentStub.js @@ -87,7 +87,7 @@ app.put('/com.instana.plugin.nodejs.discovery', (req, res) => { } }; - if (kafkaTraceCorrelation != null || extraHeaders.length > 0 || enableSpanBatching) { + if (kafkaTraceCorrelation != null || extraHeaders.length > 0 || enableSpanBatching || ignoreRedisEndpoints) { response.tracing = {}; if (extraHeaders.length > 0) { @@ -104,10 +104,10 @@ app.put('/com.instana.plugin.nodejs.discovery', (req, res) => { if (enableSpanBatching) { response.tracing['span-batching-enabled'] = true; } - } - if (ignoreRedisEndpoints != null) { - response.tracing = {}; - response.tracing['ignore-endpoints'] = ignoreRedisEndpoints; + if (ignoreRedisEndpoints) { + response.tracing = {}; + response.tracing['ignore-endpoints'] = ignoreRedisEndpoints; + } } res.send(response); }); diff --git a/packages/collector/test/tracing/database/ioredis/app.js b/packages/collector/test/tracing/database/ioredis/app.js index 8db0f2a5d9..1d5ffc4777 100644 --- a/packages/collector/test/tracing/database/ioredis/app.js +++ b/packages/collector/test/tracing/database/ioredis/app.js @@ -15,14 +15,14 @@ process.on('SIGTERM', () => { const agentPort = process.env.INSTANA_AGENT_PORT; -const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; -if (!ignoreEndpointsEnabled) { +const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; +if (!ignoreEndpoints) { require('../../../..')(); } else { require('../../../..')({ tracing: { ignoreEndpoints: { - redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + redis: ignoreEndpoints } } }); diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index 02ad317a1f..641a6404b6 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1282,25 +1282,24 @@ function checkConnection(span, setupType) { } }); mochaSuiteFn('ignore-endpoints test:', function () { - this.timeout(config.getTestTimeout() * 4); describe('ignore-endpoints enabled via agent config', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); - let ignoreControls; + let controls; before(async () => { await customAgentControls.startAgent({ - ignoreEndpoints: { redis: 'type|set' } + ignoreEndpoints: { redis: 'get|set' } }); - ignoreControls = new ProcessControls({ + controls = new ProcessControls({ agentControls: customAgentControls, dirname: __dirname, env: { REDIS_CLUSTER: setupType === 'cluster' } }); - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + await controls.startAndWaitForAgentConnection(); }); beforeEach(async () => { @@ -1309,63 +1308,60 @@ function checkConnection(span, setupType) { after(async () => { await customAgentControls.stopAgent(); - await ignoreControls.stop(); + await controls.stop(); }); it('should ignore redis spans for configured ignore endpoints', async () => { - await ignoreControls.sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'discount', - value: 50 - } - }); - - const spans = await customAgentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); + await controls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }) + .then(async () => { + return retry(async () => { + const spans = await customAgentControls.getSpans(); + expect(spans.length).to.equal(1); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); + }); }); }); describe('ignore-endpoints enabled via tracing config', async () => { globalAgent.setUpCleanUpHooks(); const agentControls = globalAgent.instance; - let ignoreControls; + let controls; before(async () => { - ignoreControls = new ProcessControls({ + controls = new ProcessControls({ useGlobalAgent: true, dirname: __dirname, env: { REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get']) + IGNORE_ENDPOINTS: JSON.stringify(['get']) } }); - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + await controls.start(); }); beforeEach(async () => { await agentControls.clearReceivedTraceData(); }); - before(async () => { - await ignoreControls.sendRequest({ - method: 'POST', - path: '/clearkeys' - }); - }); - after(async () => { - await ignoreControls.stop(); + await controls.stop(); }); afterEach(async () => { - await ignoreControls.clearIpcMessages(); + await controls.clearIpcMessages(); }); it('should ignore redis spans for configured ignore endpoints', async function () { - ignoreControls + await controls .sendRequest({ method: 'GET', path: '/values', @@ -1379,15 +1375,11 @@ function checkConnection(span, setupType) { return retry(async () => { const spans = await agentControls.getSpans(); + expect(spans.length).to.equal(1); spans.forEach(span => { expect(span.n).not.to.equal('redis'); }); - expectAtLeastOneMatching(spans, [ - span => expect(span.n).to.equal('node.http.server'), - span => expect(span.data.http.method).to.equal('POST') - ]); - expectAtLeastOneMatching(spans, [ span => expect(span.n).to.equal('node.http.server'), span => expect(span.data.http.method).to.equal('GET') @@ -1396,7 +1388,7 @@ function checkConnection(span, setupType) { }); }); it('should create redis spans for non-ignored redis endpoints', async () => { - await ignoreControls + await controls .sendRequest({ method: 'POST', path: '/values', @@ -1408,7 +1400,12 @@ function checkConnection(span, setupType) { .then(async () => { return retry(async () => { const spans = await agentControls.getSpans(); + expect(spans.length).to.equal(2); expect(spans.some(span => span.n === 'redis')).to.be.true; + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); }); }); }); diff --git a/packages/collector/test/tracing/database/redis/app.js b/packages/collector/test/tracing/database/redis/app.js index 6c1c8c2387..eeb3eb8a7b 100644 --- a/packages/collector/test/tracing/database/redis/app.js +++ b/packages/collector/test/tracing/database/redis/app.js @@ -12,14 +12,14 @@ process.on('SIGTERM', () => { }); require('./mockVersion'); -const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; -if (!ignoreEndpointsEnabled) { +const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; +if (!ignoreEndpoints) { require('../../../..')(); } else { require('../../../..')({ tracing: { ignoreEndpoints: { - redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + redis: ignoreEndpoints } } }); diff --git a/packages/collector/test/tracing/database/redis/legacyApp.js b/packages/collector/test/tracing/database/redis/legacyApp.js index 8d201ecd3b..8563adb4cb 100644 --- a/packages/collector/test/tracing/database/redis/legacyApp.js +++ b/packages/collector/test/tracing/database/redis/legacyApp.js @@ -15,14 +15,14 @@ process.on('SIGTERM', () => { require('./mockVersion'); -const ignoreEndpointsEnabled = process.env.IGNORE_ENDPOINTS === 'true'; -if (!ignoreEndpointsEnabled) { +const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; +if (!ignoreEndpoints) { require('../../../..')(); } else { require('../../../..')({ tracing: { ignoreEndpoints: { - redis: process.env.IGNORE_COMMANDS ? JSON.parse(process.env.IGNORE_COMMANDS) : [] + redis: ignoreEndpoints } } }); diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 75dae2c416..0fad02fce7 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -855,13 +855,13 @@ const globalAgent = require('../../../globalAgent'); describe('ignore-endpoints enabled via agent config', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); - let ignoreControls; + let controls; before(async () => { await customAgentControls.startAgent({ - ignoreEndpoints: { redis: 'type|set' } + ignoreEndpoints: { redis: 'get|set' } }); - ignoreControls = new ProcessControls({ + controls = new ProcessControls({ agentControls: customAgentControls, appPath: redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), @@ -871,7 +871,7 @@ const globalAgent = require('../../../globalAgent'); REDIS_CLUSTER: setupType === 'cluster' } }); - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + await controls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); }); beforeEach(async () => { @@ -880,31 +880,38 @@ const globalAgent = require('../../../globalAgent'); after(async () => { await customAgentControls.stopAgent(); - await ignoreControls.stop(); + await controls.stop(); }); it('should ignore redis spans for configured ignore endpoints', async () => { - await ignoreControls.sendRequest({ - method: 'POST', - path: '/values', - qs: { - key: 'discount', - value: 50 - } - }); - - const spans = await customAgentControls.getSpans(); - spans.forEach(span => { - expect(span.n).not.to.equal('redis'); - }); + await controls + .sendRequest({ + method: 'POST', + path: '/values', + qs: { + key: 'discount', + value: 50 + } + }) + .then(async () => { + return retry(async () => { + const spans = await customAgentControls.getSpans(); + // 1 x http entry span + // 1 x http client span + expect(spans.length).to.equal(2); + spans.forEach(span => { + expect(span.n).not.to.equal('redis'); + }); + }); + }); }); }); describe('ignore-endpoints enabled via tracing config', async () => { globalAgent.setUpCleanUpHooks(); - let ignoreControls; + let controls; before(async () => { - ignoreControls = new ProcessControls({ + controls = new ProcessControls({ useGlobalAgent: true, appPath: redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), @@ -912,11 +919,10 @@ const globalAgent = require('../../../globalAgent'); REDIS_VERSION: redisVersion, REDIS_PKG: redisPkg, REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: true, - IGNORE_COMMANDS: JSON.stringify(['get', 'set']) + IGNORE_ENDPOINTS: JSON.stringify(['get', 'set']) } }); - await ignoreControls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); + await controls.start(); }); beforeEach(async () => { @@ -924,21 +930,21 @@ const globalAgent = require('../../../globalAgent'); }); before(async () => { - await ignoreControls.sendRequest({ + await controls.sendRequest({ method: 'POST', path: '/clearkeys' }); }); after(async () => { - await ignoreControls.stop(); + await controls.stop(); }); afterEach(async () => { - await ignoreControls.clearIpcMessages(); + await controls.clearIpcMessages(); }); it('should ignore redis spans for configured ignore endpoints', async function () { - await ignoreControls + await controls .sendRequest({ method: 'POST', path: '/values', @@ -948,7 +954,7 @@ const globalAgent = require('../../../globalAgent'); } }) .then(() => - ignoreControls.sendRequest({ + controls.sendRequest({ method: 'GET', path: '/values', qs: { @@ -961,6 +967,10 @@ const globalAgent = require('../../../globalAgent'); return retry(async () => { const spans = await agentControls.getSpans(); + // 2 x http entry span + // 2 x http client span + expect(spans.length).to.equal(4); + spans.forEach(span => { expect(span.n).not.to.equal('redis'); }); @@ -978,7 +988,7 @@ const globalAgent = require('../../../globalAgent'); }); }); it('should create redis spans for non-ignored redis endpoints', async () => { - await ignoreControls + await controls .sendRequest({ method: 'GET', path: '/hset-hget' @@ -986,7 +996,11 @@ const globalAgent = require('../../../globalAgent'); .then(async () => { return retry(async () => { const spans = await agentControls.getSpans(); - + // 1 x http entry span + // 1 x http client span + // 1 x redis hSet span + // 1 x redis hGetAll span + expect(spans.length).to.equal(4); expect(spans.some(span => span.n === 'redis')).to.be.true; }); }); diff --git a/packages/core/src/tracing/backend_mappers/redis_mapper.js b/packages/core/src/tracing/backend_mappers/redis_mapper.js index 4d103e3772..4eb59b0386 100644 --- a/packages/core/src/tracing/backend_mappers/redis_mapper.js +++ b/packages/core/src/tracing/backend_mappers/redis_mapper.js @@ -6,6 +6,7 @@ // Configuration for Redis-specific field mappings for BE const fieldMappings = { + // internal-format: backend-format operation: 'command' }; diff --git a/packages/core/src/tracing/index.js b/packages/core/src/tracing/index.js index aa9b53e1a1..577284a3bb 100644 --- a/packages/core/src/tracing/index.js +++ b/packages/core/src/tracing/index.js @@ -120,8 +120,7 @@ if (customInstrumentations.length > 0) { */ /** - * @typedef {Object} IgnoreEndpoints - * @property {Object.>} [endpoints] + * @typedef {Object.} IgnoreEndpoints */ /** @type {Array.} */ diff --git a/packages/core/src/tracing/instrumentation/database/redis.js b/packages/core/src/tracing/instrumentation/database/redis.js index 945f16ff7e..36fe34c0f9 100644 --- a/packages/core/src/tracing/instrumentation/database/redis.js +++ b/packages/core/src/tracing/instrumentation/database/redis.js @@ -239,7 +239,6 @@ function instrumentCommand(original, command, address, cbStyle) { const span = cls.startSpan(exports.spanName, constants.EXIT); span.stack = tracingUtil.getStackTrace(instrumentCommand); - // Internal property `operation` to hold command span.data.redis = { connection: address || origCtx.address, operation: command diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 321303605a..0280c484db 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -90,7 +90,7 @@ const batchBucketWidth = 18; /** @type {BatchingBucketMap} */ const batchingBuckets = new Map(); /** - * @type {Object} + * @type {import('../tracing').IgnoreEndpoints} */ let ignoreEndpoints; @@ -104,10 +104,10 @@ exports.init = function init(config, _downstreamConnection) { forceTransmissionStartingAt = config.tracing.forceTransmissionStartingAt; transmissionDelay = config.tracing.transmissionDelay; batchingEnabled = config.tracing.spanBatchingEnabled; + ignoreEndpoints = config.tracing.ignoreEndpoints; initialDelayBeforeSendingSpans = Math.max(transmissionDelay, minDelayBeforeSendingSpans); isFaaS = false; transmitImmediate = false; - ignoreEndpoints = config.tracing?.ignoreEndpoints; if (config.tracing.activateImmediately) { preActivationCleanupIntervalHandle = setInterval(() => { @@ -134,11 +134,13 @@ exports.activate = function activate(extraConfig) { return; } - if (extraConfig && extraConfig.tracing && extraConfig.tracing.spanBatchingEnabled) { - batchingEnabled = true; - } - if (extraConfig?.tracing?.ignoreEndpoints) { - ignoreEndpoints = extraConfig.tracing.ignoreEndpoints; + if (extraConfig?.tracing) { + if (extraConfig.tracing.spanBatchingEnabled) { + batchingEnabled = true; + } + if (extraConfig.tracing.ignoreEndpoints) { + ignoreEndpoints = extraConfig.tracing.ignoreEndpoints; + } } isActive = true; @@ -192,7 +194,6 @@ exports.addSpan = function (span) { // Process the span, apply any transformations, and implement filtering if necessary. const processedSpan = processSpan(span); if (!processedSpan) { - logger.warn('Span of type %s has no trace ID. Not transmitting this span', span.n); return; } span = processedSpan; @@ -506,14 +507,7 @@ function removeSpansIfNecessary() { * @returns {import('../core').InstanaBaseSpan} span */ function processSpan(span) { - if (!span || typeof span.n !== 'string') { - return span; - } - - // Currently, filter only if the ignoreEndpoint is configured. - // In the next phase, this check will be removed. if (ignoreEndpoints) { - // @ts-ignore span = applyFilter({ span, ignoreEndpoints }); if (!span) { diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 9602d87439..25ccc14d97 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -27,7 +27,7 @@ const constants = require('../tracing/constants'); * @property {boolean} [disableW3cTraceCorrelation] * @property {KafkaTracingOptions} [kafka] * @property {boolean} [allowRootExitSpan] - * @property {Object.>} [ignoreEndpoints] + * @property {import('../tracing').IgnoreEndpoints} [ignoreEndpoints] */ /** @@ -72,7 +72,7 @@ const constants = require('../tracing/constants'); * @property {AgentTracingHttpConfig} [http] * @property {AgentTracingKafkaConfig} [kafka] * @property {boolean|string} [spanBatchingEnabled] - * @property {Object} [ignoreEndpoints] + * @property {import('../tracing').IgnoreEndpoints|{}} [ignoreEndpoints] */ /** @@ -85,11 +85,6 @@ const constants = require('../tracing/constants'); * @property {boolean} [traceCorrelation] */ -/** - * @typedef {Object} ignoreEndpoints - * @property {Object.>} [endpoints] - */ - /** @type {import('../core').GenericLogger} */ let logger; logger = require('../logger').getLogger('configuration', newLogger => { @@ -687,7 +682,7 @@ function normalizeSingleValue(configValue, defaultValue, configPath, envVarKey) * @param {InstanaConfig} config */ function normalizeIgnoreEndpoints(config) { - if (!config.tracing?.ignoreEndpoints) { + if (!config.tracing.ignoreEndpoints) { config.tracing.ignoreEndpoints = {}; } @@ -699,10 +694,8 @@ function normalizeIgnoreEndpoints(config) { methods )}` ); - // @ts-ignore - config.tracing.ignoreEndpoints[normalizedService] = []; + config.tracing.ignoreEndpoints[normalizedService] = null; } else { - // @ts-ignore config.tracing.ignoreEndpoints[normalizedService] = methods.map(method => method?.toLowerCase()); } } diff --git a/packages/core/src/util/spanFilter.js b/packages/core/src/util/spanFilter.js index 8608660a4f..3f7782c433 100644 --- a/packages/core/src/util/spanFilter.js +++ b/packages/core/src/util/spanFilter.js @@ -4,33 +4,34 @@ 'use strict'; -/** @type {import('../core').GenericLogger} */ -let logger = require('../logger').getLogger('util/spanFilter', newLogger => { - logger = newLogger; -}); +// List of span types to allowed to ignore +const IGNORABLE_SPAN_TYPES = ['redis']; + /** * @param {import('../core').InstanaBaseSpan} span - * @param {Array} ignoreEndpoints + * @param {import('../tracing').IgnoreEndpoints} endpoints * @returns {boolean} */ -function shouldIgnore(span, ignoreEndpoints) { - if (span.data?.[span.n]?.operation && ignoreEndpoints) { - return ignoreEndpoints.includes(span.data[span.n].operation); +function shouldIgnore(span, endpoints) { + // Skip if the span type is not in the ignored list + if (!IGNORABLE_SPAN_TYPES.includes(span.n)) { + return false; + } + const operation = span.data?.[span.n]?.operation; + + if (operation && endpoints[span.n]) { + return endpoints[span.n].includes(operation); } return false; } /** - * @param {{ span: import('../core').InstanaBaseSpan, ignoreEndpoints: { [key: string]: Array } }} params + * @param {{ span: import('../core').InstanaBaseSpan, ignoreEndpoints: import('../tracing').IgnoreEndpoints}} params * @returns {import('../core').InstanaBaseSpan | null} */ function applyFilter({ span, ignoreEndpoints }) { - if (ignoreEndpoints && shouldIgnore(span, ignoreEndpoints[span.n])) { - logger.debug('Span ignored due to matching ignore endpoint', { - spanType: span.n, - ignoreOperation: span.data[span.n]?.operation - }); + if (ignoreEndpoints && shouldIgnore(span, ignoreEndpoints)) { return null; } return span; diff --git a/packages/core/test/tracing/spanBuffer_test.js b/packages/core/test/tracing/spanBuffer_test.js index 3c24d33a3e..1ef5ca8e06 100644 --- a/packages/core/test/tracing/spanBuffer_test.js +++ b/packages/core/test/tracing/spanBuffer_test.js @@ -566,7 +566,7 @@ describe('tracing/spanBuffer', () => { verifyNoBatching(span1, span2); }); }); - describe('processSpan test', () => { + describe('processSpan fn', () => { before(() => { spanBuffer.init( { @@ -596,9 +596,6 @@ describe('tracing/spanBuffer', () => { } } }; - it('should return the span if span is not valid', () => { - expect(spanBuffer.processSpan(null)).to.equal(null); - }); it('should filter out the span when command is listed in ignoreEndpoints config', () => { expect(spanBuffer.processSpan(span)).to.equal(null); From ed2cbb96bc5a7e4e5bb4f3b4cc6ce16700a20486 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 29 Nov 2024 13:23:13 +0530 Subject: [PATCH 17/23] chore: updated the code --- .../src/announceCycle/unannounced.js | 16 +++---- .../core/src/tracing/backend_mappers/index.js | 13 ++++- .../src/tracing/backend_mappers/mapper.js | 47 ------------------- 3 files changed, 18 insertions(+), 58 deletions(-) delete mode 100644 packages/core/src/tracing/backend_mappers/mapper.js diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index 103ea6f69a..69e2d5c7c2 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -229,15 +229,13 @@ function applyIgnoreEndpointsConfiguration(agentResponse) { if (agentResponse?.tracing?.['ignore-endpoints']) { const endpointTracingConfigFromAgent = agentResponse.tracing['ignore-endpoints']; - const endpointTracingConfig = {}; - // eslint-disable-next-line no-restricted-syntax - for (const [service, actions] of Object.entries(endpointTracingConfigFromAgent)) { - // From agent, the commands are separated by a pipe ('|') - // @ts-ignore - endpointTracingConfig[service.toLowerCase()] = actions - ? actions?.split(/[|,]/).map(action => action?.trim()?.toLowerCase()) - : null; - } + // From agent, the commands are separated by a pipe ('|') + const endpointTracingConfig = Object.fromEntries( + Object.entries(endpointTracingConfigFromAgent).map(([service, actions]) => [ + service.toLowerCase(), + actions?.split('|').map(action => action?.trim()?.toLowerCase()) || null + ]) + ); ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); agentOpts.config.tracing.ignoreEndpoints = endpointTracingConfig; diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js index 22fcccfb12..a16e84a546 100644 --- a/packages/core/src/tracing/backend_mappers/index.js +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -4,5 +4,14 @@ 'use strict'; -// Entry point for the mappers module. -module.exports = require('./mapper'); +Object.defineProperty(module.exports, 'transform', { + get() { + return (/** @type {import('../../core').InstanaBaseSpan} */ span) => { + try { + return require(`./${span.n}_mapper`).transform(span); + } catch { + return span; + } + }; + } +}); diff --git a/packages/core/src/tracing/backend_mappers/mapper.js b/packages/core/src/tracing/backend_mappers/mapper.js deleted file mode 100644 index 0297afd755..0000000000 --- a/packages/core/src/tracing/backend_mappers/mapper.js +++ /dev/null @@ -1,47 +0,0 @@ -/* - * (c) Copyright IBM Corp. 2024 - */ - -'use strict'; - -/** - * @type {Record} - */ -const cachedMappers = {}; - -/** - * Dynamically require the mapper based on span.n. - * - * @param {string} spanName - * @returns {Function|null} - */ -function loadMapper(spanName) { - if (cachedMappers[spanName]) { - return cachedMappers[spanName]; - } - - try { - // Ensures mapper files follow the same naming convention. - const mapper = require(`./${spanName}_mapper.js`).transform; - cachedMappers[spanName] = mapper; - return mapper; - } catch (err) { - return null; - } -} - -/** - * @param {import('../../core').InstanaBaseSpan} span - * @returns {import('../../core').InstanaBaseSpan} - */ -function transform(span) { - const transformFunction = loadMapper(span.n); - - if (transformFunction) { - return transformFunction(span); - } else { - return span; - } -} - -module.exports = { transform }; From d420d409d0b276f809079e151d11f3aa3fe56d25 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Fri, 29 Nov 2024 15:56:01 +0530 Subject: [PATCH 18/23] test: updated the spanbuffer test --- packages/core/src/tracing/spanBuffer.js | 4 --- packages/core/test/tracing/spanBuffer_test.js | 25 +++++++++++++------ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 0280c484db..51b584cbb2 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -516,7 +516,3 @@ function processSpan(span) { } return transform(span); } -// export the processSpan function for use in test. -if (process.env.NODE_ENV === 'test') { - module.exports.processSpan = processSpan; -} diff --git a/packages/core/test/tracing/spanBuffer_test.js b/packages/core/test/tracing/spanBuffer_test.js index 1ef5ca8e06..9787bcbe79 100644 --- a/packages/core/test/tracing/spanBuffer_test.js +++ b/packages/core/test/tracing/spanBuffer_test.js @@ -566,7 +566,7 @@ describe('tracing/spanBuffer', () => { verifyNoBatching(span1, span2); }); }); - describe('processSpan fn', () => { + describe('when ignoreEndpoints is configured', () => { before(() => { spanBuffer.init( { @@ -597,15 +597,26 @@ describe('tracing/spanBuffer', () => { } }; - it('should filter out the span when command is listed in ignoreEndpoints config', () => { - expect(spanBuffer.processSpan(span)).to.equal(null); + it('should ignore the redis span when the operation is listed in the ignoreEndpoints config', () => { + spanBuffer.addSpan(span); + const spans = spanBuffer.getAndResetSpans(); + expect(spans).to.have.lengthOf(0); }); - it('should transform and return the span for command not specified in ignoreEndpoints config', () => { + it('should transform the redis span if the operation is not specified in the ignoreEndpoints config', () => { span.data.redis.operation = 'set'; - const result = spanBuffer.processSpan(span); - expect(result.data.redis.command).to.equal('set'); - expect(result.data.redis).to.not.have.property('operation'); + spanBuffer.addSpan(span); + const spans = spanBuffer.getAndResetSpans(); + expect(spans).to.have.lengthOf(1); + expect(span.data.redis.command).to.equal('set'); + expect(span.data.redis).to.not.have.property('operation'); + }); + it('should return the span unmodified for unsupported ignore endpoints', () => { + span.n = 'http'; + spanBuffer.addSpan(span); + const spans = spanBuffer.getAndResetSpans(); + expect(spans).to.have.lengthOf(1); + expect(span).to.deep.equal(span); }); }); }); From b3336f740694661f3e74cde752f899569c065a54 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 2 Dec 2024 11:09:15 +0530 Subject: [PATCH 19/23] chore: added env variable INSTANA_IGNORE_ENDPOINTS --- .../src/announceCycle/unannounced.js | 19 +++++++--- .../test/announceCycle/unannounced_test.js | 38 ++++++++++++++++--- .../test/tracing/database/ioredis/app.js | 13 +------ .../test/tracing/database/ioredis/test.js | 2 +- .../test/tracing/database/redis/app.js | 14 +------ .../test/tracing/database/redis/legacyApp.js | 13 +------ .../test/tracing/database/redis/test.js | 4 +- packages/core/src/util/normalizeConfig.js | 22 ++++++++--- .../core/test/util/normalizeConfig_test.js | 20 ++++++++++ 9 files changed, 89 insertions(+), 56 deletions(-) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index 69e2d5c7c2..512b9922b1 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -43,7 +43,7 @@ const maxRetryDelay = 60 * 1000; // one minute * @typedef {Object} TracingConfig * @property {Array.} [extra-http-headers] * @property {KafkaTracingConfig} [kafka] - * @property {Object.} [ignore-endpoints] + * @property {Object.} [ignore-endpoints] * @property {boolean} [span-batching-enabled] */ @@ -229,12 +229,19 @@ function applyIgnoreEndpointsConfiguration(agentResponse) { if (agentResponse?.tracing?.['ignore-endpoints']) { const endpointTracingConfigFromAgent = agentResponse.tracing['ignore-endpoints']; - // From agent, the commands are separated by a pipe ('|') const endpointTracingConfig = Object.fromEntries( - Object.entries(endpointTracingConfigFromAgent).map(([service, actions]) => [ - service.toLowerCase(), - actions?.split('|').map(action => action?.trim()?.toLowerCase()) || null - ]) + Object.entries(endpointTracingConfigFromAgent).map(([service, endpoints]) => { + let normalizedEndpoints = null; + if (typeof endpoints === 'string') { + normalizedEndpoints = endpoints + .split(/[|,]/) // From agent, the commands are separated by a pipe ('|') + .map(endpoint => endpoint?.trim()?.toLowerCase()); + } else if (Array.isArray(endpoints)) { + normalizedEndpoints = endpoints.map(endpoint => endpoint?.toLowerCase()); + } + + return [service.toLowerCase(), normalizedEndpoints]; + }) ); ensureNestedObjectExists(agentOpts.config, ['tracing', 'ignoreEndpoints']); diff --git a/packages/collector/test/announceCycle/unannounced_test.js b/packages/collector/test/announceCycle/unannounced_test.js index 9d6f14e8b5..1ab1a26158 100644 --- a/packages/collector/test/announceCycle/unannounced_test.js +++ b/packages/collector/test/announceCycle/unannounced_test.js @@ -213,7 +213,7 @@ describe('unannounced state', () => { } }); }); - it('should apply the configuration to ignore specified endpoint for a single package', done => { + it('should apply the configuration to ignore a single endpoint for a package', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { @@ -234,11 +234,12 @@ describe('unannounced state', () => { } }); }); - it('should apply the configuration to ignore multiple endpoints for a single package', done => { + + it('should apply the configuration to ignore multiple endpoints for a package', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { - redis: 'TYPE|GET' + redis: 'SET|GET' } } }); @@ -247,7 +248,7 @@ describe('unannounced state', () => { expect(agentOptsStub.config).to.deep.equal({ tracing: { ignoreEndpoints: { - redis: ['type', 'get'] + redis: ['set', 'get'] } } }); @@ -255,11 +256,36 @@ describe('unannounced state', () => { } }); }); - it('should apply the tracing configuration across different packages', done => { + + it('should apply tracing configuration to ignore specified endpoints across different packages', done => { + prepareAnnounceResponse({ + tracing: { + 'ignore-endpoints': { + REDIS: 'get|set', + dynamodb: 'query' + } + } + }); + unannouncedState.enter({ + transitionTo: () => { + expect(agentOptsStub.config).to.deep.equal({ + tracing: { + ignoreEndpoints: { + redis: ['get', 'set'], + dynamodb: ['query'] + } + } + }); + done(); + } + }); + }); + + it('should apply tracing configuration to ignore endpoints when specified using array format', done => { prepareAnnounceResponse({ tracing: { 'ignore-endpoints': { - REDIS: 'get|type', + REDIS: ['get', 'type'], dynamodb: 'query' } } diff --git a/packages/collector/test/tracing/database/ioredis/app.js b/packages/collector/test/tracing/database/ioredis/app.js index 1d5ffc4777..6ed89a6ef7 100644 --- a/packages/collector/test/tracing/database/ioredis/app.js +++ b/packages/collector/test/tracing/database/ioredis/app.js @@ -15,18 +15,7 @@ process.on('SIGTERM', () => { const agentPort = process.env.INSTANA_AGENT_PORT; -const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; -if (!ignoreEndpoints) { - require('../../../..')(); -} else { - require('../../../..')({ - tracing: { - ignoreEndpoints: { - redis: ignoreEndpoints - } - } - }); -} +require('../../../..')(); const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index 641a6404b6..d22d913f0b 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1343,7 +1343,7 @@ function checkConnection(span, setupType) { dirname: __dirname, env: { REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: JSON.stringify(['get']) + INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get"}' } }); await controls.start(); diff --git a/packages/collector/test/tracing/database/redis/app.js b/packages/collector/test/tracing/database/redis/app.js index eeb3eb8a7b..1da94a24ee 100644 --- a/packages/collector/test/tracing/database/redis/app.js +++ b/packages/collector/test/tracing/database/redis/app.js @@ -12,18 +12,8 @@ process.on('SIGTERM', () => { }); require('./mockVersion'); -const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; -if (!ignoreEndpoints) { - require('../../../..')(); -} else { - require('../../../..')({ - tracing: { - ignoreEndpoints: { - redis: ignoreEndpoints - } - } - }); -} +require('../../../..')(); + const redis = require(process.env.REDIS_PKG); const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/redis/legacyApp.js b/packages/collector/test/tracing/database/redis/legacyApp.js index 8563adb4cb..24cc1e208f 100644 --- a/packages/collector/test/tracing/database/redis/legacyApp.js +++ b/packages/collector/test/tracing/database/redis/legacyApp.js @@ -15,18 +15,7 @@ process.on('SIGTERM', () => { require('./mockVersion'); -const ignoreEndpoints = process.env.IGNORE_ENDPOINTS ? JSON.parse(process.env.IGNORE_ENDPOINTS) : null; -if (!ignoreEndpoints) { - require('../../../..')(); -} else { - require('../../../..')({ - tracing: { - ignoreEndpoints: { - redis: ignoreEndpoints - } - } - }); -} +require('../../../..')(); const bodyParser = require('body-parser'); const express = require('express'); diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 0fad02fce7..0ce6258d54 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -28,7 +28,7 @@ const globalAgent = require('../../../globalAgent'); // Please set the environment variables to run the tests against azure redis cluster: // export AZURE_REDIS_CLUSTER=team-nodejs-redis-cluster-tekton.redis.cache.windows.net:6380 // export AZURE_REDIS_CLUSTER_PWD= -['default', 'cluster'].forEach(setupType => { +['default'].forEach(setupType => { describe(`tracing/redis ${setupType}`, function () { ['redis', '@redis/client'].forEach(redisPkg => { describe(`require: ${redisPkg}`, function () { @@ -919,7 +919,7 @@ const globalAgent = require('../../../globalAgent'); REDIS_VERSION: redisVersion, REDIS_PKG: redisPkg, REDIS_CLUSTER: setupType === 'cluster', - IGNORE_ENDPOINTS: JSON.stringify(['get', 'set']) + INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get"}' } }); await controls.start(); diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 25ccc14d97..6397eac462 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -686,17 +686,29 @@ function normalizeIgnoreEndpoints(config) { config.tracing.ignoreEndpoints = {}; } - for (const [service, methods] of Object.entries(config.tracing.ignoreEndpoints)) { + if (process.env.INSTANA_IGNORE_ENDPOINTS) { + try { + logger.info('Ignore endpoints have been added via environment variable INSTANA_IGNORE_ENDPOINTS.'); + config.tracing.ignoreEndpoints = JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); + return; + } catch (error) { + logger.warn( + `Failed to parse INSTANA_IGNORE_ENDPOINTS: ${process.env.INSTANA_IGNORE_ENDPOINTS}. Error: ${error.message}` + ); + // Fallback to existing config if parsing fails + } + } + for (const [service, endpoints] of Object.entries(config.tracing.ignoreEndpoints)) { const normalizedService = service.toLowerCase(); - if (!Array.isArray(methods)) { + if (!Array.isArray(endpoints)) { logger.warn( - `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array, the value will be ignored: ${JSON.stringify( - methods + `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( + endpoints )}` ); config.tracing.ignoreEndpoints[normalizedService] = null; } else { - config.tracing.ignoreEndpoints[normalizedService] = methods.map(method => method?.toLowerCase()); + config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => endpoint?.toLowerCase()); } } } diff --git a/packages/core/test/util/normalizeConfig_test.js b/packages/core/test/util/normalizeConfig_test.js index d8bfab54fa..89a7820b93 100644 --- a/packages/core/test/util/normalizeConfig_test.js +++ b/packages/core/test/util/normalizeConfig_test.js @@ -476,6 +476,17 @@ describe('util.normalizeConfig', () => { expect(config.tracing.ignoreEndpoints).to.deep.equal({}); }); + it('should apply ignore endpoints if the INSTANA_IGNORE_ENDPOINTS is set and valid', () => { + process.env.INSTANA_IGNORE_ENDPOINTS = '{"redis": ["get", "set"]}'; + const config = normalizeConfig(); + expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get', 'set'] }); + }); + + it('should fallback to default if INSTANA_IGNORE_ENDPOINTS is set but has an invalid format', () => { + process.env.INSTANA_IGNORE_ENDPOINTS = '"redis": ["get", "set"]'; + const config = normalizeConfig(); + expect(config.tracing.ignoreEndpoints).to.deep.equal({}); + }); it('should apply ignore endpoints via config', () => { const config = normalizeConfig({ tracing: { @@ -500,6 +511,15 @@ describe('util.normalizeConfig', () => { }); expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'], dynamodb: ['querey'] }); }); + it('should apply ignore endpoints from the config when INSTANA_IGNORE_ENDPOINTS contains invalid JSON', () => { + process.env.INSTANA_IGNORE_ENDPOINTS = '"redis": ["get", "set"]'; + const config = normalizeConfig({ + tracing: { + ignoreEndpoints: { redis: ['get'] } + } + }); + expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'] }); + }); function checkDefaults(config) { expect(config).to.be.an('object'); From fdfc94bf7562cd770af93e1e80ca57d0734e0077 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Mon, 2 Dec 2024 12:19:53 +0530 Subject: [PATCH 20/23] test: fix failing test --- packages/collector/test/apps/agentStub.js | 9 ++-- .../collector/test/apps/agentStubControls.js | 2 +- .../test/tracing/database/ioredis/test.js | 48 +++++++++++-------- .../test/tracing/database/redis/test.js | 19 ++++---- .../tracing/backend_mappers/redis_mapper.js | 15 +++--- packages/core/src/util/normalizeConfig.js | 31 ++++++------ .../core/test/util/normalizeConfig_test.js | 9 ---- 7 files changed, 67 insertions(+), 66 deletions(-) diff --git a/packages/collector/test/apps/agentStub.js b/packages/collector/test/apps/agentStub.js index 21bd4d13e6..c051f6103c 100644 --- a/packages/collector/test/apps/agentStub.js +++ b/packages/collector/test/apps/agentStub.js @@ -38,7 +38,7 @@ const enableSpanBatching = process.env.ENABLE_SPANBATCHING === 'true'; const kafkaTraceCorrelation = process.env.KAFKA_TRACE_CORRELATION ? process.env.KAFKA_TRACE_CORRELATION === 'true' : null; -const ignoreRedisEndpoints = process.env.INSTANA_IGNORE_ENDPOINTS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); +const ignoreEndpoints = process.env.INSTANA_IGNORE_ENDPOINTS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); let discoveries = {}; let rejectAnnounceAttempts = 0; @@ -87,7 +87,7 @@ app.put('/com.instana.plugin.nodejs.discovery', (req, res) => { } }; - if (kafkaTraceCorrelation != null || extraHeaders.length > 0 || enableSpanBatching || ignoreRedisEndpoints) { + if (kafkaTraceCorrelation != null || extraHeaders.length > 0 || enableSpanBatching || ignoreEndpoints) { response.tracing = {}; if (extraHeaders.length > 0) { @@ -104,9 +104,8 @@ app.put('/com.instana.plugin.nodejs.discovery', (req, res) => { if (enableSpanBatching) { response.tracing['span-batching-enabled'] = true; } - if (ignoreRedisEndpoints) { - response.tracing = {}; - response.tracing['ignore-endpoints'] = ignoreRedisEndpoints; + if (ignoreEndpoints) { + response.tracing['ignore-endpoints'] = ignoreEndpoints; } } res.send(response); diff --git a/packages/collector/test/apps/agentStubControls.js b/packages/collector/test/apps/agentStubControls.js index e3049a9afd..7101ef9bac 100644 --- a/packages/collector/test/apps/agentStubControls.js +++ b/packages/collector/test/apps/agentStubControls.js @@ -43,7 +43,7 @@ class AgentStubControls { env.KAFKA_TRACE_CORRELATION = opts.kafkaConfig.traceCorrelation.toString(); } } - if (opts?.ignoreEndpoints) { + if (opts.ignoreEndpoints) { env.INSTANA_IGNORE_ENDPOINTS = JSON.stringify(opts.ignoreEndpoints); } diff --git a/packages/collector/test/tracing/database/ioredis/test.js b/packages/collector/test/tracing/database/ioredis/test.js index d22d913f0b..7e1c303b84 100644 --- a/packages/collector/test/tracing/database/ioredis/test.js +++ b/packages/collector/test/tracing/database/ioredis/test.js @@ -1281,8 +1281,8 @@ function checkConnection(span, setupType) { }); } }); - mochaSuiteFn('ignore-endpoints test:', function () { - describe('ignore-endpoints enabled via agent config', () => { + mochaSuiteFn('ignore-endpoints:', function () { + describe('when ignore-endpoints is enabled via agent configuration', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); let controls; @@ -1294,10 +1294,7 @@ function checkConnection(span, setupType) { controls = new ProcessControls({ agentControls: customAgentControls, - dirname: __dirname, - env: { - REDIS_CLUSTER: setupType === 'cluster' - } + dirname: __dirname }); await controls.startAndWaitForAgentConnection(); }); @@ -1311,7 +1308,7 @@ function checkConnection(span, setupType) { await controls.stop(); }); - it('should ignore redis spans for configured ignore endpoints', async () => { + it('should ignore redis spans for ignored endpoints (get, set)', async () => { await controls .sendRequest({ method: 'POST', @@ -1324,15 +1321,20 @@ function checkConnection(span, setupType) { .then(async () => { return retry(async () => { const spans = await customAgentControls.getSpans(); + // 1 x http entry span expect(spans.length).to.equal(1); spans.forEach(span => { expect(span.n).not.to.equal('redis'); }); + expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.data.http.method).to.equal('POST') + ]); }); }); }); }); - describe('ignore-endpoints enabled via tracing config', async () => { + describe('when ignore-endpoints is enabled via tracing configuration', async () => { globalAgent.setUpCleanUpHooks(); const agentControls = globalAgent.instance; let controls; @@ -1342,8 +1344,7 @@ function checkConnection(span, setupType) { useGlobalAgent: true, dirname: __dirname, env: { - REDIS_CLUSTER: setupType === 'cluster', - INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get"}' + INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get"]}' } }); await controls.start(); @@ -1360,21 +1361,21 @@ function checkConnection(span, setupType) { afterEach(async () => { await controls.clearIpcMessages(); }); - it('should ignore redis spans for configured ignore endpoints', async function () { + it('should ignore spans for ignored endpoint (get)', async function () { await controls .sendRequest({ method: 'GET', path: '/values', qs: { - key: 'price' + key: 'discount', + value: 50 } }) - .then(async response => { - expect(String(response)).to.equal('42'); - + .then(async () => { return retry(async () => { const spans = await agentControls.getSpans(); + // 1 x http entry span expect(spans.length).to.equal(1); spans.forEach(span => { expect(span.n).not.to.equal('redis'); @@ -1387,25 +1388,32 @@ function checkConnection(span, setupType) { }); }); }); - it('should create redis spans for non-ignored redis endpoints', async () => { + it('should not ignore spans for endpoints that are not in the ignore list', async () => { await controls .sendRequest({ method: 'POST', path: '/values', qs: { - key: 'price', - value: 42 + key: 'discount', + value: 50 } }) .then(async () => { return retry(async () => { const spans = await agentControls.getSpans(); expect(spans.length).to.equal(2); - expect(spans.some(span => span.n === 'redis')).to.be.true; - expectAtLeastOneMatching(spans, [ + + const entrySpan = expectAtLeastOneMatching(spans, [ span => expect(span.n).to.equal('node.http.server'), span => expect(span.data.http.method).to.equal('POST') ]); + + expectExactlyOneMatching(spans, [ + span => expect(span.t).to.equal(entrySpan.t), + span => expect(span.p).to.equal(entrySpan.s), + span => expect(span.n).to.equal('redis'), + span => expect(span.data.redis.command).to.equal('set') + ]); }); }); }); diff --git a/packages/collector/test/tracing/database/redis/test.js b/packages/collector/test/tracing/database/redis/test.js index 0ce6258d54..9217d56e6c 100644 --- a/packages/collector/test/tracing/database/redis/test.js +++ b/packages/collector/test/tracing/database/redis/test.js @@ -28,7 +28,7 @@ const globalAgent = require('../../../globalAgent'); // Please set the environment variables to run the tests against azure redis cluster: // export AZURE_REDIS_CLUSTER=team-nodejs-redis-cluster-tekton.redis.cache.windows.net:6380 // export AZURE_REDIS_CLUSTER_PWD= -['default'].forEach(setupType => { +['default', 'cluster'].forEach(setupType => { describe(`tracing/redis ${setupType}`, function () { ['redis', '@redis/client'].forEach(redisPkg => { describe(`require: ${redisPkg}`, function () { @@ -199,6 +199,7 @@ const globalAgent = require('../../../globalAgent'); }) ); })); + it('must trace hset/hget calls', () => controls .sendRequest({ @@ -851,8 +852,8 @@ const globalAgent = require('../../../globalAgent'); }); } }); - mochaSuiteFn('ignore-endpoints test:', function () { - describe('ignore-endpoints enabled via agent config', () => { + mochaSuiteFn('ignore-endpoints:', function () { + describe('when ignore-endpoints is enabled via agent configuration', () => { const { AgentStubControls } = require('../../../apps/agentStubControls'); const customAgentControls = new AgentStubControls(); let controls; @@ -867,8 +868,7 @@ const globalAgent = require('../../../globalAgent'); redisVersion === 'latest' ? path.join(__dirname, 'app.js') : path.join(__dirname, 'legacyApp.js'), env: { REDIS_VERSION: redisVersion, - REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster' + REDIS_PKG: redisPkg } }); await controls.startAndWaitForAgentConnection(5000, Date.now() + 1000 * 60 * 5); @@ -883,7 +883,7 @@ const globalAgent = require('../../../globalAgent'); await controls.stop(); }); - it('should ignore redis spans for configured ignore endpoints', async () => { + it('should ignore redis spans for ignored endpoints (get, set)', async () => { await controls .sendRequest({ method: 'POST', @@ -918,8 +918,7 @@ const globalAgent = require('../../../globalAgent'); env: { REDIS_VERSION: redisVersion, REDIS_PKG: redisPkg, - REDIS_CLUSTER: setupType === 'cluster', - INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get"}' + INSTANA_IGNORE_ENDPOINTS: '{"redis": ["get","set"]}' } }); await controls.start(); @@ -943,7 +942,7 @@ const globalAgent = require('../../../globalAgent'); afterEach(async () => { await controls.clearIpcMessages(); }); - it('should ignore redis spans for configured ignore endpoints', async function () { + it('should ignore spans for configured ignore endpoints(get,set)', async function () { await controls .sendRequest({ method: 'POST', @@ -987,7 +986,7 @@ const globalAgent = require('../../../globalAgent'); }); }); }); - it('should create redis spans for non-ignored redis endpoints', async () => { + it('should not ignore spans for endpoints that are not in the ignore list', async () => { await controls .sendRequest({ method: 'GET', diff --git a/packages/core/src/tracing/backend_mappers/redis_mapper.js b/packages/core/src/tracing/backend_mappers/redis_mapper.js index 4eb59b0386..81a8dd4422 100644 --- a/packages/core/src/tracing/backend_mappers/redis_mapper.js +++ b/packages/core/src/tracing/backend_mappers/redis_mapper.js @@ -4,22 +4,23 @@ 'use strict'; -// Configuration for Redis-specific field mappings for BE const fieldMappings = { // internal-format: backend-format operation: 'command' }; /** - * @param {Object} span - * @returns {Object} + * Transforms Redis-related span data fields to match the backend format. + * + * @param {import('../../core').InstanaBaseSpan} span + * @returns {import('../../core').InstanaBaseSpan} The transformed span. */ function transform(span) { if (span.data?.redis) { - Object.entries(fieldMappings).forEach(([oldKey, newKey]) => { - if (span.data.redis[oldKey]) { - span.data.redis[newKey] = span.data.redis[oldKey]; - delete span.data.redis[oldKey]; + Object.entries(fieldMappings).forEach(([internalField, backendField]) => { + if (span.data.redis[internalField]) { + span.data.redis[backendField] = span.data.redis[internalField]; + delete span.data.redis[internalField]; } }); } diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 6397eac462..9df4b5f295 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -685,6 +685,23 @@ function normalizeIgnoreEndpoints(config) { if (!config.tracing.ignoreEndpoints) { config.tracing.ignoreEndpoints = {}; } + if (Object.keys(config.tracing.ignoreEndpoints).length) { + for (const [service, endpoints] of Object.entries(config.tracing.ignoreEndpoints)) { + const normalizedService = service.toLowerCase(); + + if (!Array.isArray(endpoints)) { + logger.warn( + `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( + endpoints + )}` + ); + config.tracing.ignoreEndpoints[normalizedService] = null; // Set to null for invalid configuration + } else { + config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => endpoint?.toLowerCase()); + } + } + return; + } if (process.env.INSTANA_IGNORE_ENDPOINTS) { try { @@ -695,20 +712,6 @@ function normalizeIgnoreEndpoints(config) { logger.warn( `Failed to parse INSTANA_IGNORE_ENDPOINTS: ${process.env.INSTANA_IGNORE_ENDPOINTS}. Error: ${error.message}` ); - // Fallback to existing config if parsing fails - } - } - for (const [service, endpoints] of Object.entries(config.tracing.ignoreEndpoints)) { - const normalizedService = service.toLowerCase(); - if (!Array.isArray(endpoints)) { - logger.warn( - `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( - endpoints - )}` - ); - config.tracing.ignoreEndpoints[normalizedService] = null; - } else { - config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => endpoint?.toLowerCase()); } } } diff --git a/packages/core/test/util/normalizeConfig_test.js b/packages/core/test/util/normalizeConfig_test.js index 89a7820b93..4b1be86dc4 100644 --- a/packages/core/test/util/normalizeConfig_test.js +++ b/packages/core/test/util/normalizeConfig_test.js @@ -511,15 +511,6 @@ describe('util.normalizeConfig', () => { }); expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'], dynamodb: ['querey'] }); }); - it('should apply ignore endpoints from the config when INSTANA_IGNORE_ENDPOINTS contains invalid JSON', () => { - process.env.INSTANA_IGNORE_ENDPOINTS = '"redis": ["get", "set"]'; - const config = normalizeConfig({ - tracing: { - ignoreEndpoints: { redis: ['get'] } - } - }); - expect(config.tracing.ignoreEndpoints).to.deep.equal({ redis: ['get'] }); - }); function checkDefaults(config) { expect(config).to.be.an('object'); From 575654e7ffb83996c596dfe7c2d72a1b5a83d96b Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Tue, 3 Dec 2024 16:18:05 +0530 Subject: [PATCH 21/23] chore: resolved review comments --- .../src/announceCycle/unannounced.js | 10 +++++-- packages/collector/test/apps/agentStub.js | 2 +- .../collector/test/apps/agentStubControls.js | 2 +- .../core/src/tracing/backend_mappers/index.js | 6 ++-- .../tracing/backend_mappers/redis_mapper.js | 6 ++-- packages/core/src/util/normalizeConfig.js | 28 +++++++++++++------ packages/core/src/util/spanFilter.js | 2 +- .../tracing/backend_mappers/mapper_test.js | 2 +- 8 files changed, 35 insertions(+), 23 deletions(-) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index 512b9922b1..a826336575 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -222,7 +222,13 @@ function applySpanBatchingConfiguration(agentResponse) { agentOpts.config.tracing.spanBatchingEnabled = true; } } + /** + * - The agent configuration currently uses a pipe ('|') as a separator for endpoints. + * - This function splits the string by both pipe ('|') and comma (',') to ensure compatibility. + * - Additionally, it supports the `string[]` format for backward compatibility, + * as this was the previously used standard. + * * @param {AgentAnnounceResponse} agentResponse */ function applyIgnoreEndpointsConfiguration(agentResponse) { @@ -233,9 +239,7 @@ function applyIgnoreEndpointsConfiguration(agentResponse) { Object.entries(endpointTracingConfigFromAgent).map(([service, endpoints]) => { let normalizedEndpoints = null; if (typeof endpoints === 'string') { - normalizedEndpoints = endpoints - .split(/[|,]/) // From agent, the commands are separated by a pipe ('|') - .map(endpoint => endpoint?.trim()?.toLowerCase()); + normalizedEndpoints = endpoints.split(/[|,]/).map(endpoint => endpoint?.trim()?.toLowerCase()); } else if (Array.isArray(endpoints)) { normalizedEndpoints = endpoints.map(endpoint => endpoint?.toLowerCase()); } diff --git a/packages/collector/test/apps/agentStub.js b/packages/collector/test/apps/agentStub.js index c051f6103c..ebf2f62cd0 100644 --- a/packages/collector/test/apps/agentStub.js +++ b/packages/collector/test/apps/agentStub.js @@ -38,7 +38,7 @@ const enableSpanBatching = process.env.ENABLE_SPANBATCHING === 'true'; const kafkaTraceCorrelation = process.env.KAFKA_TRACE_CORRELATION ? process.env.KAFKA_TRACE_CORRELATION === 'true' : null; -const ignoreEndpoints = process.env.INSTANA_IGNORE_ENDPOINTS && JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); +const ignoreEndpoints = process.env.IGNORE_ENDPOINTS && JSON.parse(process.env.IGNORE_ENDPOINTS); let discoveries = {}; let rejectAnnounceAttempts = 0; diff --git a/packages/collector/test/apps/agentStubControls.js b/packages/collector/test/apps/agentStubControls.js index 7101ef9bac..74fc4cc618 100644 --- a/packages/collector/test/apps/agentStubControls.js +++ b/packages/collector/test/apps/agentStubControls.js @@ -44,7 +44,7 @@ class AgentStubControls { } } if (opts.ignoreEndpoints) { - env.INSTANA_IGNORE_ENDPOINTS = JSON.stringify(opts.ignoreEndpoints); + env.IGNORE_ENDPOINTS = JSON.stringify(opts.ignoreEndpoints); } this.agentStub = spawn('node', [path.join(__dirname, 'agentStub.js')], { diff --git a/packages/core/src/tracing/backend_mappers/index.js b/packages/core/src/tracing/backend_mappers/index.js index a16e84a546..831d0036dd 100644 --- a/packages/core/src/tracing/backend_mappers/index.js +++ b/packages/core/src/tracing/backend_mappers/index.js @@ -4,8 +4,8 @@ 'use strict'; -Object.defineProperty(module.exports, 'transform', { - get() { +module.exports = { + get transform() { return (/** @type {import('../../core').InstanaBaseSpan} */ span) => { try { return require(`./${span.n}_mapper`).transform(span); @@ -14,4 +14,4 @@ Object.defineProperty(module.exports, 'transform', { } }; } -}); +}; diff --git a/packages/core/src/tracing/backend_mappers/redis_mapper.js b/packages/core/src/tracing/backend_mappers/redis_mapper.js index 81a8dd4422..e470b011c1 100644 --- a/packages/core/src/tracing/backend_mappers/redis_mapper.js +++ b/packages/core/src/tracing/backend_mappers/redis_mapper.js @@ -15,7 +15,7 @@ const fieldMappings = { * @param {import('../../core').InstanaBaseSpan} span * @returns {import('../../core').InstanaBaseSpan} The transformed span. */ -function transform(span) { +module.exports.transform = span => { if (span.data?.redis) { Object.entries(fieldMappings).forEach(([internalField, backendField]) => { if (span.data.redis[internalField]) { @@ -26,6 +26,4 @@ function transform(span) { } return span; -} - -module.exports = { transform }; +}; diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 9df4b5f295..41cc61a5d9 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -685,13 +685,26 @@ function normalizeIgnoreEndpoints(config) { if (!config.tracing.ignoreEndpoints) { config.tracing.ignoreEndpoints = {}; } - if (Object.keys(config.tracing.ignoreEndpoints).length) { - for (const [service, endpoints] of Object.entries(config.tracing.ignoreEndpoints)) { + + const ignoreEndpoints = config.tracing.ignoreEndpoints; + + if (typeof ignoreEndpoints !== 'object' || Array.isArray(ignoreEndpoints)) { + logger.warn( + `Invalid tracing.ignoreEndpoints configuration. Expected an object, but received: ${JSON.stringify( + ignoreEndpoints + )}` + ); + config.tracing.ignoreEndpoints = {}; + return; + } + + if (Object.keys(ignoreEndpoints).length) { + for (const [service, endpoints] of Object.entries(ignoreEndpoints)) { const normalizedService = service.toLowerCase(); if (!Array.isArray(endpoints)) { logger.warn( - `Invalid configuration for ${normalizedService}: ignoredEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( + `Invalid configuration for ${normalizedService}: tracing.ignoreEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( endpoints )}` ); @@ -700,14 +713,11 @@ function normalizeIgnoreEndpoints(config) { config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => endpoint?.toLowerCase()); } } - return; - } - - if (process.env.INSTANA_IGNORE_ENDPOINTS) { + } else if (process.env.INSTANA_IGNORE_ENDPOINTS) { + // The environment variable name and its format are still under discussion. + // It is currently private and will not be documented or publicly shared. try { - logger.info('Ignore endpoints have been added via environment variable INSTANA_IGNORE_ENDPOINTS.'); config.tracing.ignoreEndpoints = JSON.parse(process.env.INSTANA_IGNORE_ENDPOINTS); - return; } catch (error) { logger.warn( `Failed to parse INSTANA_IGNORE_ENDPOINTS: ${process.env.INSTANA_IGNORE_ENDPOINTS}. Error: ${error.message}` diff --git a/packages/core/src/util/spanFilter.js b/packages/core/src/util/spanFilter.js index 3f7782c433..7e27f61fbe 100644 --- a/packages/core/src/util/spanFilter.js +++ b/packages/core/src/util/spanFilter.js @@ -20,7 +20,7 @@ function shouldIgnore(span, endpoints) { const operation = span.data?.[span.n]?.operation; if (operation && endpoints[span.n]) { - return endpoints[span.n].includes(operation); + return endpoints[span.n].some(op => op === operation); } return false; diff --git a/packages/core/test/tracing/backend_mappers/mapper_test.js b/packages/core/test/tracing/backend_mappers/mapper_test.js index 2e1f9bb0d4..94105c2c96 100644 --- a/packages/core/test/tracing/backend_mappers/mapper_test.js +++ b/packages/core/test/tracing/backend_mappers/mapper_test.js @@ -7,7 +7,7 @@ const expect = require('chai').expect; const { transform } = require('../../../src/tracing/backend_mappers'); -describe('BE span transformation test', () => { +describe('tracing/backend_mappers', () => { let span; beforeEach(() => { From 65428b0f78b9cb7557931858bc90fc7dfcd06354 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Tue, 3 Dec 2024 17:40:00 +0530 Subject: [PATCH 22/23] chore: updated code --- packages/collector/src/announceCycle/unannounced.js | 5 +++-- packages/collector/test/apps/agentStubControls.js | 2 ++ packages/core/src/util/normalizeConfig.js | 4 ++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/collector/src/announceCycle/unannounced.js b/packages/collector/src/announceCycle/unannounced.js index a826336575..b7cd75a1ff 100644 --- a/packages/collector/src/announceCycle/unannounced.js +++ b/packages/collector/src/announceCycle/unannounced.js @@ -225,9 +225,10 @@ function applySpanBatchingConfiguration(agentResponse) { /** * - The agent configuration currently uses a pipe ('|') as a separator for endpoints. - * - This function splits the string by both pipe ('|') and comma (',') to ensure compatibility. + * - This function supports both ('|') and comma (',') to ensure future compatibility. * - Additionally, it supports the `string[]` format for backward compatibility, - * as this was the previously used standard. + * as this was the previously used standard. The final design decision is not yet completed. + * https://github.ibm.com/instana/requests-for-discussion/pull/84 * * @param {AgentAnnounceResponse} agentResponse */ diff --git a/packages/collector/test/apps/agentStubControls.js b/packages/collector/test/apps/agentStubControls.js index 74fc4cc618..755d78af16 100644 --- a/packages/collector/test/apps/agentStubControls.js +++ b/packages/collector/test/apps/agentStubControls.js @@ -43,6 +43,8 @@ class AgentStubControls { env.KAFKA_TRACE_CORRELATION = opts.kafkaConfig.traceCorrelation.toString(); } } + // This is not the INSTANA_IGNORE_ENDPOINTS env. We use this "IGNORE_ENDPOINTS" env for the fake agent to + // serve the ignore endpoints config to our tracer. if (opts.ignoreEndpoints) { env.IGNORE_ENDPOINTS = JSON.stringify(opts.ignoreEndpoints); } diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 41cc61a5d9..285e4b5d14 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -723,5 +723,9 @@ function normalizeIgnoreEndpoints(config) { `Failed to parse INSTANA_IGNORE_ENDPOINTS: ${process.env.INSTANA_IGNORE_ENDPOINTS}. Error: ${error.message}` ); } + } else { + return; } + + logger.debug(`Ignore endpoints have been configured: ${JSON.stringify(config.tracing.ignoreEndpoints)}`); } From 97f7673b71f81cddd34937b4b7d964f3b569c9f1 Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Wed, 4 Dec 2024 14:18:41 +0530 Subject: [PATCH 23/23] chore: added string check in config --- packages/core/src/tracing/index.js | 2 +- packages/core/src/util/normalizeConfig.js | 18 +++++++++++------- packages/core/src/util/spanFilter.js | 7 ++++++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/core/src/tracing/index.js b/packages/core/src/tracing/index.js index 577284a3bb..2560cabdd4 100644 --- a/packages/core/src/tracing/index.js +++ b/packages/core/src/tracing/index.js @@ -120,7 +120,7 @@ if (customInstrumentations.length > 0) { */ /** - * @typedef {Object.} IgnoreEndpoints + * @typedef {Object.} IgnoreEndpoints */ /** @type {Array.} */ diff --git a/packages/core/src/util/normalizeConfig.js b/packages/core/src/util/normalizeConfig.js index 285e4b5d14..cad667baec 100644 --- a/packages/core/src/util/normalizeConfig.js +++ b/packages/core/src/util/normalizeConfig.js @@ -699,20 +699,24 @@ function normalizeIgnoreEndpoints(config) { } if (Object.keys(ignoreEndpoints).length) { - for (const [service, endpoints] of Object.entries(ignoreEndpoints)) { + Object.entries(ignoreEndpoints).forEach(([service, endpoints]) => { const normalizedService = service.toLowerCase(); - if (!Array.isArray(endpoints)) { + if (Array.isArray(endpoints)) { + config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => + typeof endpoint === 'string' ? endpoint.toLowerCase() : endpoint + ); + } else if (typeof endpoints === 'string') { + config.tracing.ignoreEndpoints[normalizedService] = [endpoints?.toLowerCase()]; + } else { logger.warn( - `Invalid configuration for ${normalizedService}: tracing.ignoreEndpoints.${normalizedService} is not an array. The value will be ignored: ${JSON.stringify( + `Invalid configuration for ${normalizedService}: tracing.ignoreEndpoints.${normalizedService} is neither a string nor an array. Value will be ignored: ${JSON.stringify( endpoints )}` ); - config.tracing.ignoreEndpoints[normalizedService] = null; // Set to null for invalid configuration - } else { - config.tracing.ignoreEndpoints[normalizedService] = endpoints.map(endpoint => endpoint?.toLowerCase()); + config.tracing.ignoreEndpoints[normalizedService] = null; } - } + }); } else if (process.env.INSTANA_IGNORE_ENDPOINTS) { // The environment variable name and its format are still under discussion. // It is currently private and will not be documented or publicly shared. diff --git a/packages/core/src/util/spanFilter.js b/packages/core/src/util/spanFilter.js index 7e27f61fbe..b75d205c03 100644 --- a/packages/core/src/util/spanFilter.js +++ b/packages/core/src/util/spanFilter.js @@ -20,7 +20,12 @@ function shouldIgnore(span, endpoints) { const operation = span.data?.[span.n]?.operation; if (operation && endpoints[span.n]) { - return endpoints[span.n].some(op => op === operation); + const endpoint = endpoints[span.n]; + if (Array.isArray(endpoint)) { + return endpoint.some(op => op === operation); + } else if (typeof endpoint === 'string') { + return endpoint === operation; + } } return false;