instrumentation support

- Add check for valid instrumentation config
- Pass instrumentation and logger explicitly into middleware
- Wrap instrumentation calls in begin-rescue
- Remove required activesupport dependency
- Add fake for testing instrumentation
- Maintain single-retry behavior

Author: Thomascountz

Gemfile.lock

@@ -2,7 +2,6 @@ PATH
   remote: .
   specs:
     zendesk_api (3.1.1)
-      activesupport
       base64
       faraday (> 2.0.0)
       faraday-multipart

lib/zendesk_api/client.rb

@@ -97,6 +97,7 @@ def initialize
       @resource_cache = {}
 
       check_url
+      check_instrumentation
 
       config.retry = !!config.retry # nil -> false
 
@@ -167,7 +168,7 @@ def build_connection
       Faraday.new(config.options) do |builder|
         # response
         builder.use ZendeskAPI::Middleware::Response::RaiseError
-        builder.use ZendeskAPI::Middleware::Response::ZendeskRequestEvent, self if config.instrumentation.respond_to?(:instrument)
+        builder.use ZendeskAPI::Middleware::Response::ZendeskRequestEvent, instrumentation: config.instrumentation, logger: config.logger if config.instrumentation
         builder.use ZendeskAPI::Middleware::Response::Callback, self
         builder.use ZendeskAPI::Middleware::Response::Logger, config.logger if config.logger
         builder.use ZendeskAPI::Middleware::Response::ParseIsoDates
@@ -183,7 +184,7 @@ def build_connection
         set_authentication(builder, config)
 
         if config.cache
-          builder.use ZendeskAPI::Middleware::Request::EtagCache, {cache: config.cache, instrumentation: config.instrumentation}
+          builder.use ZendeskAPI::Middleware::Request::EtagCache, cache: config.cache, instrumentation: config.instrumentation
         end
 
         builder.use ZendeskAPI::Middleware::Request::Upload
@@ -220,6 +221,14 @@ def check_url
       end
     end
 
+    def check_instrumentation
+      return unless config.instrumentation
+
+      unless config.instrumentation.respond_to?(:instrument)
+        raise ArgumentError, "instrumentation must respond to #instrument"
+      end
+    end
+
     def set_raise_error_when_rated_limited
       config.raise_error_when_rate_limited = if config.retry
         false

lib/zendesk_api/configuration.rb

@@ -54,7 +54,7 @@ class Configuration
     # specify if you want a (network layer) exception to elicit a retry
     attr_accessor :retry_on_exception
 
-    # specify if you wnat instrumentation to be used
+    # specify if you want instrumentation to be used
     attr_accessor :instrumentation
 
     def initialize

lib/zendesk_api/middleware/request/etag_cache.rb

@@ -1,5 +1,4 @@
 require "faraday/middleware"
-require "active_support/notifications"
 
 module ZendeskAPI
   module Middleware
@@ -43,18 +42,30 @@ def call(environment)
                 content_length: cached[:response_headers][:content_length],
                 content_encoding: cached[:response_headers][:content_encoding]
               )
-              @instrumentation&.instrument("zendesk.cache_hit",
-                {
-                  endpoint: env[:url].path,
-                  status: env[:status]
-                })
+              if @instrumentation
+                begin
+                  @instrumentation.instrument("zendesk.cache_hit",
+                    {
+                      endpoint: env[:url]&.path,
+                      status: env[:status]
+                    })
+                rescue
+                  # Swallow instrumentation errors to maintain cache behavior
+                end
+              end
             elsif env[:status] == 200 && env[:response_headers]["Etag"] # modified and cacheable
               @cache.write(cache_key(env), env.to_hash)
-              @instrumentation&.instrument("zendesk.cache_miss",
-                {
-                  endpoint: env[:url].path,
-                  status: env[:status]
-                })
+              if @instrumentation
+                begin
+                  @instrumentation.instrument("zendesk.cache_miss",
+                    {
+                      endpoint: env[:url]&.path,
+                      status: env[:status]
+                    })
+                rescue
+                  # Swallow instrumentation errors to maintain cache behavior
+                end
+              end
             end
           end
         end

