From 2e535c4a38448c1595fec5579961ff205558361e Mon Sep 17 00:00:00 2001 From: Tony Hsu Date: Wed, 20 Dec 2023 16:52:59 +0100 Subject: [PATCH 1/3] Fix `error_status_codes` read from env --- .../contrib/excon/configuration/settings.rb | 21 +++---- .../contrib/faraday/configuration/settings.rb | 21 +++---- .../contrib/http/configuration/settings.rb | 21 +++---- .../httpclient/configuration/settings.rb | 21 +++---- .../contrib/httprb/configuration/settings.rb | 21 +++---- .../contrib/status_range_env_parser.rb | 33 +++++++++++ .../tracing/contrib/status_range_matcher.rb | 25 ++++++++ .../contrib/excon/instrumentation_spec.rb | 14 +++++ .../contrib/faraday/middleware_spec.rb | 14 +++++ .../tracing/contrib/http/request_spec.rb | 2 +- spec/datadog/tracing/contrib/http_examples.rb | 26 ++++++++- .../httpclient/instrumentation_spec.rb | 2 +- .../contrib/httprb/instrumentation_spec.rb | 2 +- .../contrib/shared_settings_examples.rb | 24 ++++++-- .../contrib/status_code_matcher_spec.rb | 40 +++++++++++++ .../contrib/status_range_env_parser_spec.rb | 18 ++++++ .../contrib/status_range_matcher_spec.rb | 58 +++++++++++++++++++ 17 files changed, 285 insertions(+), 78 deletions(-) create mode 100644 lib/datadog/tracing/contrib/status_range_env_parser.rb create mode 100644 lib/datadog/tracing/contrib/status_range_matcher.rb create mode 100644 spec/datadog/tracing/contrib/status_code_matcher_spec.rb create mode 100644 spec/datadog/tracing/contrib/status_range_env_parser_spec.rb create mode 100644 spec/datadog/tracing/contrib/status_range_matcher_spec.rb diff --git a/lib/datadog/tracing/contrib/excon/configuration/settings.rb b/lib/datadog/tracing/contrib/excon/configuration/settings.rb index ff348796561..e7947a07b95 100644 --- 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 index b601cc5445a..9fdf7c833d4 100644 --- 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 index 5f1ed125741..8c2753c8870 100644 --- 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 index a9ee53dced1..c0c051d1e3b 100644 --- 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 index 8207414b028..d0c764a9c29 100644 --- 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 new file mode 100644 index 00000000000..09734673f1e --- /dev/null +++ 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 new file mode 100644 index 00000000000..94bafbba27d --- /dev/null +++ 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 index 671e9197587..c5ec10e1c61 100644 --- 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 index af612ae50e4..92a6d301da6 100644 --- 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 index 5c86d7683d4..d3eb8febdd3 100644 --- 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 index ea469d0354b..d3bd3a40baf 100644 --- 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 index f14a03e7f80..a4775c7290e 100644 --- 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 index 24247345568..40329a51f4c 100644 --- 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' diff --git a/spec/datadog/tracing/contrib/shared_settings_examples.rb b/spec/datadog/tracing/contrib/shared_settings_examples.rb index 41a424d872e..dec0a8ef8ee 100644 --- a/spec/datadog/tracing/contrib/shared_settings_examples.rb +++ b/spec/datadog/tracing/contrib/shared_settings_examples.rb @@ -32,7 +32,11 @@ context 'default without settings' do subject { described_class.new } - it { expect(subject.error_status_codes).to eq(400...600) } + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } end context 'when configured with environment variable' do @@ -45,7 +49,11 @@ end end - it { expect(subject.error_status_codes).to eq(['500']) } + it { expect(subject.error_status_codes).not_to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).not_to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } end context 'when given a comma separated list' do @@ -55,7 +63,11 @@ end end - it { expect(subject.error_status_codes).to eq(['400', '500']) } + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).not_to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } end context 'when given a comma separated list with space' do @@ -65,7 +77,11 @@ end end - it { expect(subject.error_status_codes).to eq(['400', '500']) } + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).not_to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } end end end diff --git a/spec/datadog/tracing/contrib/status_code_matcher_spec.rb b/spec/datadog/tracing/contrib/status_code_matcher_spec.rb new file mode 100644 index 00000000000..7508c0c3870 --- /dev/null +++ b/spec/datadog/tracing/contrib/status_code_matcher_spec.rb @@ -0,0 +1,40 @@ +require 'datadog/tracing/contrib/status_code_matcher' + +RSpec.describe Datadog::Tracing::Contrib::StatusCodeMatcher do + describe '#include?' do + context 'when given a string to parse' do + it do + matcher = described_class.new('400') + + expect(matcher).to include 400 + end + + it do + matcher = described_class.new('400,500') + + expect(matcher).to include 400 + expect(matcher).not_to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new('400-500') + + expect(matcher).to include 400 + expect(matcher).to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new('400-404,500') + + expect(matcher).to include 400 + expect(matcher).not_to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 599 + end + end + end +end diff --git a/spec/datadog/tracing/contrib/status_range_env_parser_spec.rb b/spec/datadog/tracing/contrib/status_range_env_parser_spec.rb new file mode 100644 index 00000000000..dea3cdd012e --- /dev/null +++ b/spec/datadog/tracing/contrib/status_range_env_parser_spec.rb @@ -0,0 +1,18 @@ +require 'datadog/tracing/contrib/status_range_env_parser' + +RSpec.describe Datadog::Tracing::Contrib::StatusRangeEnvParser do + describe '.call' do + [ + ['400', [400]], + ['400,500', [400, 500]], + ['400,,500', [400, 500]], + [',400,500', [400, 500]], + ['400,500,', [400, 500]], + ['400-404,500', [400..404, 500]], + ['400-404,500-504', [400..404, 500..504]], + ['400-404,444,500-504', [400..404, 444, 500..504]], + ].each do |input, result| + it { expect(described_class.call(input)).to eq(result) } + end + end +end diff --git a/spec/datadog/tracing/contrib/status_range_matcher_spec.rb b/spec/datadog/tracing/contrib/status_range_matcher_spec.rb new file mode 100644 index 00000000000..8208f3e5621 --- /dev/null +++ b/spec/datadog/tracing/contrib/status_range_matcher_spec.rb @@ -0,0 +1,58 @@ +require 'datadog/tracing/contrib/status_range_matcher' + +RSpec.describe Datadog::Tracing::Contrib::StatusRangeMatcher do + describe '#include?' do + it do + matcher = described_class.new(400) + + expect(matcher).to include 400 + expect(matcher).not_to include 401 + expect(matcher).not_to include 499 + expect(matcher).not_to include 500 + expect(matcher).not_to include 501 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new(400..500) + + expect(matcher).to include 400 + expect(matcher).to include 401 + expect(matcher).to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 501 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new([400..500]) + + expect(matcher).to include 400 + expect(matcher).to include 401 + expect(matcher).to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 501 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new([400..401, 500]) + + expect(matcher).to include 400 + expect(matcher).to include 401 + expect(matcher).not_to include 499 + expect(matcher).to include 500 + expect(matcher).not_to include 599 + end + + it do + matcher = described_class.new([400..401, 500..600]) + + expect(matcher).to include 400 + expect(matcher).to include 401 + expect(matcher).not_to include 499 + expect(matcher).to include 500 + expect(matcher).to include 599 + end + end +end From 0a4f6afafbed1b05c4ff4b21ee7895510b9fd1eb Mon Sep 17 00:00:00 2001 From: Tony Hsu Date: Wed, 3 Jan 2024 16:32:23 +0100 Subject: [PATCH 2/3] Add more tests --- .../contrib/shared_settings_examples.rb | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/spec/datadog/tracing/contrib/shared_settings_examples.rb b/spec/datadog/tracing/contrib/shared_settings_examples.rb index dec0a8ef8ee..4377ce4b2c3 100644 --- a/spec/datadog/tracing/contrib/shared_settings_examples.rb +++ b/spec/datadog/tracing/contrib/shared_settings_examples.rb @@ -39,6 +39,47 @@ it { expect(subject.error_status_codes).not_to include 600 } end + context 'when given error_status_codes' do + context 'when given a single value' do + subject { described_class.new(error_status_codes: 500) } + + it { expect(subject.error_status_codes).not_to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).not_to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } + end + + context 'when given an array of integers' do + subject { described_class.new(error_status_codes: [400, 500]) } + + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).not_to include 599 } + it { expect(subject.error_status_codes).not_to include 600 } + end + + context 'when given a range' do + subject { described_class.new(error_status_codes: 500..600) } + + it { expect(subject.error_status_codes).not_to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).to include 599 } + it { expect(subject.error_status_codes).to include 600 } + end + + context 'when given an array of integer and range' do + subject { described_class.new(error_status_codes: [400, 500..600]) } + + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).to include 599 } + it { expect(subject.error_status_codes).to include 600 } + end + context 'when configured with environment variable' do subject { described_class.new } @@ -83,5 +124,19 @@ it { expect(subject.error_status_codes).not_to include 599 } it { expect(subject.error_status_codes).not_to include 600 } end + + context 'when given a comma separated list with range' do + around do |example| + ClimateControl.modify(env => '400,500-600') do + example.run + end + end + + it { expect(subject.error_status_codes).to include 400 } + it { expect(subject.error_status_codes).not_to include 499 } + it { expect(subject.error_status_codes).to include 500 } + it { expect(subject.error_status_codes).to include 599 } + it { expect(subject.error_status_codes).to include 600 } + end end end From 4c6880998c0c3d35aa76a7bc924cdd068273c18b Mon Sep 17 00:00:00 2001 From: Tony Hsu Date: Thu, 4 Jan 2024 18:00:27 +0100 Subject: [PATCH 3/3] Fix syntax in shared examples --- spec/datadog/tracing/contrib/shared_settings_examples.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/datadog/tracing/contrib/shared_settings_examples.rb b/spec/datadog/tracing/contrib/shared_settings_examples.rb index 4377ce4b2c3..59dc6838203 100644 --- a/spec/datadog/tracing/contrib/shared_settings_examples.rb +++ b/spec/datadog/tracing/contrib/shared_settings_examples.rb @@ -79,6 +79,7 @@ it { expect(subject.error_status_codes).to include 599 } it { expect(subject.error_status_codes).to include 600 } end + end context 'when configured with environment variable' do subject { described_class.new }