Improved logging mechanism for operations on ended spans, making it easier to trace issues in both Node.js and Browser environments #5113 #5184
Conversation
…ded spans, making it easier to trace issues in both Node.js and Browser environments Reviewer: pichlermarc Refs: open-telemetry#5113
…ded spans, making it easier to trace issues in both Node.js and Browser environments Reviewer: pichlermarc Refs: open-telemetry#5113
|
👋@pichlermarc this good? |
| ); | ||
| // Adding a debug log to capture the stack trace | ||
| diag.debug(`Stack trace for ended span operation: ${new Error().stack}`); |
There was a problem hiding this comment.
Using just the stack trace string leads to dev tools not being able to apply custom formatting it does to error object - most notably the ability to jump to source code, which becomes inconsistent
// Test 1
console.log(`stack test - inline - ${new Error().stack}`);
// Test 2
console.log('stack test - arg2', new Error().stack);
// Test 3
console.log('stack test - error obj', new Error());In chrome, the experience is okay as chrome auto-linkifys links
... as long as you don't expect to use sourcemaps - you just get lead into the built file instead of the source
(realistically at otel bundle size, yeah I'll take sourcemaps thanks)
while firefox doesn't touch non-error objects
But it's also an issue when debugging node over --inspect, chrome tries it's best to find some links but only the Error object version is correct:
node --inspect-brk .\logger_test.js
(at this point it's also worth considering if it's better to change issue requirements to just change into .warn(message, Error) as diag api allows 2nd+ argument to be unknown)
|
@Victorsesan thanks for working on this. I just read through @t2t2's comment here and I think he makes a good point.
I initially formulated the requirements in that way so that people would not think it's an actual unhandled Error and open bug issues everywhere. But the more I think about it, it actually should be addressed in instrumentation code that does this because it can lead to lost telemetry. @Victorsesan would you mind changing your PR to put the error directly as the second argument in the |
…ded spans, making it easier to trace issues in both Node.js and Browser environments Reviewer: pichlermarc Refs: open-telemetry#5113
There was a problem hiding this comment.
Thanks for updating this - I think we can drop the debug log now since we just pass it to warn. There will be no extra info that end-user derive from enabling debug.
Once that's done and tests are updated we can merge this - sorry again for changing the requirements on you after you've already done the work. 😞
| // Optionally, you can still log the stack trace for additional context | ||
| diag.debug(`Stack trace for ended span operation: ${error.stack}`); |
There was a problem hiding this comment.
I think this one now does not provide extra value since the error is passed to the warn, we can safely remove it :)
| diag.warn( | ||
| `Cannot execute the operation on ended Span {traceId: ${this._spanContext.traceId}, spanId: ${this._spanContext.spanId}}. Change log level to debug for stack trace.`, | ||
| error // Pass the error object as the second argument | ||
| ); |
There was a problem hiding this comment.
Since we remove the additional debug log, we can also remove the message telling people to enable it.
| diag.warn( | |
| `Cannot execute the operation on ended Span {traceId: ${this._spanContext.traceId}, spanId: ${this._spanContext.spanId}}. Change log level to debug for stack trace.`, | |
| error // Pass the error object as the second argument | |
| ); | |
| diag.warn( | |
| `Cannot execute the operation on ended Span {traceId: ${this._spanContext.traceId}, spanId: ${this._spanContext.spanId}}.`, | |
| error // Pass the error object as the second argument | |
| ); |
No description provided.