Merge pull request #1238 from Kyle-Neale/grape_status_codes

Grape status codes

docs/GettingStarted.md

@@ -855,6 +855,7 @@ Where `options` is an optional `Hash` that accepts the following parameters:
 | `analytics_enabled` | Enable analytics for spans produced by this integration. `true` for on, `nil` to defer to global setting, `false` for off. | `nil` |
 | `enabled` | Defines whether Grape should be traced. Useful for temporarily disabling tracing. `true` or `false` | `true` |
 | `service_name` | Service name used for `grape` instrumentation | `'grape'` |
+| `error_statuses`| Defines a status code or range of status codes which should be marked as errors. `'404,405,500-599'` or `[404,405,'500-599']` | `nil` |
 
 ### GraphQL
 

lib/ddtrace/contrib/grape/configuration/settings.rb

@@ -1,6 +1,7 @@
 require 'ddtrace/contrib/configuration/settings'
 require 'ddtrace/ext/http'
 require 'ddtrace/contrib/grape/ext'
+require 'ddtrace/contrib/status_code_matcher'
 
 module Datadog
   module Contrib
@@ -24,6 +25,12 @@ class Settings < Contrib::Configuration::Settings
           end
 
           option :service_name, default: Ext::SERVICE_NAME
+
+          option :error_statuses, default: nil do |o|
+            o.setter do |new_value, _old_value|
+              Datadog::Contrib::StatusCodeMatcher.new(new_value) unless new_value.nil?
+            end
+          end
         end
       end
     end

lib/ddtrace/contrib/grape/endpoint.rb

@@ -1,4 +1,3 @@
-
 require 'ddtrace/ext/http'
 require 'ddtrace/ext/errors'
 require 'ddtrace/contrib/analytics'
@@ -90,7 +89,10 @@ def endpoint_run(name, start, finish, id, payload)
               Contrib::Analytics.set_measured(span)
 
               # catch thrown exceptions
-              span.set_error(payload[:exception_object]) unless payload[:exception_object].nil?
+
+              if exception_is_error?(payload[:exception_object])
+                span.set_error(payload[:exception_object])
+              end
 
               # override the current span with this notification values
               span.set_tag(Ext::TAG_ROUTE_ENDPOINT, api_view) unless api_view.nil?
@@ -133,7 +135,9 @@ def endpoint_render(name, start, finish, id, payload)
               # Measure service stats
               Contrib::Analytics.set_measured(span)
 
-              span.set_error(payload[:exception_object]) unless payload[:exception_object].nil?
+              if exception_is_error?(payload[:exception_object])
+                span.set_error(payload[:exception_object])
+              end
             ensure
               span.start(start)
               span.finish(finish)
@@ -168,7 +172,10 @@ def endpoint_run_filters(name, start, finish, id, payload)
               Contrib::Analytics.set_measured(span)
 
               # catch thrown exceptions
-              span.set_error(payload[:exception_object]) unless payload[:exception_object].nil?
+              if exception_is_error?(payload[:exception_object])
+                span.set_error(payload[:exception_object])
+              end
+
               span.set_tag(Ext::TAG_FILTER_TYPE, type.to_s)
             ensure
               span.start(start)
@@ -196,6 +203,14 @@ def analytics_sample_rate
             datadog_configuration[:analytics_sample_rate]
           end
 
+          def exception_is_error?(exception)
+            matcher = datadog_configuration[:error_statuses]
+            return false unless exception
+            return true unless matcher
+            return true unless exception.respond_to?('status')
+            matcher.include?(exception.status)
+          end
+
           def enabled?
             datadog_configuration[:enabled] == true
           end

lib/ddtrace/contrib/status_code_matcher.rb

@@ -0,0 +1,67 @@
+require 'set'
+require 'ddtrace/ext/http'
+
+module Datadog
+  module Contrib
+    # Contains methods helpful for tracing/annotating HTTP request libraries
+    class StatusCodeMatcher
+      REGEX_PARSER = /^\d{3}(?:-\d{3})?(?:,\d{3}(?:-\d{3})?)*$/
+
+      def initialize(range)
+        @error_response_range = range
+        set_range
+      end
+
+      def include?(exception_status)
+        set_range.include?(exception_status)
+      end
+
+      def to_s
+        @error_response_range.to_s
+      end
+
+      private
+
+      def set_range
+        @datadog_set ||= begin
+          set = Set.new
+          handle_statuses.each do |statuses|
+            status = statuses.to_s.split('-')
+            if status.length == 1
+              set.add(Integer(status[0]))
+            elsif status.length == 2
+              min, max = status.minmax
+              Array(min..max).each do |i|
+                set.add(Integer(i))
+              end
+            end
+          end
+          set
+        end
+        @datadog_set
+      end
+
+      def error_responses
+        return @error_response_range if @error_response_range.is_a?(String) && !@error_response_range.nil?
+        @error_response_range.join(',') if @error_response_range.is_a?(Array) && !@error_response_range.empty?
+      end
+
+      def handle_statuses
+        if error_responses
+          filter_error_responses = error_responses.gsub(/\s+/, '').split(',').select do |code|
+            if !code.to_s.match(REGEX_PARSER)
+              Datadog.logger.debug("Invalid config provided: #{code}. Must be formatted like '400-403,405,410-499'.")
+              next
+            else
+              true
+            end
+          end
+          filter_error_responses.empty? ? Datadog::Ext::HTTP::ERROR_RANGE.to_a : filter_error_responses
+        else
+          Datadog.logger.debug('No valid config was provided for :error_statuses - falling back to default.')
+          Datadog::Ext::HTTP::ERROR_RANGE.to_a
+        end
+      end
+    end
+  end
+end

spec/ddtrace/contrib/grape/tracer_spec.rb

@@ -196,6 +196,80 @@
     end
 
     context 'failure' do
+      context 'without filters' do
+        subject(:response) { post '/base/hard_failure' }
+
+        it 'should handle exceptions' do
+          expect(response.body).to eq('405 Not Allowed')
+          expect(spans.length).to eq(1)
+          expect(spans[0].name).to eq('grape.endpoint_run')
+          expect(spans[0].status).to eq(1)
+          expect(spans[0].get_tag('error.stack')).to_not be_nil
+          expect(spans[0].get_tag('error.type')).to_not be_nil
+          expect(spans[0].get_tag('error.msg')).to_not be_nil
+        end
+
+        context 'and error_responses' do
+          subject(:response) { post '/base/hard_failure' }
+          let(:configuration_options) { { error_statuses: '300-399,,xxx-xxx,1111,400-499' } }
+
+          it 'should handle exceptions' do
+            expect(response.body).to eq('405 Not Allowed')
+            expect(spans.length).to eq(1)
+            expect(spans[0].name).to eq('grape.endpoint_run')
+            expect(spans[0].status).to eq(1)
+            expect(spans[0].get_tag('error.stack')).to_not be_nil
+            expect(spans[0].get_tag('error.type')).to_not be_nil
+            expect(spans[0].get_tag('error.msg')).to_not be_nil
+          end
+        end
+
+        context 'and error_responses with arrays' do
+          subject(:response) { post '/base/hard_failure' }
+          let(:configuration_options) { { error_statuses: ['300-399', 'xxx-xxx', 1111, 405] } }
+
+          it 'should handle exceptions' do
+            expect(response.body).to eq('405 Not Allowed')
+            expect(spans.length).to eq(1)
+            expect(spans[0].name).to eq('grape.endpoint_run')
+            expect(spans[0].status).to eq(1)
+            expect(spans[0].get_tag('error.stack')).to_not be_nil
+            expect(spans[0].get_tag('error.type')).to_not be_nil
+            expect(spans[0].get_tag('error.msg')).to_not be_nil
+          end
+        end
+
+        context 'and error_responses with arrays that dont contain exception status' do
+          subject(:response) { post '/base/hard_failure' }
+          let(:configuration_options) { { error_statuses: ['300-399', 'xxx-xxx', 1111, 406] } }
+
+          it 'should handle exceptions' do
+            expect(response.body).to eq('405 Not Allowed')
+            expect(spans.length).to eq(1)
+            expect(spans[0].name).to eq('grape.endpoint_run')
+            expect(spans[0]).to_not have_error
+            expect(spans[0].get_tag('error.stack')).to be_nil
+            expect(spans[0].get_tag('error.type')).to be_nil
+            expect(spans[0].get_tag('error.msg')).to be_nil
+          end
+        end
+
+        context 'defaults to >=500 when provided invalid config' do
+          subject(:response) { post '/base/hard_failure' }
+          let(:configuration_options) { { error_statuses: 'xxx-499' } }
+
+          it 'should handle exceptions' do
+            expect(response.body).to eq('405 Not Allowed')
+            expect(spans.length).to eq(1)
+            expect(spans[0].name).to eq('grape.endpoint_run')
+            expect(spans[0].status).to eq(0)
+            expect(spans[0].get_tag('error.stack')).to be_nil
+            expect(spans[0].get_tag('error.type')).to be_nil
+            expect(spans[0].get_tag('error.msg')).to be_nil
+          end
+        end
+      end
+
       context 'without filters' do
         subject(:response) { get '/base/hard_failure' }
 
