fix(active-record): avoid error spans for handled RecordInvalid#2087
fix(active-record): avoid error spans for handled RecordInvalid#2087aviralgarg05 wants to merge 4 commits into
Conversation
dmathieu
left a comment
There was a problem hiding this comment.
What about other errors that usually don't raise either, such as RecordNotFound?
Should the list of exceptions be configurable?
Applied these as well |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
arielvalentin
left a comment
There was a problem hiding this comment.
Thank you for this contribution and patience. We are low on reviewing capacity recently so its taking me a while to get to changes like yours for a review.
I think that the way Rails allows flow and control through exception handling is a bit at odds with the guidance in the spec which recommends we should record an error if it is unhandled and set the span status to Error.
https://opentelemetry.io/docs/specs/otel/trace/exceptions/#recording-an-exception
Adding allow lists to a library in this way and then interrupting the flow control and re-raising seems like something we would want to centralized in the in_span helper or these methods should inline the in_span helper and only record unexpected exceptions.
I think we can also avoid the unnecessary comparisons in the handle_exception method if we explicitly handled them in rescue blocks.
Summary
This PR fixes issue #1459 by preventing low-signal error spans for app-handled ActiveRecord bang persistence exceptions.
What changed
Why this is in scope
Tests