From 8e7b7419054c93e10a58f2bb66a67746f3c77b63 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Sat, 28 Oct 2023 12:50:09 -0500 Subject: [PATCH 1/5] feat(rack)! Use Rack Events By Default I have been running the `Rack::Events` based instrumentation in our production workloads since 2023-10-23. There is no noticeable difference in performance, however applications using `Rack::BodyProxy` to stream responses will notice a slight increase in latency since it will include timings writing response output to the socket. --- instrumentation/rack/example/trace_demonstration.rb | 2 +- instrumentation/rack/example/trace_demonstration2.rb | 2 +- instrumentation/rack/example/trace_demonstration3.rb | 2 +- .../lib/opentelemetry/instrumentation/rack/instrumentation.rb | 2 +- .../opentelemetry/instrumentation/rack/instrumentation_test.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/instrumentation/rack/example/trace_demonstration.rb b/instrumentation/rack/example/trace_demonstration.rb index e6253f57c..02fc8a712 100644 --- a/instrumentation/rack/example/trace_demonstration.rb +++ b/instrumentation/rack/example/trace_demonstration.rb @@ -18,7 +18,7 @@ builder = Rack::Builder.app do # integration should be automatic in web frameworks (like rails), # but for a plain Rack application, enable it in your config.ru, e.g., - use OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware + use ::Rack::Events, [OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.new] app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } run app diff --git a/instrumentation/rack/example/trace_demonstration2.rb b/instrumentation/rack/example/trace_demonstration2.rb index c6ac649e9..66941cfb0 100644 --- a/instrumentation/rack/example/trace_demonstration2.rb +++ b/instrumentation/rack/example/trace_demonstration2.rb @@ -24,7 +24,7 @@ end # integrate instrumentation explicitly: -builder.use OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware +builder.use ::Rack::Events, [OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.new] # demonstrate tracing (span output to console): puts Rack::MockRequest.new(builder).get('/') diff --git a/instrumentation/rack/example/trace_demonstration3.rb b/instrumentation/rack/example/trace_demonstration3.rb index a444ecfeb..38226a72a 100644 --- a/instrumentation/rack/example/trace_demonstration3.rb +++ b/instrumentation/rack/example/trace_demonstration3.rb @@ -19,7 +19,7 @@ builder = Rack::Builder.app do # integration should be automatic in web frameworks (like rails), # but for a plain Rack application, enable it in your config.ru, e.g., - use OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware + use ::Rack::Events, [OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.new] app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } run app diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index 35be3a519..4ae265b7e 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -29,7 +29,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :untraced_requests, default: nil, validate: :callable option :response_propagators, default: [], validate: :array # This option is only valid for applicaitons using Rack 2.0 or greater - option :use_rack_events, default: false, validate: :boolean + option :use_rack_events, default: true, validate: :boolean # Temporary Helper for Sinatra and ActionPack middleware to use during installation # diff --git a/instrumentation/rack/test/opentelemetry/instrumentation/rack/instrumentation_test.rb b/instrumentation/rack/test/opentelemetry/instrumentation/rack/instrumentation_test.rb index 3e9529245..2a0cf5d6d 100644 --- a/instrumentation/rack/test/opentelemetry/instrumentation/rack/instrumentation_test.rb +++ b/instrumentation/rack/test/opentelemetry/instrumentation/rack/instrumentation_test.rb @@ -32,7 +32,7 @@ _(instrumentation.config[:url_quantization]).must_be_nil _(instrumentation.config[:untraced_requests]).must_be_nil _(instrumentation.config[:response_propagators]).must_be_empty - _(instrumentation.config[:use_rack_events]).must_equal false + _(instrumentation.config[:use_rack_events]).must_equal true end end From 7e7c7234f1c7714fdcb2d6648e2fbfb5da2715c3 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Sat, 28 Oct 2023 18:13:20 -0500 Subject: [PATCH 2/5] squash: fix invalid HTTP responses in tests --- instrumentation/action_pack/test/test_helpers/app_config.rb | 4 +--- .../test/test_helpers/middlewares/redirect_middleware.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/instrumentation/action_pack/test/test_helpers/app_config.rb b/instrumentation/action_pack/test/test_helpers/app_config.rb index 8874d61e2..f00733ad4 100644 --- a/instrumentation/action_pack/test/test_helpers/app_config.rb +++ b/instrumentation/action_pack/test/test_helpers/app_config.rb @@ -48,9 +48,7 @@ def initialize_app(use_exceptions_app: false, remove_rack_tracer_middleware: fal private def remove_rack_middleware(application) - application.middleware.delete( - OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware - ) + application.middleware.delete(Rack::Events) end def add_exceptions_app(application) diff --git a/instrumentation/action_pack/test/test_helpers/middlewares/redirect_middleware.rb b/instrumentation/action_pack/test/test_helpers/middlewares/redirect_middleware.rb index c531fe3dd..da843c704 100644 --- a/instrumentation/action_pack/test/test_helpers/middlewares/redirect_middleware.rb +++ b/instrumentation/action_pack/test/test_helpers/middlewares/redirect_middleware.rb @@ -10,7 +10,7 @@ def initialize(app, _options = {}) end def call(env) - return [307, {}, 'Temporary Redirect'] if should_redirect?(env) + return [307, { 'Location' => '/ok', 'Content-Type' => 'text/plain' }, ['Temporary Redirect']] if should_redirect?(env) @app.call(env) end From 765009739c0293443de0b3768634a2ab2d125dd3 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Sat, 28 Oct 2023 18:20:10 -0500 Subject: [PATCH 3/5] squash: Remove redundant test --- .../instrumentation/rails/instrumentation_test.rb | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/instrumentation_test.rb diff --git a/instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/instrumentation_test.rb b/instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/instrumentation_test.rb deleted file mode 100644 index bae64a5b6..000000000 --- a/instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/instrumentation_test.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'test_helper' - -describe OpenTelemetry::Instrumentation::Rails::Instrumentation do - it 'adds the rack tracing middleware' do - _(DEFAULT_RAILS_APP.config.middleware).must_include OpenTelemetry::Instrumentation::Rack::Middlewares::TracerMiddleware - end -end From b6ac076db57d4426090de820de1ab6a81662aba6 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Sat, 28 Oct 2023 18:25:53 -0500 Subject: [PATCH 4/5] squash: Update rack error description --- .../grape/test/opentelemetry/instrumentation/grape_test.rb | 4 ++-- .../instrumentation/rack/middlewares/event_handler.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/grape/test/opentelemetry/instrumentation/grape_test.rb b/instrumentation/grape/test/opentelemetry/instrumentation/grape_test.rb index 0e7ab4850..f9d6744cd 100644 --- a/instrumentation/grape/test/opentelemetry/instrumentation/grape_test.rb +++ b/instrumentation/grape/test/opentelemetry/instrumentation/grape_test.rb @@ -272,7 +272,7 @@ class RaisedErrorAPI < Grape::API it 'sets span status to error' do _(span.name).must_equal expected_span_name _(span.status.code).must_equal OpenTelemetry::Trace::Status::ERROR - _(span.status.description).must_equal "Unhandled exception of type: #{expected_error_type}" + _(span.status.description).must_equal expected_error_type end it 'records the exception event' do @@ -316,7 +316,7 @@ class ErrorInFilterAPI < Grape::API it 'sets span status to error' do _(span.name).must_equal expected_span_name _(span.status.code).must_equal OpenTelemetry::Trace::Status::ERROR - _(span.status.description).must_equal "Unhandled exception of type: #{expected_error_type}" + _(span.status.description).must_equal expected_error_type end it 'records the exception event' do diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb index 80096c77f..74bdb9005 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -90,7 +90,7 @@ def on_error(request, _, error) return unless span.recording? span.record_exception(error) - span.status = OpenTelemetry::Trace::Status.error + span.status = OpenTelemetry::Trace::Status.error(error.class.name) rescue StandardError => e OpenTelemetry.handle_error(exception: e) end From aee2d7cac0e9e31527cc1d258402c25c0d276eaf Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Wed, 22 Nov 2023 12:15:07 -0600 Subject: [PATCH 5/5] Update instrumentation.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- .../lib/opentelemetry/instrumentation/rack/instrumentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index 4ae265b7e..91e1f9f33 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -28,7 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :url_quantization, default: nil, validate: :callable option :untraced_requests, default: nil, validate: :callable option :response_propagators, default: [], validate: :array - # This option is only valid for applicaitons using Rack 2.0 or greater + # This option is only valid for applications using Rack 2.0 or greater option :use_rack_events, default: true, validate: :boolean # Temporary Helper for Sinatra and ActionPack middleware to use during installation