lib/zendesk_api/middleware/request/retry.rb

@@ -21,7 +21,7 @@ def initialize(app, options = {})
         def call(env)
           # Duplicate env for retries but keep attempt counter persistent
           original_env = env.dup
-          original_env[:call_attempt] ||= 0
+          original_env[:call_attempt] = (env[:call_attempt] || 0)
 
           exception_happened = false
           response = nil
@@ -47,8 +47,8 @@ def call(env)
             return @app.call(original_env)
           end
 
-          # Loop through consecutive retryable responses (e.g., multiple 429s)
-          while response && @error_codes.include?(response.env[:status])
+          # Retry once if response has a retryable error code
+          if response && @error_codes.include?(response.env[:status])
             original_env[:call_attempt] += 1
             seconds_left = (response.env[:response_headers][:retry_after] || DEFAULT_RETRY_AFTER).to_i
             @logger&.warn "You may have been rate limited. Retrying in #{seconds_left} seconds..."
@@ -64,21 +64,26 @@ def call(env)
 
         def instrument_retry(env, reason, delay)
           return unless @instrumentation
-          @instrumentation.instrument(
-            "zendesk.retry",
-            {
-              attempt: env[:call_attempt],
-              endpoint: env[:url].path,
-              method: env[:method],
-              reason: reason,
-              delay: delay
-            }
-          )
+
+          begin
+            @instrumentation.instrument(
+              "zendesk.retry",
+              {
+                attempt: env[:call_attempt],
+                endpoint: env[:url]&.path,
+                method: env[:method],
+                reason: reason,
+                delay: delay
+              }
+            )
+          rescue => e
+            @logger&.debug("zendesk.retry instrumentation failed: #{e.message}")
+          end
         end
 
         def sleep_with_logging(seconds_left)
           seconds_left.times do |i|
-            sleep 1 if seconds_left > 0
+            sleep 1
             time_left = seconds_left - i
             @logger&.warn "#{time_left}..." if time_left > 0 && time_left % 5 == 0
           end

lib/zendesk_api/middleware/response/zendesk_request_event.rb

@@ -5,32 +5,50 @@ module Middleware
     module Response
       # @private
       class ZendeskRequestEvent < Faraday::Middleware
-        def initialize(app, client)
+        def initialize(app, options = {})
           super(app)
-          @client = client
+          @instrumentation = options[:instrumentation]
+          @logger = options[:logger]
         end
 
         def call(env)
           start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
           @app.call(env).on_complete do |response_env|
             end_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
             duration = (end_time - start_time) * 1000.0
-            instrumentation = @client.config.instrumentation
-            if instrumentation
-              instrumentation.instrument("zendesk.request",
-                {duration: duration,
-                 endpoint: response_env[:url].path,
-                 method: response_env[:method],
-                 status: response_env[:status]})
-              if response_env[:status] < 500
-                instrumentation.instrument("zendesk.rate_limit",
-                  {
-                    endpoint: response_env[:url].path,
-                    status: response_env[:status],
-                    threshold: response_env[:response_headers] ? response_env[:response_headers]["X-Rate-Limit-Remaining"] : nil,
-                    limit: response_env[:response_headers] ? response_env[:response_headers]["X-Rate-Limit"] : nil,
-                    reset: response_env[:response_headers] ? response_env[:response_headers]["X-Rate-Limit-Reset"] : nil
-                  })
+            if @instrumentation
+              begin
+                @instrumentation.instrument("zendesk.request",
+                  {duration: duration,
+                   endpoint: response_env[:url]&.path,
+                   method: response_env[:method],
+                   status: response_env[:status]})
+              rescue => e
+                @logger&.debug("zendesk.request instrumentation failed: #{e.message}")
+              end
+
+              if response_env[:status] && response_env[:status] < 500
+                headers = response_env[:response_headers]
+                if headers
+                  remaining = headers["X-Rate-Limit-Remaining"]
+                  limit = headers["X-Rate-Limit"]
+                  reset = headers["X-Rate-Limit-Reset"]
+
+                  if remaining || limit || reset
+                    begin
+                      @instrumentation.instrument("zendesk.rate_limit",
+                        {
+                          endpoint: response_env[:url]&.path,
+                          status: response_env[:status],
+                          remaining: remaining,
+                          limit: limit,
+                          reset: reset
+                        })
+                    rescue => e
+                      @logger&.debug("zendesk.rate_limit instrumentation failed: #{e.message}")
+                    end
+                  end
+                end
               end
             end
           end

