Merge 5673580 into e27c539

CHANGELOG.md

@@ -1459,7 +1459,7 @@ Yanked
 - Support for arrays, literal value types and function arguments in sql lexer
 
 ## 1.2.2
-- Handle out of range numbers in queue lenght and metrics api
+- Handle out of range numbers in queue length and metrics api
 
 ## 1.2.1
 - Use Dir.pwd in CLI install wizard
@@ -1484,7 +1484,7 @@ Yanked
 ## 1.1.7
 - Make logging resilient for closing FD's (daemons gem does this)
 - Add support for using Resque through ActiveJob
-- Rescue more expections in json generation
+- Rescue more exceptions in json generation
 
 ## 1.1.6
 - Generic Rack instrumentation middleware
@@ -1534,7 +1534,7 @@ Yanked
 - Improved sql sanitization
 - Improved mongoid/mongodb sanitization
 - Minor performance improvements
-- Better handling for non-utf8 convertable strings
+- Better handling for non-utf8 convertible strings
 - Make gem installable (but not functional) on JRuby
 
 ## 1.0.4
@@ -1715,7 +1715,7 @@ Yanked
 Yanked
 
 ## 0.8.8
-- Explicitely require securerandom
+- Explicitly require securerandom
 
 ## 0.8.7
 - Dup process action event to avoid threading issue

Rakefile

@@ -219,7 +219,7 @@ namespace :build do
     Gem::PackageTask.new(base_gemspec, &block)
   rescue StandardError => e
     puts "Warning: An error occurred defining `build:#{task_name}:gem` Rake task."
-    puts "This task will not be availble."
+    puts "This task will not be available."
     if ENV["DEBUG"]
       puts "#{e}: #{e.message}"
       puts e.backtrace

lib/appsignal/auth_check.rb

@@ -40,7 +40,7 @@ def perform
     # @return [Array<String/nil, String>] response tuple.
     #   - First value is the response status code.
     #   - Second value is a description of the response and the exception error
-    #     message if an exception occured.
+    #     message if an exception occurred.
     def perform_with_result
       status = perform
       result =

lib/appsignal/config.rb

@@ -444,7 +444,7 @@ def load_from_disk
             "'APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR=1' in your system " \
             "environment."
         end
-      message = "An error occured while loading the AppSignal config file. " \
+      message = "An error occurred while loading the AppSignal config file. " \
         "#{extra_message}\n" \
         "File: #{config_file.inspect}\n" \
         "#{e.class.name}: #{e}"

lib/appsignal/event_formatter/sequel/sql_formatter.rb

@@ -4,7 +4,7 @@ module Appsignal
   class EventFormatter
     # @api private
     module Sequel
-      # Compatability with the sequel-rails gem.
+      # Compatibility with the sequel-rails gem.
       # The sequel-rails gem adds its own ActiveSupport::Notifications events
       # that conflict with our own sequel instrumentor. Without this event
       # formatter the sequel-rails events are recorded without the SQL query

lib/appsignal/logger.rb

@@ -64,7 +64,7 @@ def add(severity, message = nil, group = nil)
     alias log add
 
     # Log a debug level message
-    # @param message Mesage to log
+    # @param message Message to log
     # @param attributes Attributes to tag the log with
     # @return [void]
     def debug(message = nil, attributes = {})
@@ -77,7 +77,7 @@ def debug(message = nil, attributes = {})
     end
 
     # Log an info level message
-    # @param message Mesage to log
+    # @param message Message to log
     # @param attributes Attributes to tag the log with
     # @return [void]
     def info(message = nil, attributes = {})
@@ -90,7 +90,7 @@ def info(message = nil, attributes = {})
     end
 
     # Log a warn level message
-    # @param message Mesage to log
+    # @param message Message to log
     # @param attributes Attributes to tag the log with
     # @return [void]
     def warn(message = nil, attributes = {})
@@ -103,7 +103,7 @@ def warn(message = nil, attributes = {})
     end
 
     # Log an error level message
-    # @param message Mesage to log
+    # @param message Message to log
     # @param attributes Attributes to tag the log with
     # @return [void]
     def error(message = nil, attributes = {})
@@ -116,7 +116,7 @@ def error(message = nil, attributes = {})
     end
 
     # Log a fatal level message
-    # @param message Mesage to log
+    # @param message Message to log
     # @param attributes Attributes to tag the log with
     # @return [void]
     def fatal(message = nil, attributes = {})

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 peculiarity 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

lib/puma/plugin/appsignal.rb

@@ -18,7 +18,7 @@ def start(launcher)
 
       loop do
         # Implement similar behavior to minutely probes.
