clearer profiling error #4647
clearer profiling error #4647
Conversation
Overall package sizeSelf size: 7.08 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-09-11 14:10:28 Comparing candidate commit a66044b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4647 +/- ##
=======================================
Coverage 69.19% 69.19%
=======================================
Files 1 1
Lines 198 198
Branches 33 33
=======================================
Hits 137 137
Misses 61 61 ☔ View full report in Codecov by Sentry. |
|
Unit tests probably need to be updated |
|
clearer |
| @@ -199,7 +199,7 @@ class AgentExporter { | |||
| this._logger.error(`Error from the agent: ${err.message}`) | |||
| return | |||
| } else if (err) { | |||
| reject(new Error('Profiler agent export back-off period expired')) | |||
There was a problem hiding this comment.
Isn't there like a convention to combine/chain errors? Something like this:
const error = new Error('Profiler agent export back-off period expired')
error.reason = err
reject(error)There was a problem hiding this comment.
Or even this:
reject(new Error('Profiler agent export back-off period expired: ' + err.message))
// Profiler agent export back-off period expired: HTTP Error 500Though that might ruin the cardinality or error messages and undermine the problem you're trying to solve...
There was a problem hiding this comment.
There's error.cause, but I'm not sure it's supported across our full version range.
There was a problem hiding this comment.
If we don't use the custom syntax:
new Error('parent', { cause: new Error('child') })But instead build it manually:
const parent = new Error('parent')
const child = new Error('child')
parent.cause = childThen the Node.js version being used only matters if we care how console.log outputs the error - which I guess we don't. I assume we just need to make sure that the log.error function knows to also take into account any linked cause error.
This is something we should make sure regardless of this PR as other libraries might make use of the cause property and we need to properly capture that information.
Gotcha: There is a slight difference between how console.log outputs an error created with the two approaches listed above. If using the options object with the cause property that's only supported in Node.js 16.9+, then the stack trace outputted looks like this:
Error: parent
at REPL53:1:5
at ContextifyScript.runInThisContext (node:vm:136:12)
... 7 lines matching cause stack trace ...
at [_line] [as _line] (node:internal/readline/interface:887:18) {
[cause]: Error: child
at REPL53:1:34
at ContextifyScript.runInThisContext (node:vm:136:12)
at REPLServer.defaultEval (node:repl:598:22)
at bound (node:domain:432:15)
at REPLServer.runBound [as eval] (node:domain:443:12)
at REPLServer.onLine (node:repl:927:10)
at REPLServer.emit (node:events:532:35)
at REPLServer.emit (node:domain:488:12)
at [_onLine] [as _onLine] (node:internal/readline/interface:416:12)
at [_line] [as _line] (node:internal/readline/interface:887:18)
}
If we just naively add a cause property on an error object after creation-time, the console.log output looks like this:
Error: parent
at REPL31:1:5
at ContextifyScript.runInThisContext (node:vm:136:12)
... 7 lines matching cause stack trace ...
at [_line] [as _line] (node:internal/readline/interface:887:18) {
cause: Error: child
at REPL32:1:5
at ContextifyScript.runInThisContext (node:vm:136:12)
at REPLServer.defaultEval (node:repl:598:22)
at bound (node:domain:432:15)
at REPLServer.runBound [as eval] (node:domain:443:12)
at REPLServer.onLine (node:repl:927:10)
at REPLServer.emit (node:events:532:35)
at REPLServer.emit (node:domain:488:12)
at [_onLine] [as _onLine] (node:internal/readline/interface:416:12)
at [_line] [as _line] (node:internal/readline/interface:887:18)
}
Notice the tiny difference between [cause] and cause. I'm not sure if that's important, but it means that there's a slight difference in the implementation.
There was a problem hiding this comment.
If we sub-class our errors we could have it version detect and do super(message, options) where supported and do super(message); this.cause = options.cause where not supported.
There was a problem hiding this comment.
I think it's moot in this case because I don't think the wrapper/parent error adds any value here.
Indicating that a backoff has expired isn't enough information to diagnose what's causing the backoff in the first place, so rather than creating a new error object, this change just passes the last error along.
bengl
force-pushed
the
bengl/clearer-profiling-error
branch
from
51c2680 to
a66044b
Compare
Indicating that a backoff has expired isn't enough information to diagnose what's causing the backoff in the first place, so rather than creating a new error object, this change just passes the last error along.