refactor: added error stack replacement logic #2167
Conversation
abhilash-sivan
changed the title
abhilash-sivan
force-pushed
the
feat-stack-strace-filter
branch
from
625de88 to
ecb81b7
Compare
packages/core/src/tracing/instrumentation/databases/pgNative.js
Outdated
Show resolved
Hide resolved
abhilash-sivan
force-pushed
the
feat-stack-strace-filter
branch
from
918ea3d to
7df9769
Compare
abhilash-sivan
force-pushed
the
feat-stack-strace-filter
branch
from
7df9769 to
bff53bc
Compare
| if (err) { | ||
| span.ec = 1; | ||
|
|
||
| // TODO: Special logic of appending error |
There was a problem hiding this comment.
Agree. This should be handled as a separate PR after this merge for nats as an action item of #2181
setErrorDetails needs to handle err being a String - some libs have very bad error handling
| if (err) { | ||
| span.ec = 1; | ||
|
|
||
| // TODO: special logic of appending error |
There was a problem hiding this comment.
Agree. This should be handled as a separate PR after this merge for nats streaming as an action item of #2181
setErrorDetails needs to handle err being a String - some libs have very bad error handling
| // No-op, we do not want to mark cancelled calls as erroneous. | ||
| } else { | ||
| span.ec = 1; | ||
| // TODO: special case of error message |
There was a problem hiding this comment.
Agree. Another action item after this PR for grpcJS.
I don't know where err.details is coming from (not an official attr), but we can extend setErrorDetails.
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
| span => | ||
| options.error | ||
| ? expect(span.data.couchbase.error).to.equal(options.error) | ||
| ? expect(span.data.couchbase.error).to.contain(options.error) |
There was a problem hiding this comment.
FYI
A few instrumentations hardcoded
data.redis.error = err.message
instead of using our utility to get the error message
span.data.redis.error = tracingUtil.getErrorDetails(error);
Now all instrumentations use the same syntax to set the data error property.
Thats why we have to change a few assertions from "equal" to "contain", because it now contains the error name by default.
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
packages/core/src/tracing/instrumentation/protocols/httpClient.js
Outdated
Show resolved
Hide resolved
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
refs https://jsw.ibm.com/browse/INSTA-63749
Why
By doing this refactor now, we avoid duplicating stack-handling logic across different instrumentations (DB, messaging, protocol, etc.). This makes future changes easy, more consistent, and more maintainable.
What
This PR is the first step toward implementing the “Error only stack trace filtering" feature. It lays the foundation by centralising the logic around how we set span stack traces, specifically:
span.stackandspan.data.technology.errorinto a common utility on error cases.Making it easier to apply future filtering or removal of stacks based on configuration or environment variables.
Tasks detailed:
Extracts a common function to handle span stack-trace setting and custom replacements.
Adds logic to overwrite the span’s stack with span.technology.error.stack when relevant.
Leaves common fn open for later conditional filtering or removal based on config / environment variables.
Refactors existing code to call this new function where needed (partial — instrumentation update is not yet complete).
Task list: