Uh oh!
There was an error while loading. Please reload this page.
ref(server-utils): Set error attributes on span and simplify error info extraction - #21822
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b802211. Configure here.
| const isObject = !!error && typeof error === 'object'; | ||
| const raw = isObject ? ('message' in error ? error.message : undefined) : error; | ||
| const message = raw ? String(raw) : 'unknown_error'; |
There was a problem hiding this comment.
Empty Error message misclassified
Low Severity
getErrorInfo uses raw ? String(raw) : 'unknown_error', so when an Error has an empty message (e.g. new Error()), the span status and sentry.status.message become unknown_error instead of the empty string from .message, unlike the previous getErrorMessage logic and unlike nearby MySQL tracing-channel handling.
Reviewed by Cursor Bugbot for commit b802211. Configure here.
| message, | ||
| attributes: { | ||
| [ERROR_TYPE]: type, | ||
| [SENTRY_STATUS_MESSAGE]: message, |
There was a problem hiding this comment.
FYI: #21811 will automatically add this attribute for anything passed to the span status message. I have no objections to setting it here implicitly though. Probably good practise to do so.
I guess the theoretical edge case here is if someone (for whatever reason) calls span.setStatus with a message after this logic ran, the logic in #21811 won't apply because the SENTRY_STATUS_MESSAGE attribute was already set. So this would omit the new status message. I'd say this is acceptable though, especially because I can't come up with a concrete use case when this would happen.
There was a problem hiding this comment.
I think doing it globally in your PR is better, as close to the end as possible, especially since it is a sentry attribute.
I will remove it from here!
…butes on span Collapse the two branches of the thrown/rejected value handler into a single pass that derives the status message and `error.type` once, and set `error.type` / `sentry.status.message` attributes on the span alongside the error status. Add assertions covering Error instances, thrown primitives, bare error-like objects, and falsy throws.
b802211 to
eaec339Compare| const isObject = !!error && typeof error === 'object'; | ||
| const raw = isObject ? ('message' in error ? error.message : undefined) : error; | ||
| const message = raw ? String(raw) : 'unknown_error'; |
There was a problem hiding this comment.
Bug: The falsy check in getErrorInfo incorrectly maps an Error with an empty message to 'unknown_error' instead of an empty string.
Severity: LOW
Suggested Fix
Replace the ternary operator's falsy check with a nullish coalescing operator (??) to correctly handle empty strings, which are falsy. For example: const message = raw ?? 'unknown_error';. This ensures that an empty string message is preserved.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/tracing-channel.ts#L204
Potential issue: The `getErrorInfo` function uses a falsy check (`raw ? ...`) to
determine the error message. Since an empty string (`''`) is a falsy value in
JavaScript, if an error is thrown with an empty message (e.g., `new Error('')`), its
message will be incorrectly reported as `'unknown_error'`. This is a regression from the
previous behavior which would have correctly preserved the empty string. While creating
errors with empty messages is uncommon, it is a valid edge case that could be triggered
by third-party libraries or dynamic error creation, leading to less accurate tracing
information.
Did we get this right? 👍 / 👎 to inform future reviews.
When a traced operation throws or rejects, we now set
error.typeandsentry.status.messageattributes on the span alongside the existing error status, so the error type lines up with the rest of our span conventions.We talked about this yday, generally tracing channels allow us to observe more errors than before in some cases but these errors are not necessarily "unhandled", but we can still set the error attributes so our users can understand more about the op failure even if handled.