Add Feature#on_request hook to fix instrumentation with retries

The instrumentation feature's start/finish spans were mismatched when
using .retriable — start was called once via wrap_request at build time,
but finish was called on every retry via wrap_response. This caused
NoMethodError with ActiveSupport::Notifications on Rails 7+.

Add a new Feature#on_request lifecycle hook called before each request
attempt (including retries) and move instrumentation's span start logic
there, ensuring properly paired start/finish calls.

Closes #826

Author: sferik

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- `Feature#on_request` lifecycle hook, called before each request attempt
+  (including retries), for per-attempt side effects like instrumentation (#826)
 - Pattern matching support (`deconstruct_keys`) for Response, Response::Status,
   Headers, ContentType, and URI (#642)
 - Combined global and per-operation timeouts: global and per-operation timeouts
@@ -18,6 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Instrumentation feature now correctly starts a new span for each retry
+  attempt, fixing `NoMethodError` with `ActiveSupport::Notifications` when
+  using `.retriable` with the instrumentation feature (#826)
 - Raise `HTTP::Request::InvalidURIError` for invalid URIs (nil, empty string,
   missing scheme, malformed) instead of confusing `UnsupportedSchemeError` or
   `Addressable::URI::InvalidURIError` (#565)

lib/http/client.rb

@@ -137,7 +137,6 @@ def close
     # @api private
     def perform_once(req, options)
       verify_connection!(req.uri)
-
       @state = :dirty
 
       send_request(req, options)
@@ -168,16 +167,16 @@ def perform_with_retry(req, options)
     # @return [void]
     # @api private
     def send_request(req, options)
+      notify_features(req, options)
+
       @connection ||= HTTP::Connection.new(req, options)
 
       unless @connection.failed_proxy_connect?
         @connection.send_request(req)
         @connection.read_headers!
       end
     rescue Error => e
-      options.features.each_value do |feature|
-        feature.on_error(req, e)
-      end
+      options.features.each_value { |feature| feature.on_error(req, e) }
       raise
     end
 
@@ -192,6 +191,13 @@ def build_wrapped_response(req, options)
       end
     end
 
+    # Notify features of an upcoming request attempt
+    # @return [void]
+    # @api private
+    def notify_features(req, options)
+      options.features.each_value { |feature| feature.on_request(req) }
+    end
+
     # Wrap request through feature middleware
     # @return [HTTP::Request] the wrapped request
     # @api private

lib/http/feature.rb

@@ -27,6 +27,20 @@ def wrap_response(response)
       response
     end
 
+    # Callback invoked before each request attempt
+    #
+    # Unlike {#wrap_request}, which is called once when the request is built,
+    # this hook is called before every attempt, including retries. Use it for
+    # per-attempt side effects like starting instrumentation spans.
+    #
+    # @example
+    #   feature.on_request(request)
+    #
+    # @param _request [HTTP::Request]
+    # @return [nil]
+    # @api public
+    def on_request(_request); end
+
     # Callback for request errors
     #
     # @example

lib/http/features/instrumentation.rb

@@ -62,20 +62,24 @@ def initialize(instrumenter: NullInstrumenter.new, namespace: "http")
         @error_name = "error.#{namespace}"
       end
 
-      # Wraps a request with instrumentation events
+      # Starts instrumentation for a request attempt
+      #
+      # Called before each request attempt, including retries. Emits a
+      # separate "start" event so a logger can print the request being run
+      # without waiting for a response, then starts the main request span.
       #
       # @example
-      #   feature.wrap_request(request)
+      #   feature.on_request(request)
       #
       # @param request [HTTP::Request]
-      # @return [HTTP::Request]
+      # @return [nil]
       # @api public
-      def wrap_request(request)
+      def on_request(request)
         # Emit a separate "start" event, so a logger can print the request
         # being run without waiting for a response
         instrumenter.instrument("start_#{name}", request: request) {} # rubocop:disable Lint/EmptyBlock
         instrumenter.start(name, request: request)
-        request
+        nil
       end
 
       # Wraps a response with instrumentation events

sig/http.rbs

@@ -85,6 +85,7 @@ module HTTP
 
     def perform_once: (Request req, Options options) -> Response
     def perform_with_retry: (Request req, Options options) -> Response
+    def notify_features: (Request req, Options options) -> void
     def wrap_request: (Request req, Options opts) -> Request
     def build_response: (Request req, Options options) -> Response
     def build_wrapped_response: (Request req, Options options) -> Response
@@ -244,6 +245,7 @@ module HTTP
   class Feature
     def wrap_request: (untyped request) -> untyped
     def wrap_response: (untyped response) -> untyped
+    def on_request: (untyped _request) -> nil
     def on_error: (untyped _request, untyped _error) -> nil
   end
 
@@ -312,7 +314,7 @@ module HTTP
       attr_reader error_name: String
 
       def initialize: (?instrumenter: untyped, ?namespace: String) -> void
-      def wrap_request: (untyped request) -> untyped
+      def on_request: (untyped request) -> nil
       def wrap_response: (untyped response) -> untyped
       def on_error: (untyped request, untyped error) -> untyped
 

test/http/client_test.rb

@@ -419,6 +419,28 @@ def wrap_response(res)
           feature_class_order.order
         )
       end
+
+      it "calls on_request once per attempt" do
+        feature_class_on_request =
+          Class.new(HTTP::Feature) do
+            attr_reader :call_count
+
+            def initialize
+              super
+              @call_count = 0
+            end
+
+            def on_request(_request)
+              @call_count += 1
+            end
+          end
+        feature_instance = feature_class_on_request.new
+
+        client.use(test_feature: feature_instance)
+              .request(:get, dummy.endpoint)
+
+        assert_equal 1, feature_instance.call_count
+      end
     end
   end
 

test/http/features/instrumentation_test.rb

@@ -26,7 +26,7 @@ def finish(_name, payload)
   let(:instrumenter) { instrumenter_class.new }
   let(:feature) { HTTP::Features::Instrumentation.new(instrumenter: instrumenter) }
 
-  describe "logging the request" do
+  describe "starting instrumentation" do
     let(:request) do
       HTTP::Request.new(
         verb:    :post,
@@ -36,8 +36,8 @@ def finish(_name, payload)
       )
     end
 
-    it "logs the request" do
-      feature.wrap_request(request)
+    it "starts the instrumentation span" do
+      feature.on_request(request)
 
       assert_equal({ request: request }, instrumenter.output[:start])
     end
@@ -100,8 +100,8 @@ def finish(_name, _payload = {}); end
       HTTP::Request.new(verb: :get, uri: "https://example.com/", headers: {})
     end
 
-    it "does not raise LocalJumpError in wrap_request" do
-      feature.wrap_request(request)
+    it "does not raise LocalJumpError in on_request" do
+      feature.on_request(request)
     end
 
     it "does not raise LocalJumpError in on_error" do