spec/core/middleware/request/etag_cache_spec.rb

@@ -1,5 +1,4 @@
 require "core/spec_helper"
-require "active_support/cache"
 
 describe ZendeskAPI::Middleware::Request::EtagCache do
   it "caches" do
@@ -21,60 +20,61 @@
   end
 
   context "instrumentation" do
-    let(:instrumentation) { double("Instrumentation") }
-    let(:cache) { ActiveSupport::Cache::MemoryStore.new }
-    let(:status) { nil }
+    let(:instrumenter) { TestInstrumenter.new }
+    let(:cache) { ZendeskAPI::LRUCache.new(5) }
     let(:middleware) do
       ZendeskAPI::Middleware::Request::EtagCache.new(
         ->(env) { Faraday::Response.new(env) },
         cache: cache,
-        instrumentation: instrumentation
+        instrumentation: instrumenter
       )
     end
     let(:env) do
       {
         url: URI("https://example.zendesk.com/api/v2/blergh"),
         method: :get,
         request_headers: {},
-        response_headers: {"Etag" => "x", :x_rate_limit_remaining => 10},
-        status: status,
+        response_headers: {"Etag" => "x"},
+        status: nil,
         body: {"x" => 1},
         response_body: {"x" => 1}
       }
     end
-    let(:no_instrumentation_middleware) do
-      ZendeskAPI::Middleware::Request::EtagCache.new(
-        ->(env) { Faraday::Response.new(env) },
-        cache: cache,
-        instrumentation: nil
-      )
-    end
-    before do
-      allow(instrumentation).to receive(:instrument)
-    end
 
-    it "emits cache_miss on first request" do
-      expect(instrumentation).to receive(:instrument).with(
-        "zendesk.cache_miss",
-        hash_including(endpoint: "/api/v2/blergh", status: 200)
-      )
+    it "instruments cache miss on first request" do
       env[:status] = 200
-      middleware.call(env).on_complete { |_e| 1 }
-    end
+      middleware.call(env).on_complete { |_e| }
 
-    it "don't care on no instrumentation" do
-      env[:status] = 200
-      no_instrumentation_middleware.call(env).on_complete { |_e| 1 }
+      cache_events = instrumenter.find_events("zendesk.cache_miss")
+      expect(cache_events.size).to eq(1)
+
+      event = cache_events.first[:payload]
+      expect(event[:endpoint]).to eq("/api/v2/blergh")
+      expect(event[:status]).to eq(200)
     end
 
-    it "emits cache_hit on 304 response" do
+    it "instruments cache hit on 304 response" do
       cache.write(middleware.cache_key(env), env)
-      expect(instrumentation).to receive(:instrument).with(
-        "zendesk.cache_hit",
-        hash_including(endpoint: "/api/v2/blergh", status: 304)
-      )
       env[:status] = 304
-      middleware.call(env).on_complete { |_e| 1 }
+      middleware.call(env).on_complete { |_e| }
+
+      cache_events = instrumenter.find_events("zendesk.cache_hit")
+      expect(cache_events.size).to eq(1)
+
+      event = cache_events.first[:payload]
+      expect(event[:endpoint]).to eq("/api/v2/blergh")
+      expect(event[:status]).to eq(304)
+    end
+
+    it "does not crash when instrumentation is nil" do
+      no_instrumentation_middleware = ZendeskAPI::Middleware::Request::EtagCache.new(
+        ->(env) { Faraday::Response.new(env) },
+        cache: cache,
+        instrumentation: nil
+      )
+
+      env[:status] = 200
+      expect { no_instrumentation_middleware.call(env).on_complete { |_e| } }.not_to raise_error
     end
   end
 end