Merge pull request #3344 from DataDog/tonycthsu/fix-error-status-codes

Fix `error_status_codes` options
DataDog · Jan 8, 2024 · 6ea3035 · 6ea3035
2 parents be1c557 + 4c68809
commit 6ea3035
Show file tree

Hide file tree

Showing 17 changed files with 341 additions and 78 deletions.
diff --git a/lib/datadog/tracing/contrib/excon/configuration/settings.rb b/lib/datadog/tracing/contrib/excon/configuration/settings.rb
@@ -2,6 +2,8 @@
 
 require_relative '../../../span_operation'
 require_relative '../../configuration/settings'
+require_relative '../../status_range_matcher'
+require_relative '../../status_range_env_parser'
 require_relative '../ext'
 
 module Datadog
@@ -39,20 +41,11 @@ class Settings < Contrib::Configuration::Settings
             option :error_status_codes do |o|
               o.env Ext::ENV_ERROR_STATUS_CODES
               o.default 400...600
-              o.env_parser do |value|
-                values = if value.include?(',')
-                           value.split(',')
-                         else
-                           value.split
-                         end
-                values.map! do |v|
-                  v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')
-
-                  v.empty? ? nil : v
-                end
-
-                values.compact!
-                values
+              o.setter do |v|
+                Tracing::Contrib::StatusRangeMatcher.new(v) if v
+              end
+              o.env_parser do |v|
+                Tracing::Contrib::StatusRangeEnvParser.call(v) if v
               end
             end
 

diff --git a/lib/datadog/tracing/contrib/faraday/configuration/settings.rb b/lib/datadog/tracing/contrib/faraday/configuration/settings.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require_relative '../../configuration/settings'
+require_relative '../../status_range_matcher'
+require_relative '../../status_range_env_parser'
 require_relative '../ext'
 
 module Datadog
@@ -42,20 +44,11 @@ class Settings < Contrib::Configuration::Settings
             option :error_status_codes do |o|
               o.env Ext::ENV_ERROR_STATUS_CODES
               o.default 400...600
-              o.env_parser do |value|
-                values = if value.include?(',')
-                           value.split(',')
-                         else
-                           value.split
-                         end
-                values.map! do |v|
-                  v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')
-
-                  v.empty? ? nil : v
-                end
-
-                values.compact!
-                values
+              o.setter do |v|
+                Tracing::Contrib::StatusRangeMatcher.new(v) if v
+              end
+              o.env_parser do |v|
+                Tracing::Contrib::StatusRangeEnvParser.call(v) if v
               end
             end
 

diff --git a/lib/datadog/tracing/contrib/http/configuration/settings.rb b/lib/datadog/tracing/contrib/http/configuration/settings.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require_relative '../../configuration/settings'
+require_relative '../../status_range_matcher'
+require_relative '../../status_range_env_parser'
 require_relative '../ext'
 
 module Datadog
@@ -43,20 +45,11 @@ class Settings < Contrib::Configuration::Settings
             option :error_status_codes do |o|
               o.env Ext::ENV_ERROR_STATUS_CODES
               o.default 400...600
-              o.env_parser do |value|
-                values = if value.include?(',')
-                           value.split(',')
-                         else
-                           value.split
-                         end
-                values.map! do |v|
-                  v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')
-
-                  v.empty? ? nil : v
-                end
-
-                values.compact!
-                values
+              o.setter do |v|
+                Tracing::Contrib::StatusRangeMatcher.new(v) if v
+              end
+              o.env_parser do |v|
+                Tracing::Contrib::StatusRangeEnvParser.call(v) if v
               end
             end
 

diff --git a/lib/datadog/tracing/contrib/httpclient/configuration/settings.rb b/lib/datadog/tracing/contrib/httpclient/configuration/settings.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require_relative '../../configuration/settings'
+require_relative '../../status_range_matcher'
+require_relative '../../status_range_env_parser'
 require_relative '../ext'
 
 module Datadog
@@ -43,20 +45,11 @@ class Settings < Contrib::Configuration::Settings
             option :error_status_codes do |o|
               o.env Ext::ENV_ERROR_STATUS_CODES
               o.default 400...600
-              o.env_parser do |value|
-                values = if value.include?(',')
-                           value.split(',')
-                         else
-                           value.split
-                         end
-                values.map! do |v|
-                  v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')
-
-                  v.empty? ? nil : v
-                end
-
-                values.compact!
-                values
+              o.setter do |v|
+                Tracing::Contrib::StatusRangeMatcher.new(v) if v
+              end
+              o.env_parser do |v|
+                Tracing::Contrib::StatusRangeEnvParser.call(v) if v
               end
             end
 

diff --git a/lib/datadog/tracing/contrib/httprb/configuration/settings.rb b/lib/datadog/tracing/contrib/httprb/configuration/settings.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require_relative '../../configuration/settings'
+require_relative '../../status_range_matcher'
+require_relative '../../status_range_env_parser'
 require_relative '../ext'
 
 module Datadog
@@ -43,20 +45,11 @@ class Settings < Contrib::Configuration::Settings
             option :error_status_codes do |o|
               o.env Ext::ENV_ERROR_STATUS_CODES
               o.default 400...600
-              o.env_parser do |value|
-                values = if value.include?(',')
-                           value.split(',')
-                         else
-                           value.split
-                         end
-                values.map! do |v|
-                  v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')
-
-                  v.empty? ? nil : v
-                end
-
-                values.compact!
-                values
+              o.setter do |v|
+                Tracing::Contrib::StatusRangeMatcher.new(v) if v
+              end
+              o.env_parser do |v|
+                Tracing::Contrib::StatusRangeEnvParser.call(v) if v
               end
             end
 

diff --git a/lib/datadog/tracing/contrib/status_range_env_parser.rb b/lib/datadog/tracing/contrib/status_range_env_parser.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Datadog
+  module Tracing
+    module Contrib
+      # Parsing status range from environment variable
+      class StatusRangeEnvParser
+        class << self
+          def call(value)
+            [].tap do |array|
+              value.split(',').each do |e|
+                next unless e
+
+                v = e.split('-')
+
+                case v.length
+                when 0 then next
+                when 1 then array << Integer(v.first)
+                when 2 then array << (Integer(v.first)..Integer(v.last))
+                else
+                  Datadog.logger.debug(
+                    "Invalid error_status_codes configuration: Unable to parse #{value}, containing #{v}."
+                  )
+                  next
+                end
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/datadog/tracing/contrib/status_range_matcher.rb b/lib/datadog/tracing/contrib/status_range_matcher.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module Datadog
+  module Tracing
+    module Contrib
+      # Useful checking whether the defined range covers status code
+      class StatusRangeMatcher
+        def initialize(ranges)
+          @ranges = Array(ranges)
+        end
+
+        def include?(status)
+          @ranges.any? do |e|
+            case e
+            when Range
+              e.include? status
+            when Integer
+              e == status
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/datadog/tracing/contrib/excon/instrumentation_spec.rb b/spec/datadog/tracing/contrib/excon/instrumentation_spec.rb
@@ -187,6 +187,20 @@
         expect(request_span).not_to have_error
       end
     end
+
+    context 'when configured from env' do
+      around do |example|
+        ClimateControl.modify('DD_TRACE_EXCON_ERROR_STATUS_CODES' => '500-600') do
+          example.run
+        end
+      end
+
+      it do
+        connection.get(path: '/not_found')
+
+        expect(span).not_to have_error
+      end
+    end
   end
 
   context 'when the request times out' do

diff --git a/spec/datadog/tracing/contrib/faraday/middleware_spec.rb b/spec/datadog/tracing/contrib/faraday/middleware_spec.rb
@@ -320,6 +320,20 @@
         expect(span).not_to have_error
       end
     end
+
+    context 'when configured from env' do
+      around do |example|
+        ClimateControl.modify('DD_TRACE_FARADAY_ERROR_STATUS_CODES' => '500-600') do
+          example.run
+        end
+      end
+
+      it do
+        client.get('not_found')
+
+        expect(span).not_to have_error
+      end
+    end
   end
 
   context 'when split by domain' do

diff --git a/spec/datadog/tracing/contrib/http/request_spec.rb b/spec/datadog/tracing/contrib/http/request_spec.rb
@@ -43,7 +43,7 @@
     subject(:response) { client.get(path) }
     before { stub_request(:any, "#{uri}#{path}").to_return(status: status_code, body: '{}') }
 
-    include_examples 'with error status code configuration'
+    include_examples 'with error status code configuration', env: 'DD_TRACE_HTTP_ERROR_STATUS_CODES'
   end
 
   describe '#get' do

diff --git a/spec/datadog/tracing/contrib/http_examples.rb b/spec/datadog/tracing/contrib/http_examples.rb
@@ -1,4 +1,4 @@
-RSpec.shared_examples 'with error status code configuration' do
+RSpec.shared_examples 'with error status code configuration' do |env:|
   before { subject }
 
   context 'with a custom range' do
@@ -22,6 +22,30 @@
       end
     end
 
+    context 'when custom range from env' do
+      around do |example|
+        ClimateControl.modify(env => '500-502') do
+          example.run
+        end
+      end
+
+      context 'with a status code within the range' do
+        let(:status_code) { 501 }
+
+        it 'marks the span as an error' do
+          expect(span).to have_error
+        end
+      end
+
+      context 'with a status code outside of the range' do
+        let(:status_code) { 503 }
+
+        it 'does not mark the span as an error' do
+          expect(span).to_not have_error
+        end
+      end
+    end
+
     context 'with an Array object' do
       let(:status_code) { 500 }
 

diff --git a/spec/datadog/tracing/contrib/httpclient/instrumentation_spec.rb b/spec/datadog/tracing/contrib/httpclient/instrumentation_spec.rb
@@ -319,7 +319,7 @@
       let(:code) { status_code }
       before { response }
 
-      include_examples 'with error status code configuration'
+      include_examples 'with error status code configuration', env: 'DD_TRACE_HTTPCLIENT_ERROR_STATUS_CODES'
     end
 
     it_behaves_like 'instrumented request'

diff --git a/spec/datadog/tracing/contrib/httprb/instrumentation_spec.rb b/spec/datadog/tracing/contrib/httprb/instrumentation_spec.rb
@@ -322,7 +322,7 @@
       let(:code) { status_code }
       before { response }
 
-      include_examples 'with error status code configuration'
+      include_examples 'with error status code configuration', env: 'DD_TRACE_HTTPRB_ERROR_STATUS_CODES'
     end
 
     it_behaves_like 'instrumented request'