diff --git a/.changesets/add-instrumentation-for-streaming-rack-responses.md b/.changesets/add-instrumentation-for-streaming-rack-responses.md new file mode 100644 index 000000000..6308665da --- /dev/null +++ b/.changesets/add-instrumentation-for-streaming-rack-responses.md @@ -0,0 +1,21 @@ +--- +bump: "minor" +type: "add" +--- + +Add instrumentation for all Rack responses, including streaming responses. New `response_body_each.rack`, `response_body_call.rack` and `response_body_to_ary.rack` events will be shown in the event timeline. This will show how long it takes to complete responses, depending on the response implementation. + +This Sinatra route with a streaming response will be better instrumented, for example: + +```ruby +get "/stream" do + stream do |out| + sleep 1 + out << "1" + sleep 1 + out << "2" + sleep 1 + out << "3" + end +end +``` diff --git a/lib/appsignal.rb b/lib/appsignal.rb index eb2e024ee..cc39a4e1e 100644 --- a/lib/appsignal.rb +++ b/lib/appsignal.rb @@ -305,5 +305,6 @@ def collect_environment_metadata require "appsignal/integrations/railtie" if defined?(::Rails) require "appsignal/transaction" require "appsignal/version" +require "appsignal/rack/body_wrapper" require "appsignal/rack/generic_instrumentation" require "appsignal/transmitter" diff --git a/lib/appsignal/rack/body_wrapper.rb b/lib/appsignal/rack/body_wrapper.rb new file mode 100644 index 000000000..f76947304 --- /dev/null +++ b/lib/appsignal/rack/body_wrapper.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +module Appsignal + # @api private + module Rack + class BodyWrapper + def self.wrap(original_body, appsignal_transaction) + # The logic of how Rack treats a response body differs based on which methods + # the body responds to. This means that to support the Rack 3.x spec in full + # we need to return a wrapper which matches the API of the wrapped body as closely + # as possible. Pick the wrapper from the most specific to the least specific. + # See https://github.com/rack/rack/blob/main/SPEC.rdoc#the-body- + # + # What is important is that our Body wrapper responds to the same methods Rack + # (or a webserver) would be checking and calling, and passes through that functionality + # to the original body. This can be done using delegation via i.e. SimpleDelegate + # but we also need "close" to get called correctly so that the Appsignal transaction + # gets completed - which will not happen, for example, when #to_ary gets called + # just on the delegated Rack body. + # + # This comment https://github.com/rails/rails/pull/49627#issuecomment-1769802573 + # is of particular interest to understand why this has to be somewhat complicated. + if original_body.respond_to?(:to_path) + PathableBodyWrapper.new(original_body, appsignal_transaction) + elsif original_body.respond_to?(:to_ary) + ArrayableBodyWrapper.new(original_body, appsignal_transaction) + elsif !original_body.respond_to?(:each) && original_body.respond_to?(:call) + # This body only supports #call, so we must be running a Rack 3 application + # It is possible that a body exposes both `each` and `call` in the hopes of + # being backwards-compatible with both Rack 3.x and Rack 2.x, however + # this is not going to work since the SPEC says that if both are available, + # `each` should be used and `call` should be ignored. + # So for that case we can drop by to our default EnumerableBodyWrapper + CallableBodyWrapper.new(original_body, appsignal_transaction) + else + EnumerableBodyWrapper.new(original_body, appsignal_transaction) + end + end + + def initialize(body, appsignal_transaction) + @body_already_closed = false + @body = body + @transaction = appsignal_transaction + end + + # This must be present in all Rack bodies and will be called by the serving adapter + def close + # The @body_already_closed check is needed so that if `to_ary` + # of the body has already closed itself (as prescribed) we do not + # attempt to close it twice + if !@body_already_closed && @body.respond_to?(:close) + Appsignal.instrument("response_body_close.rack") { @body.close } + end + @body_already_closed = true + rescue Exception => error # rubocop:disable Lint/RescueException + @transaction.set_error(error) + raise error + ensure + complete_transaction! + end + + def complete_transaction! + # We need to call the Transaction class method and not + # @transaction.complete because the transaction is still + # thread-local and it needs to remove itself from the + # thread variables correctly, which does not happen on + # Transaction#complete. + # + # In the future it would be a good idea to ensure + # that the current transaction is the same as @transaction, + # or allow @transaction to complete itself and remove + # itself from Thread.current + Appsignal::Transaction.complete_current! + end + end + + # The standard Rack body wrapper which exposes "each" for iterating + # over the response body. This is supported across all 3 major Rack + # versions. + # + # @api private + class EnumerableBodyWrapper < BodyWrapper + def each(&blk) + # This is a workaround for the Rails bug when there was a bit too much + # eagerness in implementing to_ary, see: + # https://github.com/rails/rails/pull/44953 + # https://github.com/rails/rails/pull/47092 + # https://github.com/rails/rails/pull/49627 + # https://github.com/rails/rails/issues/49588 + # While the Rack SPEC does not mandate `each` to be callable + # in a blockless way it is still a good idea to have it in place. + return enum_for(:each) unless block_given? + + Appsignal.instrument("process_response_body.rack", "Process Rack response body (#each)") do + @body.each(&blk) + end + rescue Exception => error # rubocop:disable Lint/RescueException + @transaction.set_error(error) + raise error + end + end + + # The callable response bodies are a new Rack 3.x feature, and would not work + # with older Rack versions. They must not respond to `each` because + # "If it responds to each, you must call each and not call". This is why + # it inherits from BodyWrapper directly and not from EnumerableBodyWrapper + # + # @api private + class CallableBodyWrapper < BodyWrapper + def call(stream) + # `stream` will be closed by the app we are calling, no need for us + # to close it ourselves + Appsignal.instrument("process_response_body.rack", "Process Rack response body (#call)") do + @body.call(stream) + end + rescue Exception => error # rubocop:disable Lint/RescueException + @transaction.set_error(error) + raise error + end + end + + # "to_ary" takes precedence over "each" and allows the response body + # to be read eagerly. If the body supports that method, it takes precedence + # over "each": + # "Middleware may call to_ary directly on the Body and return a new Body in its place" + # One could "fold" both the to_ary API and the each() API into one Body object, but + # to_ary must also call "close" after it executes - and in the Rails implementation + # this pecularity was not handled properly. + # + # @api private + class ArrayableBodyWrapper < EnumerableBodyWrapper + def to_ary + @body_already_closed = true + Appsignal.instrument( + "process_response_body.rack", + "Process Rack response body (#to_ary)" + ) do + @body.to_ary + end + rescue Exception => error # rubocop:disable Lint/RescueException + @transaction.set_error(error) + raise error + ensure + # We do not call "close" on ourselves as the only action + # we need to complete is completing the transaction. + complete_transaction! + end + end + + # Having "to_path" on a body allows Rack to serve out a static file, or to + # pass that file to the downstream webserver for sending using X-Sendfile + class PathableBodyWrapper < EnumerableBodyWrapper + def to_path + Appsignal.instrument("response_body_to_path.rack") { @body.to_path } + rescue Exception => error # rubocop:disable Lint/RescueException + @transaction.set_error(error) + raise error + end + end + end +end diff --git a/lib/appsignal/rack/generic_instrumentation.rb b/lib/appsignal/rack/generic_instrumentation.rb index 2599566e2..17f99c28e 100644 --- a/lib/appsignal/rack/generic_instrumentation.rb +++ b/lib/appsignal/rack/generic_instrumentation.rb @@ -16,7 +16,9 @@ def call(env) if Appsignal.active? call_with_appsignal_monitoring(env) else - @app.call(env) + nil_transaction = Appsignal::Transaction::NilTransaction.new + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, nil_transaction)] end end @@ -27,19 +29,30 @@ def call_with_appsignal_monitoring(env) Appsignal::Transaction::HTTP_REQUEST, request ) + # We need to complete the transaction if there is an exception inside the `call` + # of the app. If there isn't one and the app returns us a Rack response triplet, we let + # the BodyWrapper complete the transaction when #close gets called on it + # (guaranteed by the webserver) + complete_transaction_without_body = false begin Appsignal.instrument("process_action.generic") do - @app.call(env) + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, transaction)] end rescue Exception => error # rubocop:disable Lint/RescueException transaction.set_error(error) + complete_transaction_without_body = true raise error ensure - transaction.set_action_if_nil(env["appsignal.route"] || "unknown") + default_action = env["appsignal.route"] || env["appsignal.action"] || "unknown" + transaction.set_action_if_nil(default_action) transaction.set_metadata("path", request.path) transaction.set_metadata("method", request.request_method) transaction.set_http_or_background_queue_start - Appsignal::Transaction.complete_current! + + # Transaction gets completed when the body gets read out, except in cases when + # the app failed before returning us the Rack response triplet. + Appsignal::Transaction.complete_current! if complete_transaction_without_body end end end diff --git a/lib/appsignal/rack/rails_instrumentation.rb b/lib/appsignal/rack/rails_instrumentation.rb index 8de088cd1..6fa30c4ee 100644 --- a/lib/appsignal/rack/rails_instrumentation.rb +++ b/lib/appsignal/rack/rails_instrumentation.rb @@ -16,7 +16,9 @@ def call(env) if Appsignal.active? call_with_appsignal_monitoring(env) else - @app.call(env) + nil_transaction = Appsignal::Transaction::NilTransaction.new + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, nil_transaction)] end end @@ -28,10 +30,17 @@ def call_with_appsignal_monitoring(env) request, :params_method => :filtered_parameters ) + # We need to complete the transaction if there is an exception exception inside the `call` + # of the app. If there isn't one and the app returns us a Rack response triplet, we let + # the BodyWrapper complete the transaction when #close gets called on it + # (guaranteed by the webserver) + complete_transaction_without_body = false begin - @app.call(env) + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, transaction)] rescue Exception => error # rubocop:disable Lint/RescueException transaction.set_error(error) + complete_transaction_without_body = true raise error ensure controller = env["action_controller.instance"] @@ -45,7 +54,10 @@ def call_with_appsignal_monitoring(env) rescue => error Appsignal.internal_logger.error("Unable to report HTTP request method: '#{error}'") end - Appsignal::Transaction.complete_current! + + # Transaction gets completed when the body gets read out, except in cases when + # the app failed before returning us the Rack response triplet. + Appsignal::Transaction.complete_current! if complete_transaction_without_body end end diff --git a/lib/appsignal/rack/sinatra_instrumentation.rb b/lib/appsignal/rack/sinatra_instrumentation.rb index ef88c0f7d..436b26c4d 100644 --- a/lib/appsignal/rack/sinatra_instrumentation.rb +++ b/lib/appsignal/rack/sinatra_instrumentation.rb @@ -42,7 +42,9 @@ def call(env) if Appsignal.active? call_with_appsignal_monitoring(env) else - @app.call(env) + nil_transaction = Appsignal::Transaction::NilTransaction.new + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, nil_transaction)] end end @@ -56,12 +58,19 @@ def call_with_appsignal_monitoring(env) request, options ) + # We need to complete the transaction if there is an exception exception inside the `call` + # of the app. If there isn't one and the app returns us a Rack response triplet, we let + # the BodyWrapper complete the transaction when #close gets called on it + # (guaranteed by the webserver) + complete_transaction_without_body = false begin Appsignal.instrument("process_action.sinatra") do - @app.call(env) + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, transaction)] end rescue Exception => error # rubocop:disable Lint/RescueException transaction.set_error(error) + complete_transaction_without_body = true raise error ensure # If raise_error is off versions of Sinatra don't raise errors, but store @@ -73,7 +82,10 @@ def call_with_appsignal_monitoring(env) transaction.set_metadata("path", request.path) transaction.set_metadata("method", request.request_method) transaction.set_http_or_background_queue_start - Appsignal::Transaction.complete_current! + + # Transaction gets completed when the body gets read out, except in cases when + # the app failed before returning us the Rack response triplet. + Appsignal::Transaction.complete_current! if complete_transaction_without_body end end diff --git a/lib/appsignal/rack/streaming_listener.rb b/lib/appsignal/rack/streaming_listener.rb index 3a9da2f3c..a46644f25 100644 --- a/lib/appsignal/rack/streaming_listener.rb +++ b/lib/appsignal/rack/streaming_listener.rb @@ -16,7 +16,9 @@ def call(env) if Appsignal.active? call_with_appsignal_monitoring(env) else - @app.call(env) + nil_transaction = Appsignal::Transaction::NilTransaction.new + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, nil_transaction)] end end @@ -28,46 +30,35 @@ def call_with_appsignal_monitoring(env) request ) + # We need to complete the transaction if there is an exception exception inside the `call` + # of the app. If there isn't one and the app returns us a Rack response triplet, we let + # the BodyWrapper complete the transaction when #close gets called on it + # (guaranteed by the webserver) + complete_transaction_without_body = false + # Instrument a `process_action`, to set params/action name - status, headers, body = + begin Appsignal.instrument("process_action.rack") do - @app.call(env) - rescue Exception => e # rubocop:disable Lint/RescueException - transaction.set_error(e) - raise e - ensure - transaction.set_action_if_nil(env["appsignal.action"]) - transaction.set_metadata("path", request.path) - transaction.set_metadata("method", request.request_method) - transaction.set_http_or_background_queue_start + status, headers, obody = @app.call(env) + [status, headers, Appsignal::Rack::BodyWrapper.wrap(obody, transaction)] end + rescue Exception => error # rubocop:disable Lint/RescueException + transaction.set_error(error) + complete_transaction_without_body = true + raise error + ensure + transaction.set_action_if_nil(env["appsignal.action"]) + transaction.set_metadata("path", request.path) + transaction.set_metadata("method", request.request_method) + transaction.set_http_or_background_queue_start - # Wrap the result body with our StreamWrapper - [status, headers, StreamWrapper.new(body, transaction)] + # Transaction gets completed when the body gets read out, except in cases when + # the app failed before returning us the Rack response triplet. + Appsignal::Transaction.complete_current! if complete_transaction_without_body + end end end end - class StreamWrapper - def initialize(stream, transaction) - @stream = stream - @transaction = transaction - end - - def each(&block) - @stream.each(&block) - rescue Exception => e # rubocop:disable Lint/RescueException - @transaction.set_error(e) - raise e - end - - def close - @stream.close if @stream.respond_to?(:close) - rescue Exception => e # rubocop:disable Lint/RescueException - @transaction.set_error(e) - raise e - ensure - Appsignal::Transaction.complete_current! - end - end + StreamWrapper = Rack::EnumerableBodyWrapper end diff --git a/spec/lib/appsignal/rack/body_wrapper_spec.rb b/spec/lib/appsignal/rack/body_wrapper_spec.rb new file mode 100644 index 000000000..f263ce5bf --- /dev/null +++ b/spec/lib/appsignal/rack/body_wrapper_spec.rb @@ -0,0 +1,220 @@ +describe Appsignal::Rack::BodyWrapper do + let(:nil_txn) { Appsignal::Transaction::NilTransaction.new } + + describe "with a body that supports all possible features" do + it "reduces the supported methods to just each()" do + # which is the safest thing to do, since the body is likely broken + fake_body = double(:each => nil, :call => nil, :to_ary => [], :to_path => "/tmp/foo.bin", + :close => nil) + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).to respond_to(:each) + expect(wrapped).not_to respond_to(:to_ary) + expect(wrapped).not_to respond_to(:call) + expect(wrapped).to respond_to(:close) + end + end + + describe "with a body only supporting each()" do + it "wraps with appropriate class" do + fake_body = double + allow(fake_body).to receive(:each) + + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).to respond_to(:each) + expect(wrapped).not_to respond_to(:to_ary) + expect(wrapped).not_to respond_to(:call) + expect(wrapped).to respond_to(:close) + end + + it "reads out the body in full using each" do + fake_body = double + expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c") + wrapped = described_class.wrap(fake_body, nil_txn) + expect { |b| wrapped.each(&b) }.to yield_successive_args("a", "b", "c") + end + + it "returns an Enumerator if each() gets called without a block" do + fake_body = double + expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c") + + wrapped = described_class.wrap(fake_body, nil_txn) + enum = wrapped.each + expect(enum).to be_kind_of(Enumerator) + expect { |b| enum.each(&b) }.to yield_successive_args("a", "b", "c") + end + + it "sets the exception raised inside each() into the Appsignal transaction" do + fake_body = double + expect(fake_body).to receive(:each).once.and_raise(Exception.new("Oops")) + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + + wrapped = described_class.wrap(fake_body, txn) + expect do + expect { |b| wrapped.each(&b) }.to yield_control + end.to raise_error(/Oops/) + end + + it "closes the body and the transaction when it gets closed" do + fake_body = double + expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c") + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(Appsignal::Transaction).to receive(:complete_current!).once + + wrapped = described_class.wrap(fake_body, txn) + expect { |b| wrapped.each(&b) }.to yield_successive_args("a", "b", "c") + expect { wrapped.close }.not_to raise_error + end + end + + describe "with a body supporting both each() and call" do + it "wraps with the wrapper that conceals call() and exposes each" do + fake_body = double + allow(fake_body).to receive(:each) + allow(fake_body).to receive(:call) + + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).to respond_to(:each) + expect(wrapped).not_to respond_to(:to_ary) + expect(wrapped).not_to respond_to(:call) + expect(wrapped).not_to respond_to(:to_path) + expect(wrapped).to respond_to(:close) + end + end + + describe "with a body supporting both to_ary and each" do + let(:fake_body) { double(:each => nil, :to_ary => []) } + it "wraps with appropriate class" do + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).to respond_to(:each) + expect(wrapped).to respond_to(:to_ary) + expect(wrapped).not_to respond_to(:call) + expect(wrapped).not_to respond_to(:to_path) + expect(wrapped).to respond_to(:close) + end + + it "reads out the body in full using each" do + expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c") + + wrapped = described_class.wrap(fake_body, nil_txn) + expect { |b| wrapped.each(&b) }.to yield_successive_args("a", "b", "c") + end + + it "sets the exception raised inside each() into the Appsignal transaction" do + expect(fake_body).to receive(:each).once.and_raise(Exception.new("Oops")) + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + + wrapped = described_class.wrap(fake_body, txn) + expect do + expect { |b| wrapped.each(&b) }.to yield_control + end.to raise_error(/Oops/) + end + + it "reads out the body in full using to_ary" do + expect(fake_body).to receive(:to_ary).and_return(["one", "two", "three"]) + + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped.to_ary).to eq(["one", "two", "three"]) + end + + it "sends the exception raised inside to_ary() into the Appsignal and closes txn" do + fake_body = double + allow(fake_body).to receive(:each) + expect(fake_body).to receive(:to_ary).once.and_raise(Exception.new("Oops")) + expect(fake_body).not_to receive(:close) # Per spec we expect the body has closed itself + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + expect(Appsignal::Transaction).to receive(:complete_current!).once + + wrapped = described_class.wrap(fake_body, txn) + expect { wrapped.to_ary }.to raise_error(/Oops/) + end + end + + describe "with a body supporting both to_path and each" do + let(:fake_body) { double(:each => nil, :to_path => nil) } + + it "wraps with appropriate class" do + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).to respond_to(:each) + expect(wrapped).not_to respond_to(:to_ary) + expect(wrapped).not_to respond_to(:call) + expect(wrapped).to respond_to(:to_path) + expect(wrapped).to respond_to(:close) + end + + it "reads out the body in full using each()" do + expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c") + + wrapped = described_class.wrap(fake_body, nil_txn) + expect { |b| wrapped.each(&b) }.to yield_successive_args("a", "b", "c") + end + + it "sets the exception raised inside each() into the Appsignal transaction" do + expect(fake_body).to receive(:each).once.and_raise(Exception.new("Oops")) + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + + wrapped = described_class.wrap(fake_body, txn) + expect do + expect { |b| wrapped.each(&b) }.to yield_control + end.to raise_error(/Oops/) + end + + it "sets the exception raised inside to_path() into the Appsignal transaction" do + allow(fake_body).to receive(:to_path).once.and_raise(Exception.new("Oops")) + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + expect(txn).not_to receive(:complete) # gets called by the caller via close() + + wrapped = described_class.wrap(fake_body, txn) + expect { wrapped.to_path }.to raise_error(/Oops/) + end + + it "exposes to_path to the sender" do + allow(fake_body).to receive(:to_path).and_return("/tmp/file.bin") + + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped.to_path).to eq("/tmp/file.bin") + end + end + + describe "with a body only supporting call()" do + let(:fake_body) { double(:call => nil) } + it "wraps with appropriate class" do + wrapped = described_class.wrap(fake_body, nil_txn) + expect(wrapped).not_to respond_to(:each) + expect(wrapped).not_to respond_to(:to_ary) + expect(wrapped).to respond_to(:call) + expect(wrapped).not_to respond_to(:to_path) + expect(wrapped).to respond_to(:close) + end + + it "passes the stream into the call() of the body" do + fake_rack_stream = double("stream") + expect(fake_body).to receive(:call).with(fake_rack_stream) + + wrapped = described_class.wrap(fake_body, nil_txn) + expect { wrapped.call(fake_rack_stream) }.not_to raise_error + end + + it "sets the exception raised inside call() into the Appsignal transaction" do + fake_rack_stream = double + allow(fake_body).to receive(:call).with(fake_rack_stream).and_raise(Exception.new("Oopsie")) + + txn = double("Appsignal transaction", "nil_transaction?" => false) + expect(txn).to receive(:set_error).once.with(instance_of(Exception)) + expect(txn).not_to receive(:complete) # gets called by the caller via close() + wrapped = described_class.wrap(fake_body, txn) + + expect { wrapped.call(fake_rack_stream) }.to raise_error(/Oopsie/) + end + end +end diff --git a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb index 0fa0104b9..27ee99e82 100644 --- a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb @@ -50,7 +50,7 @@ expect(app).to receive(:call).with(env) end - context "with an exception", :error => true do + context "with an exception raised from call()", :error => true do let(:error) { ExampleException } let(:app) do double.tap do |d| @@ -58,8 +58,9 @@ end end - it "records the exception" do + it "records the exception and completes the transaction" do expect_any_instance_of(Appsignal::Transaction).to receive(:set_error).with(error) + expect(Appsignal::Transaction).to receive(:complete_current!) end end diff --git a/spec/lib/appsignal/rack/rails_instrumentation_spec.rb b/spec/lib/appsignal/rack/rails_instrumentation_spec.rb index 481854b11..7937a4782 100644 --- a/spec/lib/appsignal/rack/rails_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/rails_instrumentation_spec.rb @@ -65,7 +65,8 @@ class MockController describe "#call_with_appsignal_monitoring" do def run - middleware.call(env) + _status, _headers, body = middleware.call(env) + body.close # Rack will always call close() on the body end it "calls the wrapped app" do @@ -126,7 +127,8 @@ def run end end - it "records the exception" do + it "records the exception and completes the transaction" do + expect(Appsignal::Transaction).to receive(:complete_current!) expect { run }.to raise_error(error) transaction_hash = last_transaction.to_h diff --git a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb index 279b45a9b..dc0387959 100644 --- a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb @@ -3,7 +3,9 @@ module SinatraRequestHelpers def make_request(env) - middleware.call(env) + _status, _headers, body = middleware.call(env) + # Close the body so that the transaction gets completed + body&.close end def make_request_with_error(env, error) diff --git a/spec/lib/appsignal/rack/streaming_listener_spec.rb b/spec/lib/appsignal/rack/streaming_listener_spec.rb index 4b6d7fa82..e0cd81bf5 100644 --- a/spec/lib/appsignal/rack/streaming_listener_spec.rb +++ b/spec/lib/appsignal/rack/streaming_listener_spec.rb @@ -90,10 +90,11 @@ context "with an exception in the instrumentation call" do let(:error) { ExampleException } - it "should add the exception to the transaction" do + it "should add the exception to the transaction and complete the transaction" do allow(app).to receive(:call).and_raise(error) expect(transaction).to receive(:set_error).with(error) + expect(Appsignal::Transaction).to receive(:complete_current!).and_call_original expect do listener.call_with_appsignal_monitoring(env) @@ -101,64 +102,19 @@ end end - it "should wrap the body in a wrapper" do - expect(Appsignal::StreamWrapper).to receive(:new) - .with("body", transaction) - .and_return(wrapper) - + it "should wrap the response body in a wrapper" do body = listener.call_with_appsignal_monitoring(env)[2] - expect(body).to be_a(Appsignal::StreamWrapper) + expect(body).to be_kind_of(Appsignal::Rack::BodyWrapper) end end end describe Appsignal::StreamWrapper do - let(:stream) { double } - let(:transaction) do - Appsignal::Transaction.create(SecureRandom.uuid, Appsignal::Transaction::HTTP_REQUEST, {}) - end - let(:wrapper) { Appsignal::StreamWrapper.new(stream, transaction) } - - describe "#each" do - it "calls the original stream" do - expect(stream).to receive(:each) - - wrapper.each - end - - context "when #each raises an error" do - let(:error) { ExampleException } - - it "records the exception" do - allow(stream).to receive(:each).and_raise(error) - - expect(transaction).to receive(:set_error).with(error) - - expect { wrapper.send(:each) }.to raise_error(error) - end - end - end - - describe "#close" do - it "closes the original stream and completes the transaction" do - expect(stream).to receive(:close) - expect(Appsignal::Transaction).to receive(:complete_current!) - - wrapper.close - end - - context "when #close raises an error" do - let(:error) { ExampleException } - - it "records the exception and completes the transaction" do - allow(stream).to receive(:close).and_raise(error) - - expect(transaction).to receive(:set_error).with(error) - expect(transaction).to receive(:complete) - - expect { wrapper.send(:close) }.to raise_error(error) - end - end + it ".new returns an EnumerableWrapper" do + fake_body = double(:each => nil) + fake_txn = double + stream_wrapper = Appsignal::StreamWrapper.new(fake_body, fake_txn) + expect(stream_wrapper).to be_kind_of(Appsignal::Rack::EnumerableBodyWrapper) end end