Skip to content

Commit

Permalink
feat(rack)! Use Rack Events By Default (#709)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
arielvalentin and kaylareopelle authored Dec 23, 2023
1 parent d3fc541 commit 562f649
Show file tree
Hide file tree
Showing 10 changed files with 11 additions and 26 deletions.
4 changes: 1 addition & 3 deletions instrumentation/action_pack/test/test_helpers/app_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/rack/example/trace_demonstration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/rack/example/trace_demonstration2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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('/')
2 changes: 1 addition & 1 deletion instrumentation/rack/example/trace_demonstration3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

This file was deleted.

0 comments on commit 562f649

Please sign in to comment.