-        # Initial sleep to wait until the app is fully initalized.
+        # Initial sleep to wait until the app is fully initialized.
         # Then loop every 60 seconds and collect the Puma stats as AppSignal
         # metrics.
         sleep sleep_time

spec/lib/appsignal/capistrano2_spec.rb

@@ -160,7 +160,7 @@ def stub_marker_request(data = {})
               run
             end
 
-            it "transmits the overriden revision" do
+            it "transmits the overridden revision" do
               expect(output).to include \
                 "Notifying AppSignal of deploy with: revision: abc123, user: batman",
                 "AppSignal has been notified of this deploy!"
@@ -174,7 +174,7 @@ def stub_marker_request(data = {})
               run
             end
 
-            it "transmits the overriden deploy user" do
+            it "transmits the overridden deploy user" do
               expect(output).to include \
                 "Notifying AppSignal of deploy with: revision: 503ce0923ed177a3ce000005," \
                   " user: robin",

spec/lib/appsignal/capistrano3_spec.rb

@@ -172,7 +172,7 @@ def run
               run
             end
 
-            it "transmits the overriden revision" do
+            it "transmits the overridden revision" do
               expect(output).to include \
                 "Notifying AppSignal of deploy with: revision: abc123, user: batman",
                 "AppSignal has been notified of this deploy!"
@@ -186,7 +186,7 @@ def run
               run
             end
 
-            it "transmits the overriden deploy user" do
+            it "transmits the overridden deploy user" do
               expect(output).to include \
                 "Notifying AppSignal of deploy with: revision: 503ce0923ed177a3ce000005, " \
                   "user: robin",

spec/lib/appsignal/cli/diagnose_spec.rb

@@ -466,7 +466,7 @@ def dont_accept_prompt_to_send_diagnostics_report
         end
       end
 
-      context "when the extention returns invalid JSON" do
+      context "when the extension returns invalid JSON" do
         before do
           expect(Appsignal::Extension).to receive(:diagnose).and_return("invalid agent\njson")
           run

spec/lib/appsignal/config_spec.rb

@@ -264,7 +264,7 @@
       end
     end
 
-    context "with an overriden config file" do
+    context "with an overridden config file" do
       let(:config) do
         project_fixture_config("production", {}, Appsignal.internal_logger,
           File.join(project_fixture_path, "config", "appsignal.yml"))
@@ -275,7 +275,7 @@
         expect(config.active?).to be_truthy
       end
 
-      context "with an invalid overriden config file" do
+      context "with an invalid overridden config file" do
         let(:config) do
           project_fixture_config("production", {}, Appsignal.internal_logger,
             File.join(project_fixture_path, "config", "missing.yml"))
@@ -300,7 +300,7 @@
           stdout = std_stream
           stderr = std_stream
           log = capture_logs { capture_std_streams(stdout, stderr) { config } }
-          message = "An error occured while loading the AppSignal config file. " \
+          message = "An error occurred while loading the AppSignal config file. " \
             "Skipping file config. " \
             "In future versions AppSignal will not start on a config file " \
             "error. To opt-in to this new behavior set " \
@@ -326,7 +326,7 @@
           ENV["APPSIGNAL_PUSH_API_KEY"] = "something valid"
           ENV["APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR"] = "1"
           log = capture_logs { capture_std_streams(stdout, stderr) { config } }
-          message = "An error occured while loading the AppSignal config file. " \
+          message = "An error occurred while loading the AppSignal config file. " \
             "Not starting AppSignal because APPSIGNAL_INACTIVE_ON_CONFIG_FILE_ERROR is set.\n" \
             "File: #{File.join(config_path, "config", "appsignal.yml").inspect}\n" \
             "KeyError: key not found"

spec/lib/appsignal/environment_spec.rb

@@ -90,7 +90,7 @@ def report(key, &value_block)
       end
     end
 
-    context "when something unforseen errors" do
+    context "when something unforeseen errors" do
       it "does not re-raise the error and writes it to the log" do
         klass = Class.new do
           def inspect
@@ -127,7 +127,7 @@ def inspect
       expect(rake_spec.version.to_s).to_not be_empty
     end
 
-    context "when something unforseen errors" do
+    context "when something unforeseen errors" do
       it "does not re-raise the error and writes it to the log" do
         expect(Bundler).to receive(:rubygems).and_raise(RuntimeError, "bundler error")
 
@@ -148,7 +148,7 @@ def inspect
       expect_environment_metadata("ruby_a_test_enabled", "true")
     end
 
-    context "when something unforseen errors" do
+    context "when something unforeseen errors" do
       it "does not re-raise the error and writes it to the log" do
         klass = Class.new do
           def to_s