Merge pull request #1528 from appsignal/http-rb-session-instrumentation

Instrument HTTP.rb chained requests on version 6

Author: unflxw

.changesets/instrument-http-rb-chained-requests.md

@@ -0,0 +1,9 @@
+---
+bump: patch
+type: fix
+---
+
+Instrument HTTP.rb chained requests on http 6. Requests made through a chained
+client -- `HTTP.follow.get(...)`, `HTTP.headers(...).get(...)`, and so on -- go
+through `HTTP::Session` rather than `HTTP::Client`, and were not being recorded.
+They now produce a `request.http_rb` event like any other request.

lib/appsignal/hooks/http.rb

@@ -16,13 +16,22 @@ def dependencies_present?
 
       def install
         require "appsignal/integrations/http"
+        # `Client#request` takes positional options in http5 and keyword options
+        # in http6.
         integration =
           if self.class.http6_or_higher?
             Appsignal::Integrations::HttpIntegration::KeywordOptions
           else
             Appsignal::Integrations::HttpIntegration::HashOptions
           end
         HTTP::Client.prepend integration
+        # In http6 a chained request (`.follow`, `.headers`, ...) goes through
+        # `HTTP::Session#request` instead of `HTTP::Client#request`, so
+        # instrument it too (keyword options). http5 has no Session; chained
+        # requests run through `Client#request` there.
+        if defined?(HTTP::Session)
+          HTTP::Session.prepend Appsignal::Integrations::HttpIntegration::KeywordOptions
+        end
 
         Appsignal::Environment.report_enabled("http_rb")
       end

lib/appsignal/integrations/http.rb

@@ -12,6 +12,15 @@ def self.instrument(verb, uri, &block)
         Appsignal.instrument("request.http_rb", "#{verb.upcase} #{request_uri}", &block)
       end
 
+      # The event is recorded at the request boundary, so a redirected request
+      # stays a single `request.http_rb` event spanning every hop. That boundary
+      # lives in more than one place: a bare request runs through
+      # `HTTP::Client#request`, but in http6 a chained request (`.follow`,
+      # `.headers`, `.timeout`, ...) runs through `HTTP::Session#request`
+      # instead, which never touches `Client#request`. The hook prepends one of
+      # these onto each. `Client#request` takes positional options in http5 and
+      # keyword options in http6; `Session#request` (http6 only) takes keyword
+      # options.
       module HashOptions
         def request(verb, uri, opts = {})
           HttpIntegration.instrument(verb, uri) { super }

spec/lib/appsignal/hooks/http_spec.rb

@@ -17,6 +17,11 @@
           expect(HTTP::Client.included_modules)
             .to include(Appsignal::Integrations::HttpIntegration::KeywordOptions)
         end
+
+        it "instruments chained requests through HTTP::Session" do
+          expect(HTTP::Session.included_modules)
+            .to include(Appsignal::Integrations::HttpIntegration::KeywordOptions)
+        end
       else
         it "installs the HTTP plugin with hash options" do
           expect(HTTP::Client.included_modules)

spec/lib/appsignal/integrations/http_spec.rb

@@ -67,6 +67,26 @@
       end
     end
 
+    describe "following redirects" do
+      # `HTTP.follow` chains through `HTTP::Session#request` in http6, which is
+      # instrumented separately from `HTTP::Client#request`. The event is
+      # recorded at the request boundary, so a redirected request is a single
+      # `request.http_rb` event spanning every hop.
+      it "records a single event spanning every hop" do
+        stub_request(:get, "http://www.google.com")
+          .to_return(:status => 301, :headers => { "Location" => "http://www.example.com" })
+        stub_request(:get, "http://www.example.com").to_return(:status => 200)
+
+        HTTP.follow.get("http://www.google.com")
+
+        events = transaction.to_h["events"]
+          .select { |event| event["name"] == "request.http_rb" }
+        expect(events.map { |event| event["title"] }).to eq(
+          ["GET http://www.google.com"]
+        )
+      end
+    end
+
     context "with various URI objects" do
       it "parses an object responding to #to_s" do
         request_uri = Struct.new(:uri) do