Skip to content

Commit

Permalink
Don't instrument response bodies twice
Browse files Browse the repository at this point in the history
If multiple rack instrumentation middleware are present in the
middleware stack, don't instrument the response body multiple times.

Once it detects the response body is already a BodyWrapper subclass,
pass it through without change.
  • Loading branch information
tombruijn committed Jul 8, 2024
1 parent 57d6fa3 commit bbb957e
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 4 deletions.
10 changes: 8 additions & 2 deletions lib/appsignal/rack/abstract_middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,14 @@ def instrument_app_call(env, transaction)

def call_app(env, transaction)
status, headers, obody = @app.call(env)
# Instrument response body and closing of the response body
[status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, transaction)]
body =
if obody.is_a? Appsignal::Rack::BodyWrapper
obody
else
# Instrument response body and closing of the response body
Appsignal::Rack::BodyWrapper.wrap(obody, transaction)
end
[status, headers, body]
end

# Instrument the request fully. This is used by the top instrumentation
Expand Down
33 changes: 32 additions & 1 deletion spec/lib/appsignal/rack/abstract_middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,10 @@ def filtered_params
end

context "with parent instrumentation" do
let(:transaction) { http_request_transaction }
before do
env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = http_request_transaction
env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = transaction
set_current_transaction(transaction)
end

it "uses the existing transaction" do
Expand All @@ -341,6 +343,35 @@ def filtered_params
expect { make_request }.to_not(change { created_transactions.count })
end

it "wraps the response body in a BodyWrapper subclass" do
_status, _headers, body = make_request
expect(body).to be_kind_of(Appsignal::Rack::BodyWrapper)

body.to_ary
response_events =
last_transaction.to_h["events"].count do |event|
event["name"] == "process_response_body.rack"
end
expect(response_events).to eq(1)
end

context "when response body is already a BodyWrapper subclass" do
let(:body) { Appsignal::Rack::BodyWrapper.wrap(["hello!"], transaction) }
let(:app) { DummyApp.new { [200, {}, body] } }

it "doesn't wrap the body again" do
_status, _headers, body = make_request
expect(body).to eq(body)

body.to_ary
response_events =
last_transaction.to_h["events"].count do |event|
event["name"] == "process_response_body.rack"
end
expect(response_events).to eq(1)
end
end

context "with error" do
let(:app) { lambda { |_env| raise ExampleException, "error message" } }

Expand Down
2 changes: 1 addition & 1 deletion spec/support/mocks/dummy_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def call(env)
if @app
@app&.call(env)
else
[200, {}, "body"]
[200, {}, ["body"]]
end
ensure
@called = true
Expand Down

0 comments on commit bbb957e

Please sign in to comment.