diff --git a/packages/collector/test/tracing/databases/mysql/app.js b/packages/collector/test/tracing/databases/mysql/app.js index 9e2600c628..08c89151fe 100644 --- a/packages/collector/test/tracing/databases/mysql/app.js +++ b/packages/collector/test/tracing/databases/mysql/app.js @@ -200,6 +200,14 @@ app.post('/valuesAndCall', (req, res) => { } }); +app.post('/error', (req, res) => { + if (driver === 'mysql2/promise') { + triggerErrorWithPromises(req, res); + } else { + triggerError(req, res); + } +}); + app.listen(port, () => { log( `Listening on port: ${process.env.APP_PORT} (driver: ${driver}, access: ${accessFunction}, cluster: ${useCluster})` @@ -305,6 +313,51 @@ function insertValuesWithPromisesAndCall(req, res) { }); } +function triggerError(req, res) { + pool.getConnection((err, connection) => { + if (err) { + log('Failed to get connection', err); + res.sendStatus(500); + return; + } + + // Execute an invalid SQL query to trigger an error + connection[accessFunction]('SELECT * FROM non_existent_table', queryError => { + connection.release(); + + if (queryError) { + log('Expected error occurred', queryError); + res.sendStatus(500); + return; + } + + res.sendStatus(200); + }); + }); +} + +function triggerErrorWithPromises(req, res) { + pool + .getConnection() + .then(connection => { + // Execute an invalid SQL query to trigger an error + wrapAccess(connection, 'SELECT * FROM non_existent_table', null, queryError => { + connection.release(); + + if (queryError) { + log('Expected error occurred', queryError); + res.sendStatus(500); + } else { + res.sendStatus(200); + } + }); + }) + .catch(err => { + log('Failed to get connection', err); + res.sendStatus(500); + }); +} + function log() { const args = Array.prototype.slice.call(arguments); args[0] = logPrefix + args[0]; diff --git a/packages/collector/test/tracing/databases/mysql/test.js b/packages/collector/test/tracing/databases/mysql/test.js index 0b0c4f2f69..f53ce17855 100644 --- a/packages/collector/test/tracing/databases/mysql/test.js +++ b/packages/collector/test/tracing/databases/mysql/test.js @@ -320,4 +320,53 @@ function test(env, agentControls) { }) ); })); + + it('must replace stack trace with error stack when query fails', () => + controls + .sendRequest({ + method: 'POST', + path: '/error' + }) + .then(() => + testUtils.retry(() => + agentControls.getSpans().then(spans => { + expect(spans.length).to.equal(2); + const entrySpan = testUtils.expectAtLeastOneMatching(spans, [ + span => expect(span.n).to.equal('node.http.server'), + span => expect(span.f.e).to.equal(String(controls.getPid())), + span => expect(span.f.h).to.equal('agent-stub-uuid') + ]); + + const mysqlSpan = testUtils.expectAtLeastOneMatching(spans, [ + span => expect(span.t).to.equal(entrySpan.t), + span => expect(span.p).to.equal(entrySpan.s), + span => expect(span.n).to.equal('mysql'), + span => expect(span.k).to.equal(constants.EXIT), + span => expect(span.f.e).to.equal(String(controls.getPid())), + span => expect(span.f.h).to.equal('agent-stub-uuid'), + span => expect(span.ec).to.equal(1), + span => expect(span.data.mysql.error).to.exist + ]); + + expect(mysqlSpan.stack).to.be.a('string'); + // expect(mysqlSpan.stack.length).to.be.greaterThan(0); + + // mysqlSpan.stack.forEach(frame => { + // expect(frame).to.have.property('m'); + // expect(frame).to.have.property('c'); + // expect(frame.m).to.be.a('string'); + // expect(frame.c).to.be.a('string'); + // }); + + // const hasRelevantFrame = mysqlSpan.stack.some( + // frame => + // frame.c.includes('app.js') || + // frame.c.includes('mysql') || + // frame.m.includes('query') || + // frame.m.includes('Query') + // ); + // expect(hasRelevantFrame).to.be.true; + }) + ) + )); } diff --git a/packages/collector/test/tracing/databases/pg_native/test.js b/packages/collector/test/tracing/databases/pg_native/test.js index b3cf30b6ae..06805f3990 100644 --- a/packages/collector/test/tracing/databases/pg_native/test.js +++ b/packages/collector/test/tracing/databases/pg_native/test.js @@ -189,6 +189,51 @@ mochaSuiteFn('tracing/pg-native', function () { ); })); + it('must replace span stack with error stack when error occurs', () => + controls + .sendRequest({ + method: 'POST', + path: '/error', + simple: false + }) + .then(response => { + expect(response).to.exist; + expect(response.error).to.contain('Error: ERROR:'); + expect(response.error).to.contain('relation "nonexistanttable" does not exist'); + + return retry(() => + agentControls.getSpans().then(spans => { + const httpEntry = verifyHttpEntry(spans, '/error'); + const pgExit = expectAtLeastOneMatching(spans, span => { + verifyPgExitBase(span, httpEntry, 'SELECT name, email FROM nonexistanttable'); + expect(span.error).to.not.exist; + expect(span.ec).to.equal(1); + }); + + expect(pgExit.stack).to.be.a('string'); + // expect(pgExit.stack.length).to.be.greaterThan(0); + + // pgExit.stack.forEach(frame => { + // expect(frame).to.have.property('m'); + // expect(frame).to.have.property('c'); + // expect(frame.m).to.be.a('string'); + // expect(frame.c).to.be.a('string'); + // }); + + // const hasErrorFrame = pgExit.stack.some( + // frame => + // frame.c.includes('pg-native') || + // frame.c.includes('app.js') || + // frame.m.includes('query') || + // frame.m.includes('Query') + // ); + // expect(hasErrorFrame).to.be.true; + + verifyHttpExit(spans, httpEntry); + }) + ); + })); + it('must suppress', () => controls .sendRequest({ diff --git a/packages/collector/test/tracing/misc/stack_trace/test.js b/packages/collector/test/tracing/misc/stack_trace/test.js index 26ca4db039..d3c9c480f2 100644 --- a/packages/collector/test/tracing/misc/stack_trace/test.js +++ b/packages/collector/test/tracing/misc/stack_trace/test.js @@ -89,6 +89,10 @@ const mochaSuiteFn = supportedVersion(process.versions.node) ? describe : descri await expressProxyControls.stop(); }); + beforeEach(async () => { + await agentControls.clearReceivedTraceData(); + }); + beforeEach(async () => { await agentControls.waitUntilAppIsCompletelyInitialized(expressControls.getPid()); }); @@ -107,7 +111,7 @@ const mochaSuiteFn = supportedVersion(process.versions.node) ? describe : descri .then(() => testUtils.retry(() => agentControls.getSpans().then(spans => { - expect(spans.length).to.equal(6); + expect(spans.length).to.equal(3); testUtils.expectAtLeastOneMatching(spans, [ span => expect(span.n).to.equal('node.http.server'), span => expect(span.stack).to.have.lengthOf(0) @@ -126,10 +130,11 @@ const mochaSuiteFn = supportedVersion(process.versions.node) ? describe : descri .then(() => testUtils.retry(() => agentControls.getSpans().then(spans => { - expect(spans.length).to.equal(9); + expect(spans.length).to.equal(3); testUtils.expectAtLeastOneMatching(spans, [ span => expect(span.n).to.equal('node.http.client'), span => expect(span.k).to.equal(constants.EXIT), + span => expect(span.data.http.status).to.equal(201), span => expect(span.stack[2].m).to.equal('fetch'), span => expect(span.stack[2].c).to.contains('node-fetch') ]); diff --git a/packages/collector/test/tracing/protocols/http/client/clientApp.js b/packages/collector/test/tracing/protocols/http/client/clientApp.js index 8526f9e9fe..0f2a8839eb 100644 --- a/packages/collector/test/tracing/protocols/http/client/clientApp.js +++ b/packages/collector/test/tracing/protocols/http/client/clientApp.js @@ -29,6 +29,7 @@ const baseUrl = `${protocol}://user:password@localhost:${process.env.SERVER_PORT const sslDir = path.join(__dirname, '..', '..', '..', '..', 'apps', 'ssl'); const key = fs.readFileSync(path.join(sslDir, 'key')); const cert = fs.readFileSync(path.join(sslDir, 'cert')); +const { handleErrorResponse } = require('./errorHelper'); const app = express(); @@ -376,6 +377,43 @@ app.get('/without-port', (req, res) => { request.end(); }); +app.get('/trigger-error', (req, res) => { + const options = { + hostname: 'localhost', + port: process.env.SERVER_PORT, + method: 'GET', + path: '/error-endpoint', + ca: cert + }; + + log('Initiating call to error endpoint'); + + const request = httpModule.request(options, response => { + let data = ''; + response.on('data', chunk => { + data += chunk; + }); + response.on('end', () => { + log(`Error endpoint responded with status ${response.statusCode}`); + try { + handleErrorResponse(response.statusCode, data); + res.status(response.statusCode).json({ message: 'Error endpoint called', body: data }); + } catch (error) { + log('Caught error from handleErrorResponse:', error.message); + + res.status(500).json({ error: error.message, stack: error.stack }); + } + }); + }); + + request.on('error', err => { + log('Error in downstream call:', err.message); + res.sendStatus(500); + }); + + request.end(); +}); + function createUrl(req, urlPath) { const pathWithQuery = req.query.withQuery ? `${urlPath}?q1=some&pass=verysecret&q2=value` : urlPath; return req.query.urlObject ? new URL(pathWithQuery, baseUrl) : baseUrl + pathWithQuery; diff --git a/packages/collector/test/tracing/protocols/http/client/errorHelper.js b/packages/collector/test/tracing/protocols/http/client/errorHelper.js new file mode 100644 index 0000000000..a2c8ee2eb4 --- /dev/null +++ b/packages/collector/test/tracing/protocols/http/client/errorHelper.js @@ -0,0 +1,23 @@ +/* + * (c) Copyright IBM Corp. 2025 + */ + +'use strict'; + +/** + * Helper function that throws an error when a 500 status is received. + * This is used to test error stack replacement in HTTP client instrumentation. + */ +function handleErrorResponse(statusCode, responseBody) { + if (statusCode >= 500) { + const error = new Error(`Server error: ${statusCode}`); + error.statusCode = statusCode; + error.responseBody = responseBody; + throw error; + } + return { statusCode, responseBody }; +} + +module.exports = { + handleErrorResponse +}; diff --git a/packages/collector/test/tracing/protocols/http/client/serverApp.js b/packages/collector/test/tracing/protocols/http/client/serverApp.js index df186c9974..9e19ad9dbf 100644 --- a/packages/collector/test/tracing/protocols/http/client/serverApp.js +++ b/packages/collector/test/tracing/protocols/http/client/serverApp.js @@ -67,6 +67,10 @@ app.put('/continue', (req, res) => { res.json({ response: 'yada yada yada' }); }); +app.get('/error-endpoint', (req, res) => { + res.status(500).json({ error: 'Internal Server Error' }); +}); + if (process.env.APP_USES_HTTPS === 'true') { const sslDir = path.join(__dirname, '..', '..', '..', '..', 'apps', 'ssl'); require('https') diff --git a/packages/collector/test/tracing/protocols/http/client/test.js b/packages/collector/test/tracing/protocols/http/client/test.js index 55942d61e9..c1d86d83bd 100644 --- a/packages/collector/test/tracing/protocols/http/client/test.js +++ b/packages/collector/test/tracing/protocols/http/client/test.js @@ -633,7 +633,8 @@ function registerTests(appUsesHttps) { expect(span.data.http.error).to.match(/Invalid URL/); }, span => expect(span.t).to.equal(entrySpan.t), - span => expect(span.p).to.equal(entrySpan.s) + span => expect(span.p).to.equal(entrySpan.s), + span => expect(span.stack).to.be.a('string') ]); expectExactlyOneMatching(spans, span => { expect(span.n).to.equal('node.http.client'); diff --git a/packages/core/src/tracing/instrumentation/databases/mysql.js b/packages/core/src/tracing/instrumentation/databases/mysql.js index afee7877bc..f9aa120ac8 100644 --- a/packages/core/src/tracing/instrumentation/databases/mysql.js +++ b/packages/core/src/tracing/instrumentation/databases/mysql.js @@ -190,7 +190,7 @@ function instrumentedAccessFunction( }) .catch(error => { span.ec = 1; - span.data.mysql.error = tracingUtil.getErrorDetails(error); + tracingUtil.setErrorStack(span, error, exports.spanName); span.d = Date.now() - span.ts; span.transmit(); @@ -205,7 +205,7 @@ function instrumentedAccessFunction( function onResult(error) { if (error) { span.ec = 1; - span.data.mysql.error = tracingUtil.getErrorDetails(error); + tracingUtil.setErrorStack(span, error, exports.spanName); } span.d = Date.now() - span.ts; diff --git a/packages/core/src/tracing/instrumentation/databases/pgNative.js b/packages/core/src/tracing/instrumentation/databases/pgNative.js index ec75bde81e..e102805254 100644 --- a/packages/core/src/tracing/instrumentation/databases/pgNative.js +++ b/packages/core/src/tracing/instrumentation/databases/pgNative.js @@ -197,7 +197,10 @@ function startSpanBeforeSync(ctx, originalFn, originalArgs, statement, stackTrac function finishSpan(error, span) { if (error) { span.ec = 1; - span.data.pg.error = tracingUtil.getErrorDetails(error); + // Note: Instead of 'pg', we could've passed exports.spanName if they were the same, + // We can’t use spanName here because for this instr the span name is + // "postgres", but the data is stored under span.data.pg. + tracingUtil.setErrorStack(span, error, 'pg'); } span.d = Date.now() - span.ts; diff --git a/packages/core/src/tracing/instrumentation/protocols/httpClient.js b/packages/core/src/tracing/instrumentation/protocols/httpClient.js index 831b62133b..9a2b8a4aec 100644 --- a/packages/core/src/tracing/instrumentation/protocols/httpClient.js +++ b/packages/core/src/tracing/instrumentation/protocols/httpClient.js @@ -242,6 +242,14 @@ function instrument(coreModule, forceHttps) { span.d = Date.now() - span.ts; span.ec = res.statusCode >= 500 ? 1 : 0; + + // Internal: Remove/update before RFR + // Currently we don't parse anything from the body, and its not advised too + // so do not override here, only override on the event of an error catch + // if (span.ec === 1) { + // tracingUtil.setErrorStack(span, res.body.stack); + // } + span.transmit(); if (callback) { @@ -268,6 +276,8 @@ function instrument(coreModule, forceHttps) { }; span.d = Date.now() - span.ts; span.ec = 1; + tracingUtil.setErrorStack(span, e, exports.spanName); + span.transmit(); throw e; } @@ -316,6 +326,8 @@ function instrument(coreModule, forceHttps) { }; span.d = Date.now() - span.ts; span.ec = 1; + tracingUtil.setErrorStack(span, err, exports.spanName); + span.transmit(); }); }); @@ -336,10 +348,10 @@ function constructFromUrlOpts(options, self, forceHttps) { try { const agent = options.agent || self.agent; - const isHttps = forceHttps || options.protocol === 'https:' || (agent?.protocol === 'https:'); + const isHttps = forceHttps || options.protocol === 'https:' || agent?.protocol === 'https:'; // Use protocol-aware default port, 443 for HTTPS, 80 for HTTP - const port = options.port || options.defaultPort || (agent?.defaultPort) || (isHttps ? 443 : 80); + const port = options.port || options.defaultPort || agent?.defaultPort || (isHttps ? 443 : 80); const protocol = (port === 443 && 'https:') || diff --git a/packages/core/src/tracing/tracingUtil.js b/packages/core/src/tracing/tracingUtil.js index 9e0768130b..df84ca2845 100644 --- a/packages/core/src/tracing/tracingUtil.js +++ b/packages/core/src/tracing/tracingUtil.js @@ -276,3 +276,26 @@ exports.findCallback = (/** @type {string | any[]} */ originalArgs) => { callbackIndex }; }; + +/** + * @param {import('../core').InstanaBaseSpan} span - The span to update + * @param {Error} error + * @param {string} technology - The technology name (e.g., 'mysql', 'pg', 'http') + */ +// @ts-ignore +exports.setErrorStack = function setErrorStack(span, error, technology) { + if (error == null) { + return undefined; + } + + if (technology && span.data[technology]) { + // for some cases like http, it is already set with custom values and no need to overwrite the message + span.data[technology].error = span.data[technology].error || exports.getErrorDetails(error); + } + + if (error && error.stack) { + // no need to consider length for error cases, we can send the whole stack trace as per design + // TODO: It will be recorded as string, revisit to change structure + span.stack = error.stack; + } +};