From 2b0e70d2bb10ee729ef7a7dff9768620d2e000f9 Mon Sep 17 00:00:00 2001 From: Arya Date: Fri, 21 Feb 2025 17:27:04 +0530 Subject: [PATCH] refactor: improved ignoring endpoints implementation (#1558) ref INSTA-26148 --- packages/core/src/tracing/cls.js | 95 ++++++++++++++++++- packages/core/src/tracing/index.js | 6 +- .../cloud/aws-sdk/v2/dynamodb.js | 11 ++- .../cloud/aws-sdk/v3/dynamodb.js | 9 +- .../instrumentation/database/ioredis.js | 39 ++++---- .../tracing/instrumentation/database/redis.js | 33 ++++--- packages/core/src/tracing/spanBuffer.js | 26 +---- packages/core/test/tracing/cls_test.js | 60 ++++++++++++ packages/core/test/tracing/spanBuffer_test.js | 26 +---- 9 files changed, 214 insertions(+), 91 deletions(-) diff --git a/packages/core/src/tracing/cls.js b/packages/core/src/tracing/cls.js index 0793b7ab98..b84ec7c0fa 100644 --- a/packages/core/src/tracing/cls.js +++ b/packages/core/src/tracing/cls.js @@ -11,6 +11,8 @@ const tracingUtil = require('./tracingUtil'); const { ENTRY, EXIT, INTERMEDIATE, isExitSpan } = require('./constants'); const hooked = require('./clsHooked'); const tracingMetrics = require('./metrics'); +const { applyFilter } = require('../util/spanFilter'); + /** @type {import('../core').GenericLogger} */ let logger; logger = require('../logger').getLogger('tracing/cls', newLogger => { @@ -30,6 +32,10 @@ let serviceName; let processIdentityProvider = null; /** @type {Boolean} */ let allowRootExitSpan; +/** + * @type {import('../tracing').IgnoreEndpoints} + */ +let ignoreEndpoints; /* * Access the Instana namespace in continuation local storage. @@ -51,13 +57,23 @@ function init(config, _processIdentityProvider) { } processIdentityProvider = _processIdentityProvider; allowRootExitSpan = config?.tracing?.allowRootExitSpan; + ignoreEndpoints = config?.tracing?.ignoreEndpoints; +} +/** + * @param {import('../util/normalizeConfig').AgentConfig} extraConfig + */ +function activate(extraConfig) { + if (extraConfig?.tracing?.ignoreEndpoints) { + ignoreEndpoints = extraConfig.tracing.ignoreEndpoints; + } } class InstanaSpan { /** * @param {string} name + * @param {object} [data] */ - constructor(name) { + constructor(name, data) { // properties that part of our span model this.t = undefined; this.s = undefined; @@ -98,7 +114,7 @@ class InstanaSpan { /** @type {Array.<*>} */ this.stack = []; /** @type {Object.} */ - this.data = {}; + this.data = data || {}; // Properties for the span that are only used internally but will not be transmitted to the agent/backend, // therefore defined as non-enumerabled. NOTE: If you add a new property, make sure that it is also defined as @@ -256,18 +272,20 @@ class InstanaIgnoredSpan extends InstanaSpan { * @param {string} [spanAttributes.traceId] * @param {string} [spanAttributes.parentSpanId] * @param {import('./w3c_trace_context/W3cTraceContext')} [spanAttributes.w3cTraceContext] + * @param {Object} [spanAttributes.spanData] * @returns {InstanaSpan} */ function startSpan(spanAttributes = {}) { - let { spanName, kind, traceId, parentSpanId, w3cTraceContext } = spanAttributes; + let { spanName, kind, traceId, parentSpanId, w3cTraceContext, spanData } = spanAttributes; tracingMetrics.incrementOpened(); if (!kind || (kind !== ENTRY && kind !== EXIT && kind !== INTERMEDIATE)) { logger.warn(`Invalid span (${spanName}) without kind/with invalid kind: ${kind}, assuming EXIT.`); kind = EXIT; } - const span = new InstanaSpan(spanName); + + const span = new InstanaSpan(spanName, spanData); span.k = kind; const parentSpan = getCurrentSpan(); @@ -313,6 +331,20 @@ function startSpan(spanAttributes = {}) { span.addCleanup(ns.set(w3cTraceContextKey, w3cTraceContext)); } + const filteredSpan = applySpanFilter(span); + + // If the span was filtered out, we do not process it further. + // Instead, we return an 'InstanaIgnoredSpan' instance to explicitly indicate that it was excluded from tracing. + if (!filteredSpan) { + return setIgnoredSpan({ + spanName: span.n, + kind: span.k, + traceId: span.t, + parentId: span.p, + data: span.data + }); + } + if (span.k === ENTRY) { // Make the entry span available independently (even if getCurrentSpan would return an intermediate or an exit at // any given moment). This is used by the instrumentations of web frameworks like Express.js to add path templates @@ -367,6 +399,49 @@ function putPseudoSpan(spanName, kind, traceId, spanId) { return span; } +/** + * Adds an ignored span to the CLS context, serving as a holder for a trace ID and span ID. + * Customers can access the current span via `instana.currentSpan()`, and we avoid returning a "NoopSpan". + * We need this ignored span instance to ensure the trace ID remains accessible for future cases such as propagating + * the trace ID although suppression is on to reactivate a trace. + * These spans will not be sent to the backend. + * @param {Object} options - The options for the span. + * @param {string} options.spanName + * @param {number} options.kind + * @param {string} options.traceId + * @param {string} options.parentId + * @param {Object} options.data + */ +function setIgnoredSpan({ spanName, kind, traceId, parentId, data = {} }) { + if (!kind || (kind !== ENTRY && kind !== EXIT && kind !== INTERMEDIATE)) { + logger.warn(`Invalid ignored span (${spanName}) without kind/with invalid kind: ${kind}, assuming EXIT.`); + kind = EXIT; + } + + const span = new InstanaIgnoredSpan(spanName, data); + span.k = kind; + span.t = traceId; + span.p = parentId; + + // Setting the 'parentId' of the span to 'span.s' to ensure trace continuity. + // Although this span doesn't physically exist, we are ignoring it, but retaining its parentId. + // This parentId is propagated downstream. + // The spanId does not need to be retained. + span.s = parentId; + + if (span.k === ENTRY) { + // Make the entry span available independently (even if getCurrentSpan would return an intermediate or an exit at + // any given moment). This is used by the instrumentations of web frameworks like Express.js to add path templates + // and error messages to the entry span. + span.addCleanup(ns.set(currentEntrySpanKey, span)); + } + + // Set the span object as the currently active span in the active CLS context and also add a cleanup hook for when + // this span is transmitted. + span.addCleanup(ns.set(currentSpanKey, span)); + return span; +} + /* * Get the currently active entry span. */ @@ -622,6 +697,15 @@ function skipExitTracing(options) { return skip; } +/** + * @param {InstanaSpan | import("../core").InstanaBaseSpan} span + */ +function applySpanFilter(span) { + if (ignoreEndpoints) { + return applyFilter({ span, ignoreEndpoints }); + } + return span; +} module.exports = { skipExitTracing, @@ -648,5 +732,6 @@ module.exports = { enterAsyncContext, leaveAsyncContext, runInAsyncContext, - runPromiseInAsyncContext + runPromiseInAsyncContext, + activate }; diff --git a/packages/core/src/tracing/index.js b/packages/core/src/tracing/index.js index b2d9612846..6aed850465 100644 --- a/packages/core/src/tracing/index.js +++ b/packages/core/src/tracing/index.js @@ -15,14 +15,14 @@ const tracingUtil = require('./tracingUtil'); const spanBuffer = require('./spanBuffer'); const supportedVersion = require('./supportedVersion'); const { otelInstrumentations } = require('./opentelemetry-instrumentations'); +const cls = require('./cls'); const coreUtil = require('../util'); let tracingEnabled = false; let tracingActivated = false; let instrumenationsInitialized = false; let automaticTracingEnabled = false; -/** @type {import('./cls')} */ -let cls = null; + /** @type {import('../util/normalizeConfig').InstanaConfig} */ let config = null; @@ -203,7 +203,6 @@ exports.init = function init(_config, downstreamConnection, _processIdentityProv tracingHeaders.init(config); spanBuffer.init(config, downstreamConnection); opentracing.init(config, automaticTracingEnabled, processIdentityProvider); - cls = require('./cls'); cls.init(config, processIdentityProvider); sdk.init(cls); @@ -259,6 +258,7 @@ function initInstrumenations(_config) { exports.activate = function activate(extraConfig = {}) { if (tracingEnabled && !tracingActivated) { tracingActivated = true; + cls.activate(extraConfig); spanBuffer.activate(extraConfig); opentracing.activate(); sdk.activate(); diff --git a/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v2/dynamodb.js b/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v2/dynamodb.js index 96893e9500..20604e646e 100644 --- a/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v2/dynamodb.js +++ b/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v2/dynamodb.js @@ -38,14 +38,19 @@ class InstanaAWSDynamoDB extends InstanaAWSProduct { return cls.ns.runAndReturn(() => { const self = this; + + // Data attributes: operation, table + const spanData = { + [this.spanName]: this.buildSpanData(ctx, originalArgs[0], originalArgs[1]) + }; + const span = cls.startSpan({ spanName: this.spanName, - kind: EXIT + kind: EXIT, + spanData }); span.ts = Date.now(); span.stack = tracingUtil.getStackTrace(this.instrumentedMakeRequest, 1); - // Data attribs: op and table - span.data[this.spanName] = this.buildSpanData(ctx, originalArgs[0], originalArgs[1]); if (typeof originalArgs[2] === 'function') { // callback case diff --git a/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v3/dynamodb.js b/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v3/dynamodb.js index 2553268e39..9c53743cb5 100644 --- a/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v3/dynamodb.js +++ b/packages/core/src/tracing/instrumentation/cloud/aws-sdk/v3/dynamodb.js @@ -23,16 +23,19 @@ class InstanaAWSDynamoDB extends InstanaAWSProduct { } const command = smithySendArgs[0]; - return cls.ns.runAndReturn(() => { const self = this; + + const spanData = { + [this.spanName]: this.buildSpanData(command.constructor.name, command.input) + }; const span = cls.startSpan({ spanName: this.spanName, - kind: EXIT + kind: EXIT, + spanData }); span.ts = Date.now(); span.stack = tracingUtil.getStackTrace(this.instrumentedSmithySend, 1); - span.data[this.spanName] = this.buildSpanData(command.constructor.name, command.input); this.captureRegion(ctx, span); if (typeof smithySendArgs[1] === 'function') { diff --git a/packages/core/src/tracing/instrumentation/database/ioredis.js b/packages/core/src/tracing/instrumentation/database/ioredis.js index 650acbcd93..8bbb02dea8 100644 --- a/packages/core/src/tracing/instrumentation/database/ioredis.js +++ b/packages/core/src/tracing/instrumentation/database/ioredis.js @@ -76,24 +76,26 @@ function instrumentSendCommand(original) { } const argsForOriginal = arguments; + const connection = `${client.options.host}:${client.options.port}`; + + // TODO: https://jsw.ibm.com/browse/INSTA-14540 + // if (client.isCluster) { + // connection = client.startupNodes.map(node => `${node.host}:${node.port}`).join(','); + return cls.ns.runAndReturn(() => { + const spanData = { + redis: { + connection, + operation: command.name.toLowerCase() + } + }; const span = cls.startSpan({ spanName: exports.spanName, - kind: constants.EXIT + kind: constants.EXIT, + spanData }); span.stack = tracingUtil.getStackTrace(wrappedInternalSendCommand); - const connection = `${client.options.host}:${client.options.port}`; - - // TODO: https://jsw.ibm.com/browse/INSTA-14540 - // if (client.isCluster) { - // connection = client.startupNodes.map(node => `${node.host}:${node.port}`).join(','); - - span.data.redis = { - connection, - operation: command.name.toLowerCase() - }; - callback = cls.ns.bind(onResult); command.promise.then( // make sure that the first parameter is never truthy @@ -156,15 +158,18 @@ function instrumentMultiOrPipelineCommand(commandName, original) { // create a new cls context parent to track the multi/pipeline child calls const clsContextForMultiOrPipeline = cls.ns.createContext(); cls.ns.enter(clsContextForMultiOrPipeline); + const spanData = { + redis: { + connection, + operation: commandName + } + }; const span = cls.startSpan({ spanName: exports.spanName, - kind: constants.EXIT + kind: constants.EXIT, + spanData }); span.stack = tracingUtil.getStackTrace(wrappedInternalMultiOrPipelineCommand); - span.data.redis = { - connection, - operation: commandName - }; const multiOrPipeline = original.apply(this, arguments); shimmer.wrap( diff --git a/packages/core/src/tracing/instrumentation/database/redis.js b/packages/core/src/tracing/instrumentation/database/redis.js index 02fc059626..eb05bb30c6 100644 --- a/packages/core/src/tracing/instrumentation/database/redis.js +++ b/packages/core/src/tracing/instrumentation/database/redis.js @@ -236,17 +236,19 @@ function instrumentCommand(original, command, address, cbStyle) { } return cls.ns.runAndReturn(() => { + const spanData = { + redis: { + connection: address || origCtx.address, + operation: command + } + }; const span = cls.startSpan({ spanName: exports.spanName, - kind: constants.EXIT + kind: constants.EXIT, + spanData }); span.stack = tracingUtil.getStackTrace(instrumentCommand); - span.data.redis = { - connection: address || origCtx.address, - operation: command - }; - let userProvidedCallback; if (cbStyle) { @@ -315,27 +317,30 @@ function instrumentMultiExec(origCtx, origArgs, original, address, isAtomic, cbS return cls.ns.runAndReturn(() => { let span; - + const spanData = { + redis: { + connection: address, + // pipeline = batch + operation: isAtomic ? 'multi' : 'pipeline' + } + }; if (skipExitResult.allowRootExitSpan) { span = cls.startSpan({ spanName: exports.spanName, - kind: constants.EXIT + kind: constants.EXIT, + spanData }); } else { span = cls.startSpan({ spanName: exports.spanName, kind: constants.EXIT, traceId: parentSpan.t, - parentSpanId: parentSpan.s + parentSpanId: parentSpan.s, + spanData }); } span.stack = tracingUtil.getStackTrace(instrumentMultiExec); - span.data.redis = { - connection: address, - // pipeline = batch - operation: isAtomic ? 'multi' : 'pipeline' - }; const subCommands = (span.data.redis.subCommands = []); let legacyMultiMarkerHasBeenSeen = false; diff --git a/packages/core/src/tracing/spanBuffer.js b/packages/core/src/tracing/spanBuffer.js index 3da61b1114..11f3c13d29 100644 --- a/packages/core/src/tracing/spanBuffer.js +++ b/packages/core/src/tracing/spanBuffer.js @@ -6,7 +6,6 @@ 'use strict'; const tracingMetrics = require('./metrics'); -const { applyFilter } = require('../util/spanFilter'); const { transform } = require('./backend_mappers'); /** @type {import('../core').GenericLogger} */ @@ -89,10 +88,6 @@ const batchBucketWidth = 18; /** @type {BatchingBucketMap} */ const batchingBuckets = new Map(); -/** - * @type {import('../tracing').IgnoreEndpoints} - */ -let ignoreEndpoints; /** * @param {import('../util/normalizeConfig').InstanaConfig} config @@ -104,7 +99,6 @@ 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; @@ -138,9 +132,6 @@ exports.activate = function activate(extraConfig) { if (extraConfig.tracing.spanBatchingEnabled) { batchingEnabled = true; } - if (extraConfig.tracing.ignoreEndpoints) { - ignoreEndpoints = extraConfig.tracing.ignoreEndpoints; - } } isActive = true; @@ -191,12 +182,8 @@ exports.addSpan = function (span) { return; } - // Process the span, apply any transformations, and implement filtering if necessary. - const processedSpan = processSpan(span); - if (!processedSpan) { - return; - } - span = processedSpan; + // Transform internal span data format into external (backend) readable format. + span = applySpanTransformation(span); if (span.t == null) { logger.warn(`Span of type ${span.n} has no trace ID. Not transmitting this span`); @@ -506,13 +493,6 @@ function removeSpansIfNecessary() { * @param {import('../core').InstanaBaseSpan} span * @returns {import('../core').InstanaBaseSpan} span */ -function processSpan(span) { - if (ignoreEndpoints) { - span = applyFilter({ span, ignoreEndpoints }); - - if (!span) { - return null; - } - } +function applySpanTransformation(span) { return transform(span); } diff --git a/packages/core/test/tracing/cls_test.js b/packages/core/test/tracing/cls_test.js index 672b23f710..dafb5165ee 100644 --- a/packages/core/test/tracing/cls_test.js +++ b/packages/core/test/tracing/cls_test.js @@ -369,4 +369,64 @@ describe('tracing/cls', () => { expect(reducedSpan.stack).to.not.exist; }); }); + it('should return the span with correct data when span data is provided', () => { + cls.ns.run(() => { + const span = cls.startSpan({ + spanName: 'redis', + kind: constants.EXIT, + spanData: { + redis: { + operation: 'get', + connection: 'http://localhost' + } + } + }); + expect(span).to.be.an('object'); + expect(span.n).to.equal('redis'); + expect(span.t).to.be.a('string'); + expect(span.s).to.be.a('string'); + expect(span.k).to.equal(constants.EXIT); + expect(span.error).to.not.exist; + expect(span.ec).to.equal(0); + expect(span.ts).to.be.a('number'); + expect(span.d).to.equal(0); + expect(span.stack).to.deep.equal([]); + expect(span.data).to.be.an('object'); + expect(Object.keys(span.data)).to.have.lengthOf(1); + expect(span.data.redis).to.be.exist; + expect(span.data.redis.operation).to.equal('get'); + expect(span.data.redis.connection).to.equal('http://localhost'); + }); + }); + it('should return InstanaIgnoredSpan when the span is configured to be ignored', () => { + cls.ns.run(() => { + cls.init({ + tracing: { + ignoreEndpoints: { + redis: ['get'] + } + } + }); + const span = cls.startSpan({ + spanName: 'redis', + kind: constants.EXIT, + spanData: { + redis: { + operation: 'get', + connection: 'http://localhost' + } + } + }); + expect(span.constructor.name).to.equal('InstanaIgnoredSpan'); + expect(span).to.be.an('object'); + expect(span.n).to.equal('redis'); + expect(span.t).to.be.a('string'); + expect(span.k).to.equal(constants.EXIT); + expect(span.data).to.be.an('object'); + expect(Object.keys(span.data)).to.have.lengthOf(1); + expect(span.data.redis).to.be.exist; + expect(span.data.redis.operation).to.equal('get'); + expect(span.data.redis.connection).to.equal('http://localhost'); + }); + }); }); diff --git a/packages/core/test/tracing/spanBuffer_test.js b/packages/core/test/tracing/spanBuffer_test.js index 9787bcbe79..b9fc2c6602 100644 --- a/packages/core/test/tracing/spanBuffer_test.js +++ b/packages/core/test/tracing/spanBuffer_test.js @@ -566,21 +566,7 @@ describe('tracing/spanBuffer', () => { verifyNoBatching(span1, span2); }); }); - describe('when ignoreEndpoints is configured', () => { - before(() => { - spanBuffer.init( - { - tracing: { - ignoreEndpoints: { redis: ['get'] } - } - }, - { - /* downstreamConnection */ - sendSpans: function () {} - } - ); - }); - + describe('when applying span transformations', () => { beforeEach(() => spanBuffer.activate()); afterEach(() => spanBuffer.deactivate()); @@ -597,13 +583,7 @@ describe('tracing/spanBuffer', () => { } }; - 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 the redis span if the operation is not specified in the ignoreEndpoints config', () => { + it('should correctly transform the Redis span by renaming the operation property', () => { span.data.redis.operation = 'set'; spanBuffer.addSpan(span); const spans = spanBuffer.getAndResetSpans(); @@ -611,7 +591,7 @@ describe('tracing/spanBuffer', () => { 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', () => { + it('should return the span unchanged for non-mapped types', () => { span.n = 'http'; spanBuffer.addSpan(span); const spans = spanBuffer.getAndResetSpans();