Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Rack/Rails span error setting behavior #1162

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

ericmustin
Copy link
Contributor

This PR addresses #1155 , when a user modifies or handles a Rails Controller exception in a rescue_from block. Since adding #1124, the instrumentation has been setting any rails errors upstream on the rack span. However, in cases where these exceptions get swallowed, or re-raised with a different exception, in a rescue_from, this causes the rack span to contain stale information, since we haven't been accounting for rescue_from in our current rails instrumentation.

By moving the rails => rack span upstream error propagation from the action_controller instrumentation, and into the datadog rails exception_middleware, we ensure we're modifying the rack span as late as possible before the exception information is swallowed by rails internal exception handling. This means that we are not going to incorrectly set a handled exception on the rack span.

Long term, adding more explicit instrumentation of the rescue_from code blocks (perhaps by patching ActionController::Rescue.process_action) and Controller before/after hooks would also cover the above use case, but it's a little unclear what that would look like (are they child spans of the controller? part of the controller? child spans of rack? etc) and how to safely test and handle all the edge cases that come with that in a way we could release reliably. For now, the priority of this PR is to ensure the rack spans are not receiving stale error information.

@ericmustin ericmustin requested a review from a team September 1, 2020 09:02
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ericmustin ericmustin merged commit b9341ca into master Sep 2, 2020
@marcotc marcotc deleted the add_rescue_from_handling branch September 2, 2020 18:39
@marcotc marcotc added this to the 0.40.0 milestone Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants