From e520b14970d5dedbeb6d6db1edcb634e203dc255 Mon Sep 17 00:00:00 2001 From: xuan-cao-swi Date: Wed, 30 Aug 2023 17:13:16 -0400 Subject: [PATCH 1/3] fix: catch any exception from action_controller --- .../patches/action_controller/metal.rb | 4 ++ .../patches/action_controller/metal_test.rb | 46 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb index 6e3b17c9e..edb86c620 100644 --- a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb +++ b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb @@ -25,6 +25,10 @@ def dispatch(name, request, response) end super(name, request, response) + rescue Exception => e # rubocop:disable Lint/RescueException + rack_span.record_exception(e) + rack_span.status = OpenTelemetry::Trace::Status.error + raise end private diff --git a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb index fe488d31c..a06e324ef 100644 --- a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb +++ b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb @@ -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 @@ -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 From 26d1dd51f4161f92a5dd82f89bb3fb4b6267844a Mon Sep 17 00:00:00 2001 From: xuan-cao-swi Date: Mon, 11 Sep 2023 15:27:44 -0400 Subject: [PATCH 2/3] avoid misleading assertion on internal_server_error with raise_in_middleware --- .../action_pack/patches/action_controller/metal_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb index a06e324ef..e8d8e462d 100644 --- a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb +++ b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb @@ -142,6 +142,12 @@ _(span.attributes['http.user_agent']).must_be_nil _(span.attributes['code.namespace']).must_equal 'ExceptionsController' _(span.attributes['code.function']).must_equal 'show' + + _(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 end From c549724cd6156b2a101338b71edce3293b712ce2 Mon Sep 17 00:00:00 2001 From: xuan-cao-swi Date: Tue, 12 Sep 2023 16:10:46 -0400 Subject: [PATCH 3/3] avoid set status in action controller (let rack set the span status); add more test case for 4xx error --- .../patches/action_controller/metal.rb | 1 - .../patches/action_controller/metal_test.rb | 72 +++++++++++++++++++ .../controllers/example_controller.rb | 8 +++ .../action_pack/test/test_helpers/routes.rb | 2 + 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb index edb86c620..4383da2f4 100644 --- a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb +++ b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb @@ -27,7 +27,6 @@ def dispatch(name, request, response) super(name, request, response) rescue Exception => e # rubocop:disable Lint/RescueException rack_span.record_exception(e) - rack_span.status = OpenTelemetry::Trace::Status.error raise end diff --git a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb index e8d8e462d..52b83219f 100644 --- a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb +++ b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/patches/action_controller/metal_test.rb @@ -121,6 +121,78 @@ _(span.name).must_equal 'HTTP GET' end + it 'does not set span to error when 404 is raised from controller' do + get '/internal_page_not_found' + + _(span.name).must_equal 'ExampleController#internal_page_not_found' + _(span.kind).must_equal :server + _(span.status.ok?).must_equal true + + _(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_page_not_found' + _(span.attributes['http.status_code']).must_equal 404 + _(span.attributes['http.user_agent']).must_be_nil + _(span.attributes['code.namespace']).must_equal 'ExampleController' + _(span.attributes['code.function']).must_equal 'internal_page_not_found' + + _(span.events.size).must_equal 1 + _(span.events.first.name).must_equal 'exception' + _(span.events.first.attributes['exception.type']).must_equal 'ActionController::RoutingError' + _(span.events.first.attributes['exception.message']).must_equal 'Not Found' + _(span.events.first.attributes['exception.stacktrace'].nil?).must_equal false + end + + it 'does not set span to error when wrong url is requested' do + get '/not_found_url' + + _(span.name).must_equal 'HTTP GET' + _(span.kind).must_equal :server + _(span.status.ok?).must_equal true + + _(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 '/not_found_url' + _(span.attributes['http.status_code']).must_equal 404 + _(span.attributes['http.user_agent']).must_be_nil + + _(span.events).must_be_nil + end + + it 'does not set span to error when 422 is raised from controller' do + get '/internal_invalid_auth' + + _(span.name).must_equal 'ExampleController#internal_invalid_auth' + _(span.kind).must_equal :server + _(span.status.ok?).must_equal true + + _(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_invalid_auth' + _(span.attributes['http.status_code']).must_equal 422 + _(span.attributes['http.user_agent']).must_be_nil + _(span.attributes['code.namespace']).must_equal 'ExampleController' + _(span.attributes['code.function']).must_equal 'internal_invalid_auth' + + _(span.events.size).must_equal 1 + _(span.events.first.name).must_equal 'exception' + _(span.events.first.attributes['exception.type']).must_equal 'ActionController::InvalidAuthenticityToken' + _(span.events.first.attributes['exception.message']).must_equal 'Invalid Authentication' + _(span.events.first.attributes['exception.stacktrace'].nil?).must_equal false + end + describe 'when the application has exceptions_app configured' do let(:rails_app) { AppConfig.initialize_app(use_exceptions_app: true) } diff --git a/instrumentation/action_pack/test/test_helpers/controllers/example_controller.rb b/instrumentation/action_pack/test/test_helpers/controllers/example_controller.rb index 228c3c8c2..093a4d448 100644 --- a/instrumentation/action_pack/test/test_helpers/controllers/example_controller.rb +++ b/instrumentation/action_pack/test/test_helpers/controllers/example_controller.rb @@ -22,4 +22,12 @@ def new_item def internal_server_error raise :internal_server_error end + + def internal_page_not_found + raise ::ActionController::RoutingError.new("Not Found") + end + + def internal_invalid_auth + raise ::ActionController::InvalidAuthenticityToken.new("Invalid Authentication") + end end diff --git a/instrumentation/action_pack/test/test_helpers/routes.rb b/instrumentation/action_pack/test/test_helpers/routes.rb index 07d72708d..fc004d23e 100644 --- a/instrumentation/action_pack/test/test_helpers/routes.rb +++ b/instrumentation/action_pack/test/test_helpers/routes.rb @@ -11,5 +11,7 @@ def draw_routes(rails_app) get '/items/new', to: 'example#new_item' get '/items/:id', to: 'example#item' get '/internal_server_error', to: 'example#internal_server_error' + get '/internal_page_not_found', to: 'example#internal_page_not_found' + get '/internal_invalid_auth', to: 'example#internal_invalid_auth' end end