From 575654e7ffb83996c596dfe7c2d72a1b5a83d96b Mon Sep 17 00:00:00 2001 From: Arya Mohanan Date: Tue, 3 Dec 2024 16:18:05 +0530 Subject: [PATCH] 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(() => {