@@ -333,79 +407,79 @@
         expect(rack_span).to_not have_error
         expect(rack_span.parent).to be_nil
       end
+    end
 
-      context 'failure' do
-        subject(:response) { get '/api/hard_failure' }
+    context 'failure' do
+      subject(:response) { get '/api/hard_failure' }
 
-        it_behaves_like 'measured span for integration', true do
-          before do
-            expect { subject }.to raise_error(StandardError, 'Ouch!')
-          end
+      it_behaves_like 'measured span for integration', true do
+        before do
+          expect { subject }.to raise_error(StandardError, 'Ouch!')
         end
+      end
 
-        it_behaves_like 'analytics for integration', ignore_global_flag: false do
-          let(:span) { spans.find { |x| x.name == Datadog::Contrib::Grape::Ext::SPAN_ENDPOINT_RUN } }
-          let(:analytics_enabled_var) { Datadog::Contrib::Grape::Ext::ENV_ANALYTICS_ENABLED }
-          let(:analytics_sample_rate_var) { Datadog::Contrib::Grape::Ext::ENV_ANALYTICS_SAMPLE_RATE }
-          before do
-            expect { subject }.to raise_error(StandardError, 'Ouch!')
-          end
+      it_behaves_like 'analytics for integration', ignore_global_flag: false do
+        let(:span) { spans.find { |x| x.name == Datadog::Contrib::Grape::Ext::SPAN_ENDPOINT_RUN } }
+        let(:analytics_enabled_var) { Datadog::Contrib::Grape::Ext::ENV_ANALYTICS_ENABLED }
+        let(:analytics_sample_rate_var) { Datadog::Contrib::Grape::Ext::ENV_ANALYTICS_SAMPLE_RATE }
+        before do
+          expect { subject }.to raise_error(StandardError, 'Ouch!')
         end
+      end
 
-        it 'should integrate with Racck integration when exception is thrown' do
-          expect { subject }.to raise_error(StandardError, 'Ouch!')
-          expect(spans.length).to eq(3)
+      it 'should integrate with Rack integration when exception is thrown' do
+        expect { subject }.to raise_error(StandardError, 'Ouch!')
+        expect(spans.length).to eq(3)
 
-          render_span, run_span, rack_span = spans
+        render_span, run_span, rack_span = spans
 
