From 562f6490653fc862e68a3c05efd4fc574d694e20 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Sat, 23 Dec 2023 10:21:43 -0600 Subject: [PATCH] feat(rack)! Use Rack Events By Default (#709) * 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. * squash: fix invalid HTTP responses in tests * squash: Remove redundant test * squash: Update rack error description * Update instrumentation.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --------- Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- .../action_pack/test/test_helpers/app_config.rb | 4 +--- .../test_helpers/middlewares/redirect_middleware.rb | 2 +- .../opentelemetry/instrumentation/grape_test.rb | 4 ++-- instrumentation/rack/example/trace_demonstration.rb | 2 +- .../rack/example/trace_demonstration2.rb | 2 +- .../rack/example/trace_demonstration3.rb | 2 +- .../instrumentation/rack/instrumentation.rb | 4 ++-- .../rack/middlewares/event_handler.rb | 2 +- .../instrumentation/rack/instrumentation_test.rb | 2 +- .../instrumentation/rails/instrumentation_test.rb | 13 ------------- 10 files changed, 11 insertions(+), 26 deletions(-) delete mode 100644 instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/instrumentation_test.rb 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 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/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..91e1f9f33 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -28,8 +28,8 @@ 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 - option :use_rack_events, default: false, validate: :boolean + # 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 # 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 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 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