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

Trace exceptions through Rack middleware #110

Closed
wants to merge 3 commits into from
Closed

Trace exceptions through Rack middleware #110

wants to merge 3 commits into from

Conversation

ferdynton
Copy link
Contributor

We're currently running the latest version of the gem in our production Rails 4.2.8 application and in all of our errors in DataDog's UI we see the same stacktrace:

/app/vendor/bundle/ruby/2.2.0/gems/ddtrace-0.6.1/lib/ddtrace/contrib/rails/action_controller.rb:19:in `block in instrument'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/fanout.rb:127:in `call'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/fanout.rb:127:in `finish'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/fanout.rb:46:in `block in finish'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/fanout.rb:46:in `each'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/fanout.rb:46:in `finish'
/app/vendor/bundle/ruby/2.2.0/gems/activesupport-4.2.8/lib/active_support/notifications/instrumenter.rb:36:in `finish'

This is not the stacktrace of any of the errors thrown, but simply the callstack of the code that's reporting the error. The reason is because the code is using caller to populate the stacktrace and caller doesn't have much to do with exceptions. Even when we manually set the error from somewhere in code, that line overrides it with the same callstack, over and over.

Examining the code a little closer it looks like it's actually hard to get the right stacktrace from the ActiveSupport's instrumentation. Looking at how do other gems get this information (Honeybadger, NewRelic, Sentry), it looks to us like using a custom middleware layer is the way to go.

For us this also means we'll be able to trace errors thrown from our API layer, where we use Grape; Grape reuses the same Rack middleware stack, but does not go through ActiveController so we currently don't have a simple way of tracing those errors.

In order to make this change, we've changed some of the tests from being ActionController::TestCase to be ActionDispatch::IntegrationTest. The latter are said to be slower, but the former don't go through the middleware stack so the tests were failing.

Let us know your thoughts on this. Thanks!

Even though Integration tests are said to be 25% slower, they run
through Rake's middleware stack, which we might leverage in order to
improve the error capturing capabilities.
ActiveSupport's instrumentation doesn't provide a lot of information
about an error that might have been raised, so introducing a piece of
middleware seems to be the best way of getting full control. Other
similar gems also adopt this approach.
Serves a couple purposes:
* It work with other Rack-based solutions, such as Grape.
* It captures the stacktrace, which the current implementation does not.

This also fixes a test that claimed to grab a stacktrace but no actual
exception was ever thrown, only the calltrace was attached.
@palazzem
Copy link
Contributor

Ouch...we were working exactly on this task and this is definitely the way to go. I'm also working to add the Grape instrumentation so that it will be easier for you to auto instrument your code. Because we've already in place another PR that handles that (#111), I'll be sure to merge later this PR!

Thanks a lot for this work and for reporting this issue. It will be part of the next release!

@palazzem palazzem added the integrations Involves tracing integrations label Apr 13, 2017
@palazzem palazzem added this to the 0.7.0 milestone Apr 13, 2017
@palazzem palazzem self-assigned this Apr 13, 2017
@palazzem palazzem self-requested a review April 13, 2017 23:14
@palazzem
Copy link
Contributor

Hello @Ferdy89 ! Thank you for this PR. I've basically cherry picked your commits and did some changes, mostly related to tests and to make the Rack middleware available for generic usage. The followed approach is exactly that one with some minor changes. Here the PRs that are part of this improvement:

I'm going to ship a new release now! Because you told me that you're using Grape, I've also merged this PR that adds Grape instrumentation #117! Feel free to leave any feedback because we're sure that all these changes have still room of improvement!

Thank you again!

@palazzem palazzem closed this Apr 24, 2017
@ferdynton ferdynton deleted the trace_through_middleware branch April 24, 2017 15:38
@palazzem
Copy link
Contributor

Hey @Ferdy89 saw your comments! Unfortunately all PRs have already been merged and released in the new 0.7.0, but I want to reassure you that we're keeping track of them!

Thanks again and feel free to submit proposals or anything that can improve this client library!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants