From 77cbf818417c7eaa2a727fb736898875c96409d6 Mon Sep 17 00:00:00 2001 From: Abhilash Date: Wed, 10 Dec 2025 01:59:17 +0530 Subject: [PATCH 1/3] refactor: updated treacingutil to allow custom key path for sdk --- packages/core/src/tracing/tracingUtil.js | 61 +++++++++--- .../core/test/tracing/tracingUtil_test.js | 92 +++++++++++++++++++ 2 files changed, 139 insertions(+), 14 deletions(-) diff --git a/packages/core/src/tracing/tracingUtil.js b/packages/core/src/tracing/tracingUtil.js index 9fdbaacb9b..eeb0093e39 100644 --- a/packages/core/src/tracing/tracingUtil.js +++ b/packages/core/src/tracing/tracingUtil.js @@ -278,9 +278,18 @@ exports.findCallback = (/** @type {string | any[]} */ originalArgs) => { }; /** + * Sets error details on a span for a specific technology. + * Handles different error formats: strings, objects with details/message/code properties. + * Supports nested paths for SDK spans via dot-separated strings or arrays. + * + * Examples: + * - setErrorDetails(span, error, 'nats') // flat key + * - setErrorDetails(span, error, 'sdk.custom.tags.message') // dot-separated string + * - setErrorDetails(span, error, ['sdk', 'custom', 'tags', 'message']) // array path + * * @param {import('../core').InstanaBaseSpan} span - The span to update - * @param {Error | string} error - * @param {string} technology - The technology name (e.g., 'mysql', 'pg', 'http') + * @param {Error | string | Object} error - The error object, error string, or object with error properties + * @param {string | Array} technology - The technology name or nested path */ exports.setErrorDetails = function setErrorDetails(span, error, technology) { if (!error) { @@ -297,20 +306,44 @@ exports.setErrorDetails = function setErrorDetails(span, error, technology) { normalizedError = error; } - if (technology && span.data?.[technology]) { - // This extra check allows instrumentations to override the error property (not recommended) - if (!span.data[technology].error) { - let combinedMessage; - - if (normalizedError?.details) { - combinedMessage = `${normalizedError.name || 'Error'}: ${normalizedError.details}`; - } else if (normalizedError?.message) { - combinedMessage = `${normalizedError.name || 'Error'}: ${normalizedError.message}`; - } else { - combinedMessage = normalizedError?.code || 'No error message found.'; + const extractErrorMessage = () => { + if (normalizedError?.details) { + return `${normalizedError.name || 'Error'}: ${normalizedError.details}`; + } else if (normalizedError?.message) { + return `${normalizedError.name || 'Error'}: ${normalizedError.message}`; + } else { + return normalizedError?.code || 'No error message found.'; + } + }; + + let errorPath = null; + if (Array.isArray(technology)) { + errorPath = technology; + } else if (typeof technology === 'string' && technology.includes('.')) { + errorPath = technology.split('.'); + } + + if (errorPath) { + let target = span.data; + + // Traverse the object path and create missing nested objects along the way + // Without this, deeper properties would fail to assign if their parent objects don't exist + for (let i = 0; i < errorPath.length - 1; i++) { + const key = errorPath[i]; + if (!target[key]) { + target[key] = {}; } + target = target[key]; + } - span.data[technology].error = combinedMessage.substring(0, 200); + const errorKey = errorPath[errorPath.length - 1]; + + if (!target[errorKey]) { + target[errorKey] = extractErrorMessage().substring(0, 200); + } + } else if (typeof technology === 'string' && technology && span.data?.[technology]) { + if (!span.data[technology].error) { + span.data[technology].error = extractErrorMessage().substring(0, 200); } } diff --git a/packages/core/test/tracing/tracingUtil_test.js b/packages/core/test/tracing/tracingUtil_test.js index e079040f11..620e14361e 100644 --- a/packages/core/test/tracing/tracingUtil_test.js +++ b/packages/core/test/tracing/tracingUtil_test.js @@ -567,5 +567,97 @@ describe('tracing/tracingUtil', () => { expect(span.data.rpc.error).to.contain('UNAVAILABLE: Connection refused'); }); + + describe('SDK spans with nested error paths', () => { + it('should not overwrite existing SDK error', () => { + const span = { + data: { + sdk: { + custom: { + tags: { + message: 'Existing SDK error' + } + } + } + } + }; + const error = 'New SDK error'; + setErrorDetails(span, error, ['sdk', 'custom', 'tags', 'message']); + + expect(span.data.sdk.custom.tags.message).to.equal('Existing SDK error'); + }); + + it('should truncate SDK error messages to 200 characters', () => { + const span = { + data: {} + }; + const longError = 'b'.repeat(600); + setErrorDetails(span, longError, ['sdk', 'custom', 'tags', 'message']); + + expect(span.data.sdk.custom.tags.message).to.have.lengthOf(200); + expect(span.data.sdk.custom.tags.message).to.equal(`Error: ${'b'.repeat(193)}`); + }); + + it('should create nested structure if it does not exist', () => { + const span = { + data: {} + }; + const error = 'Test error'; + setErrorDetails(span, error, ['sdk', 'custom', 'tags', 'error']); + + expect(span.data.sdk).to.be.an('object'); + expect(span.data.sdk.custom).to.be.an('object'); + expect(span.data.sdk.custom.tags).to.be.an('object'); + expect(span.data.sdk.custom.tags.error).to.equal('Error: Test error'); + }); + + it('should handle different nested paths', () => { + const span = { + data: {} + }; + const error = 'Custom path error'; + setErrorDetails(span, error, ['custom', 'nested', 'path', 'errorField']); + + expect(span.data.custom.nested.path.errorField).to.equal('Error: Custom path error'); + }); + + it('should handle Error object with name and message for SDK spans', () => { + const span = { + data: {} + }; + const error = new TypeError('Type mismatch'); + setErrorDetails(span, error, ['sdk', 'custom', 'tags', 'message']); + + expect(span.data.sdk.custom.tags.message).to.match(/TypeError: Type mismatch/); + expect(span.stack).to.match(/Type mismatch/); + }); + + it('should handle SDK error with dot-separated string path', () => { + const span = { + data: {} + }; + const error = new Error('SDK error via string path'); + setErrorDetails(span, error, 'sdk.custom.tags.message'); + + expect(span.data.sdk).to.exist; + expect(span.data.sdk.custom).to.exist; + expect(span.data.sdk.custom.tags).to.exist; + expect(span.data.sdk.custom.tags.message).to.match(/Error: SDK error via string path/); + expect(span.stack).to.be.a('string'); + expect(span.stack).to.match(/SDK error via string path/); + }); + + it('should handle both array and string paths equivalently', () => { + const span1 = { data: {} }; + const span2 = { data: {} }; + const error = new Error('Test error'); + + setErrorDetails(span1, error, ['sdk', 'custom', 'tags', 'message']); + setErrorDetails(span2, error, 'sdk.custom.tags.message'); + + expect(span1.data.sdk.custom.tags.message).to.equal(span2.data.sdk.custom.tags.message); + expect(span1.data.sdk.custom.tags.message).to.match(/Error: Test error/); + }); + }); }); }); From 27cf79aa9730e2da3a5ef32e3182a561c54a6c90 Mon Sep 17 00:00:00 2001 From: Abhilash Date: Wed, 10 Dec 2025 11:06:38 +0530 Subject: [PATCH 2/3] refactor: updated sdk to use setErrorDetails function --- packages/collector/test/tracing/sdk/test.js | 24 ++++++++++++++++----- packages/core/src/tracing/sdk/sdk.js | 16 +++----------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/packages/collector/test/tracing/sdk/test.js b/packages/collector/test/tracing/sdk/test.js index ebe8ef5601..57b022f0e8 100644 --- a/packages/collector/test/tracing/sdk/test.js +++ b/packages/collector/test/tracing/sdk/test.js @@ -710,8 +710,7 @@ mochaSuiteFn('tracing/sdk', function () { if (error) { expectations = expectations.concat([ span => expect(span.ec).to.equal(1), - span => expect(span.data.sdk.custom.tags.message).to.contain('Error: Boom!\n'), - span => expect(span.data.sdk.custom.tags.message).to.contain('packages/collector/test/tracing/sdk/app.js') + span => expect(span.data.sdk.custom.tags.message).to.contain('Error: Boom!') ]); } else { expectations.push(span => expect(span.ec).to.equal(0)); @@ -721,8 +720,17 @@ mochaSuiteFn('tracing/sdk', function () { span => expect(span.data.sdk).to.exist, span => expect(span.data.sdk.name).to.equal('custom-entry'), span => expect(span.data.sdk.type).to.equal(constants.SDK.ENTRY), - span => expect(span.stack[0].c).to.match(/test\/tracing\/sdk\/app.js$/), - span => expect(span.stack[0].m).to.match(functionName) + span => { + // TODO: Remove this !error condition when the span.stack to array conversion is implemented + if (functionName && !error) { + // When there's an error, span.stack is overwritten with error stack string, not stack frames array + expect(span.stack[0].c).to.match(/test\/tracing\/sdk\/app.js$/); + expect(span.stack[0].m).to.match(functionName); + } else if (error) { + // For errors, span.stack is a string containing the error stack trace + expect(span.stack).to.be.a('string'); + } + } ]); tagsAt = tagsAt || 'none'; @@ -828,9 +836,15 @@ mochaSuiteFn('tracing/sdk', function () { : expect(span.data.sdk.name).to.equal(kind === 'INTERMEDIATE' ? 'intermediate-file-access' : 'file-access'), span => expect(span.data.sdk.type).to.equal(constants.SDK[kind]), span => { - if (functionName) { + // TODO: Remove this !error condition when the span.stack to array conversion is implemented + if (functionName && !error) { + // When there's an error, span.stack is overwritten with error stack string, not stack frames array expect(span.stack[0].c).to.match(/test\/tracing\/sdk\/app.js$/); expect(span.stack[0].m).to.match(functionName); + } else if (error) { + // For errors, span.stack is a string containing the error stack trace + expect(span.stack).to.be.a('string'); + expect(span.stack).to.contain('Error: ENOENT'); } }, span => expect(span.data.sdk.custom).to.be.an('object'), diff --git a/packages/core/src/tracing/sdk/sdk.js b/packages/core/src/tracing/sdk/sdk.js index cc072053ed..d939c08ffa 100644 --- a/packages/core/src/tracing/sdk/sdk.js +++ b/packages/core/src/tracing/sdk/sdk.js @@ -321,19 +321,9 @@ exports.generate = function (isCallbackApi) { if (error) { span.ec = 1; - if (!span.data.sdk.custom) { - span.data.sdk.custom = { - tags: {} - }; - } - if (!span.data.sdk.custom.tags) { - span.data.sdk.custom.tags = {}; - } - if (span.data.sdk.custom.tags.message == null) { - // TODO: fix sdk error capture and handling - // This fn used getErrorDetails for now and will need to change - span.data.sdk.custom.tags.message = tracingUtil.getErrorDetails(error); - } + // since the key for error message is different in sdk, we pass the complete path as dot(.) separated string + // also it can be passed as an array like ["sdk", "custom", "tags", "message"] + tracingUtil.setErrorDetails(span, error, 'sdk.custom.tags.message'); } if (span.data.sdk.custom && tags) { From b62f51a173e413ff43cdb67c213da98e3de8ee75 Mon Sep 17 00:00:00 2001 From: Abhilash Date: Wed, 10 Dec 2025 11:21:31 +0530 Subject: [PATCH 3/3] test: removed unused fn getErrorDetails --- packages/core/src/tracing/tracingUtil.js | 11 ---------- .../core/test/tracing/tracingUtil_test.js | 20 ------------------- 2 files changed, 31 deletions(-) diff --git a/packages/core/src/tracing/tracingUtil.js b/packages/core/src/tracing/tracingUtil.js index eeb0093e39..734209b688 100644 --- a/packages/core/src/tracing/tracingUtil.js +++ b/packages/core/src/tracing/tracingUtil.js @@ -159,17 +159,6 @@ exports.renderTraceContextToBuffer = function renderTraceContextToBuffer(span) { return exports.unsignedHexStringsToBuffer(span.t, span.s); }; -/** - * @param {Error} err - * @returns {string} - */ -exports.getErrorDetails = function getErrorDetails(err) { - if (err == null) { - return undefined; - } - return String(err.stack || err.message || err).substring(0, 500); -}; - /** * @param {string} stmt * @returns {string} diff --git a/packages/core/test/tracing/tracingUtil_test.js b/packages/core/test/tracing/tracingUtil_test.js index 620e14361e..825117f9a2 100644 --- a/packages/core/test/tracing/tracingUtil_test.js +++ b/packages/core/test/tracing/tracingUtil_test.js @@ -18,7 +18,6 @@ const { generateRandomId, generateRandomSpanId, generateRandomTraceId, - getErrorDetails, readTraceContextFromBuffer, sanitizeConnectionStr, unsignedHexStringToBuffer, @@ -357,25 +356,6 @@ describe('tracing/tracingUtil', () => { } }); - describe('getErrorDetails', () => { - it('must not fail on null/undefined', () => { - expect(getErrorDetails(null)).to.equal(undefined); - expect(getErrorDetails(undefined)).to.equal(undefined); - }); - - it('must use error stack when available', () => { - expect(getErrorDetails(new Error('Whhoooopppppss'))).to.match(/Whhoooopppppss/); - }); - - it('must use error message when available', () => { - expect(getErrorDetails({ message: 'Whhoooopppppss' })).to.match(/Whhoooopppppss/); - }); - - it('must use the whole provided error when all else fails', () => { - expect(getErrorDetails('Whhoooopppppss')).to.match(/Whhoooopppppss/); - }); - }); - describe('sanitizeConnectionStr', () => { it('should redact password at the start of the connection string', () => { expect(