-          expect(render_span.name).to eq('grape.endpoint_render')
-          expect(render_span.span_type).to eq('template')
-          expect(render_span.service).to eq('grape')
-          expect(render_span.resource).to eq('grape.endpoint_render')
-          expect(render_span).to have_error
-          expect(render_span).to have_error_type('StandardError')
-          expect(render_span).to have_error_message('Ouch!')
-          expect(render_span.get_tag('error.stack')).to include('grape/tracer_spec.rb')
-          expect(render_span.parent).to eq(run_span)
+        expect(render_span.name).to eq('grape.endpoint_render')
+        expect(render_span.span_type).to eq('template')
+        expect(render_span.service).to eq('grape')
+        expect(render_span.resource).to eq('grape.endpoint_render')
+        expect(render_span).to have_error
+        expect(render_span).to have_error_type('StandardError')
+        expect(render_span).to have_error_message('Ouch!')
+        expect(render_span.get_tag('error.stack')).to include('grape/tracer_spec.rb')
+        expect(render_span.parent).to eq(run_span)
 
-          expect(run_span.name).to eq('grape.endpoint_run')
-          expect(run_span.span_type).to eq('web')
-          expect(run_span.service).to eq('grape')
-          expect(run_span.resource).to eq('RackTestingAPI#hard_failure')
-          expect(run_span).to have_error
-          expect(run_span.parent).to eq(rack_span)
-
-          expect(rack_span.name).to eq('rack.request')
-          expect(rack_span.span_type).to eq('web')
-          expect(rack_span.service).to eq('rack')
-          expect(rack_span.resource).to eq('RackTestingAPI#hard_failure')
-          expect(rack_span).to have_error
-          expect(rack_span.parent).to be_nil
-        end
+        expect(run_span.name).to eq('grape.endpoint_run')
+        expect(run_span.span_type).to eq('web')
+        expect(run_span.service).to eq('grape')
+        expect(run_span.resource).to eq('RackTestingAPI#hard_failure')
+        expect(run_span).to have_error
+        expect(run_span.parent).to eq(rack_span)
+
+        expect(rack_span.name).to eq('rack.request')
+        expect(rack_span.span_type).to eq('web')
+        expect(rack_span.service).to eq('rack')
+        expect(rack_span.resource).to eq('RackTestingAPI#hard_failure')
+        expect(rack_span).to have_error
+        expect(rack_span.parent).to be_nil
       end
+    end
 
-      context 'missing route' do
-        subject(:response) { get '/api/not_existing' }
+    context 'missing route' do
+      subject(:response) { get '/api/not_existing' }
 
-        it_behaves_like 'measured span for integration', true do
-          before do
-            expect(subject.status).to eq(404)
-          end
+      it_behaves_like 'measured span for integration', true do
+        before do
+          expect(subject.status).to eq(404)
         end
+      end
 
-        it 'it should not impact the Rack integration that must work as usual' do
-          expect(subject.status).to eq(404)
-          expect(spans.length).to eq(1)
+      it 'it should not impact the Rack integration that must work as usual' do
+        expect(subject.status).to eq(404)
+        expect(spans.length).to eq(1)
 
-          rack_span = spans[0]
+        rack_span = spans[0]
 
-          expect(rack_span.name).to eq('rack.request')
-          expect(rack_span.span_type).to eq('web')
-          expect(rack_span.service).to eq('rack')
-          expect(rack_span.resource).to eq('GET 404')
-          expect(rack_span).to_not have_error
-          expect(rack_span.parent).to be_nil
-        end
+        expect(rack_span.name).to eq('rack.request')
+        expect(rack_span.span_type).to eq('web')
+        expect(rack_span.service).to eq('rack')
+        expect(rack_span.resource).to eq('GET 404')
+        expect(rack_span).to_not have_error
+        expect(rack_span.parent).to be_nil
       end
     end
   end