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

Add exception_controller option to Rails #320

Merged
merged 3 commits into from
Jan 24, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Jan 19, 2018

If a Rails app has a custom error controller defined via config.exceptions_app= for the purpose of rendering errors, the name of this custom error controller and action will supersede the name of the controller that originally raised the exception for the trace. This is because the errors controller runs afterwards, and replaces the name on the parent span, no matter what. Ultimately, it results in all error traces being listed under "ErrorsController#internal_server_error" or similarly named resource, which isn't a great experience.

This pull request introduces the exception_class option to Rails configuration, which allows a user to specify a class or module to identify a custom exception controller with. Spans for any controller that matches this definition will not override the parent span's resource name.

If you do not specify this option, the tracer will "guess" if the controller is an error controller, by looking for the presence and value of an action_dispatch.exception header, which is typically provided to error controllers in Rails. Most users should see improved behavior without any additional configuration.

In practice:

Datadog.configure do |c|
  # Implicitly guess at exception controllers
  c.use :rails
end

OR

Datadog.configure do |c|
  # Explicitly defining an exception controller
  c.use :rails, exception_controller: ErrorsController
end

And a resulting trace with improved resource name:

screen shot 2018-01-19 at 1 21 21 pm

@delner delner added enhancement integrations Involves tracing integrations do-not-merge/WIP Not ready for merge labels Jan 19, 2018
@delner delner added this to the 0.11.1 milestone Jan 19, 2018
@delner delner self-assigned this Jan 19, 2018
@delner delner force-pushed the delner/rails_error_redirection branch from e3896e0 to bc97825 Compare January 19, 2018 18:56
@palazzem palazzem removed the request for review from p-lambert January 19, 2018 19:06
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

@delner can you also update the documentation here? https://github.com/DataDog/dd-trace-rb/blob/bc978254a6c6e1e66b48d5ca64c236693f108fa9/docs/GettingStarted.md#ruby-on-rails

I would say to not give details about the false case.

@delner delner force-pushed the delner/rails_error_redirection branch 3 times, most recently from f8c856c to 42d54e4 Compare January 22, 2018 19:08
@delner delner removed the do-not-merge/WIP Not ready for merge label Jan 22, 2018
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

one question and a nitpick, otherwise it looks good to me!

controller <= exception_controller_class
# Else if no exception controller class has been set,
# guess whether this is an exception controller from the headers.
elsif exception_controller_class.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

it's just "cosmetic", but this is the base case because most of developers don't set exception_controller, so would be great having it as a first if condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I can rearrange this.

@@ -150,8 +150,13 @@ def process_action_with_datadog(*args)
# signals; it propagates the request span so that it can be finished
# no matter what
payload = {
controller: self.class.name,
controller: self.class,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you provide more details about this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was only passing a string of the class name before. We need the actual constant now, so we can actually do type checking.

Copy link
Contributor

Choose a reason for hiding this comment

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

changing that doesn't have any side effect for the rest of the code, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it was being used to tag the name on the span. Since "#{class}" is the same thing as class.name, it has no change.

@delner delner force-pushed the delner/rails_error_redirection branch from 42d54e4 to c675606 Compare January 24, 2018 17:49
@delner
Copy link
Contributor Author

delner commented Jan 24, 2018

Feedback addressed @palazzem !

Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Good to me! thank you very much!

@delner delner merged commit f6e83ca into master Jan 24, 2018
@delner delner deleted the delner/rails_error_redirection branch February 9, 2018 18:51
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