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.

Sign up for GitHub

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

fix: catch any exception from action_controller #638

Closed
wants to merge 4 commits into from
Closed

fix: catch any exception from action_controller #638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e520b14
fix: catch any exception from action_controller
xuan-cao-swi Aug 30, 2023
26d1dd5
avoid misleading assertion on internal_server_error with raise_in_mid…
xuan-cao-swi Sep 11, 2023
c549724
avoid set status in action controller (let rack set the span status);…
xuan-cao-swi Sep 12, 2023
002fe4d
Merge branch 'open-telemetry:main' into rescue_from_action_controller
xuan-cao-swi Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...ion_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def dispatch(name, request, response)
end

super(name, request, response)
rescue Exception => e # rubocop:disable Lint/RescueException
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the best place to intercept errors and record errors?

Do the errors that are raised here all translate to HTTP 5xx errors?

Is it possible that HTTP 4xx responses are also triggered using errors? If so then this should not result in setting the span status to an error. https://opentelemetry.io/docs/specs/otel/trace/semantic_conventions/http/#status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @arielvalentin , thanks for the feedback.

Is this the best place to intercept errors and record errors?

If we don't change any code structure or how ActionPack::Instrumentation instruments ActionController, I think add rescue with dispatch function would be a good approach. Otherwise, we can try to prepend different method like process_action. Or just create subscriber to subscribe the log event like some vendor does (e.g. sentry)

For current implementation, dispatch is calling process, which I think will be the last call sequence of the action controller.

Since instrumentation prepend dispatch and with super, so it's probably fine to rescue from there. For example, like show_exceptions.rb, it also try to rescue from error of @app.call(env).

And like the comment mentioned, the mode of rails app running will determine if they want to show the exception on web page or raise it internally, but I think it will not affect what dispatch function behave beause the error had already occured.

For rescue_from, then yes, it seems like if user use rescue_from, then the error won't be intercepted, but I think user would expect the error, and they can decide what to do with expected error?

Do the errors that are raised here all translate to HTTP 5xx errors?

I did some tests against 4xx error, and I think there are two methods to raise 404 with rails

Following methods are direct return 404 without any internal exception/error

# 403 forbidden
class UsersController < ApplicationController
  def sample_action
    head: forbidden
    @action.do_action
  end
end
# Same as `head: too_many_requests` and other 4xx error

# 404 by render 404
class UsersController < ApplicationController
  def sample_action
    @action.do_action
    render :status => 404
  end
end

# also get 404 not found by requesting wrong url e.g. 0.0.0.0:8002/whatever_is_wrong_url

Above method won't create error span, nor record exception on span.

This method cause 4XX error with direct raise exception

def sample_action
  @action.do_action
  raise ActionController::RoutingError.new('Not Found')
end

Since it raised the exception, the error will be intercepted, above method will create error span and record exception on span. By looking at this code, I think when 404 occurs, since it will not get into dispatch function, then it won't make span as error span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @arielvalentin , after re-read the rack instrumentation, it seems like rack instrumentation will decide the rack_span's status based on status_code, so I removed setting status code on action_pack instrumentation.

Copy link
Contributor

@robertlaurin robertlaurin Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a potential alternate to this PR, still exploring the limitations and merits of this. But if an app is configured with an exceptions app, the exception is stored on the request env for re-use which seems like an appropriate thing for us to attach to a span if detected.
https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/667/files

rack_span.record_exception(e)
rack_span.status = OpenTelemetry::Trace::Status.error
raise
end

private
Expand Down
46 changes: 46 additions & 0 deletions ...ck/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,44 @@
get 'internal_server_error'

_(span.name).must_equal 'ExampleController#internal_server_error'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal false

_(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
_(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION

_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.host']).must_equal 'example.org'
_(span.attributes['http.scheme']).must_equal 'http'
_(span.attributes['http.target']).must_equal '/internal_server_error'
_(span.attributes['http.status_code']).must_equal 500
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['code.namespace']).must_equal 'ExampleController'
_(span.attributes['code.function']).must_equal 'internal_server_error'

_(span.events.size).must_equal 1
_(span.events.first.name).must_equal 'exception'
_(span.events.first.attributes['exception.type']).must_equal 'TypeError'
_(span.events.first.attributes['exception.message']).must_equal 'exception class/object expected'
_(span.events.first.attributes['exception.stacktrace'].nil?).must_equal false
end

it 'does not set the span name when an exception is raised in middleware' do
get '/ok?raise_in_middleware'

_(span.name).must_equal 'HTTP GET'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal false

_(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
_(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION

_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.host']).must_equal 'example.org'
_(span.attributes['http.scheme']).must_equal 'http'
_(span.attributes['http.target']).must_equal '/ok?raise_in_middleware'
_(span.attributes['http.status_code']).must_equal 500
_(span.attributes['http.user_agent']).must_be_nil
end

it 'does not set the span name when the request is redirected in middleware' do
Expand All @@ -96,6 +128,20 @@
get 'internal_server_error'

_(span.name).must_equal 'ExampleController#internal_server_error'
_(span.kind).must_equal :server
_(span.status.ok?).must_equal false

_(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
_(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION

_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.host']).must_equal 'example.org'
_(span.attributes['http.scheme']).must_equal 'http'
_(span.attributes['http.target']).must_equal '/internal_server_error'
_(span.attributes['http.status_code']).must_equal 500
_(span.attributes['http.user_agent']).must_be_nil
_(span.attributes['code.namespace']).must_equal 'ExceptionsController'
_(span.attributes['code.function']).must_equal 'show'
end
end

Expand Down