Grape status codes #1238

Kyle-Neale · 2020-11-06T20:09:29Z

No description provided.

ericmustin

Nice work here, sorry for the delay. I think this is pretty close all things considered. There are a few architecture decisions i have some feedback on that I think will help us leverage this feature for other frameworks, but the implementation of the logic itself looks good to me. The choice of a set is very clever, nice stuff.

I'll try to review the tests a bit more thoroughly as well but I wanted to get back this feedback so you could review the design feedback

lib/ddtrace/contrib/grape/endpoint.rb

ericmustin · 2020-11-10T15:51:37Z

lib/ddtrace/contrib/grape/error_matcher.rb

+module Datadog
+  module Contrib
+    module Grape
+      class ErrorMatcher


I think the idea of splitting out all the error matching logic into it's own re-usable chunk of code makes sense. We can abstract out some of the logic of set_range / handle_statuses / error_responses into a broad contrib helper function that other web frameworks could leverage in the future. One approach is to construct a module similar to Http Annotation Helper, that could live in the same directory, that exposes a method called something like compute_status_ranges which takes as an input the specific integration's raw error_responses value, and does the validation+parsing, and returns a Set. then in the grape/endpoint.rb method set_range all we'd have to do is include something like:
include Datadog::Contrib::HttpErrorResponseStatusHelper

call something like

def set_range @datadog_set ||= compute_status_ranges(datadog_configuration[:error_responses])` end

And all the repetitive work would be re-usable in different web frameworks. basically let's encapsulate everything we can in a way that framework agnostic, so then all we have to do is include the helper module. This also makes testing easier

ericmustin · 2020-11-10T15:53:05Z

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

@@ -24,6 +24,7 @@ class Settings < Contrib::Configuration::Settings
          end

          option :service_name, default: Ext::SERVICE_NAME
+          option :error_responses, default: '500-599' # quite possible we may be able to use Datadog::Ext::HTTP::ERROR_RANGE


I agree, It makes sense to use the constant when possible, i think it fits here. It's also fine to make a new constant if you feel the use case is different (even if the value is the same) Datadog::Ext::HTTP::ERROR_RESPONSE_RANGE perhaps

option :error_responses, default: '500-599' do |o| o.default { '500-599' } o.setter do |new_value, old_value| Datadog::Contrib::Helpers::StatusCodeMatcher.new(new_value) end end

We should ensure that StatusCodeMatcher implements to_s, as we are going to print it during start up. to_s should return the configured value ('500-599' in this case).

lib/ddtrace/contrib/grape/endpoint.rb

ericmustin · 2020-11-10T15:55:55Z

lib/ddtrace/contrib/grape/endpoint.rb

+            return datadog_configuration[:error_responses] if datadog_configuration[:error_responses].is_a?(String) && !status.nil?
+            datadog_configuration[:error_responses].join(',') if status.is_a?(Array) && !status.empty?


what is status in this case? i don't see that method defined anywhere

This was actually code that I was testing and forgot to update in the actual dd-trace-rb. I'll update it but its' supposed to be datadog_configuration[:error_responses].

👍 sounds good

ericmustin · 2020-11-10T16:00:24Z

lib/ddtrace/contrib/grape/endpoint.rb

+          def handle_statuses
+            if error_responses
+              error_responses.gsub(/\s+/, '').split(',').select do |code|
+                if !code.to_s.match(/^\d{3}(?:-\d{3})?(?:,\d{3}(?:-\d{3})?)*$/)


Let's make abstract as much of this regex stuff into constants as we can, it should help perf, see an example here

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

marcotc · 2020-11-10T17:07:20Z

lib/ddtrace/contrib/grape/endpoint.rb

+          def exception_is_error?(exception)
+            status = nil
+            return false unless exception
+            if exception.respond_to?('status') && set_range.include?(exception.status)


After https://github.com/DataDog/dd-trace-rb/pull/1238/files#r520721404 is implemented, datadog_configuration[:error_responses] will return the matcher object StatusCodeMatcher, and we'll be able to directly call it:

matcher = datadog_configuration[:error_responses] if exception.respond_to?('status') && matcher && matcher.include?(exception.status)

marcotc · 2020-11-11T18:50:56Z

lib/ddtrace/contrib/grape/endpoint.rb

+          def exception_is_error?(exception)
+            matcher = datadog_configuration[:error_responses]
+            return false unless exception
+            if exception.respond_to?('status') && matcher && matcher.set_range.include?(exception.status)


I think the implementation looks more clear if we hide the fact the we have a set_range internally.

If we only expose the methods that we'd like the user to have access to, I believe the code looks more organized:

Suggested change

if exception.respond_to?('status') && matcher && matcher.set_range.include?(exception.status)

if exception.respond_to?('status') && matcher && matcher.include?(exception.status)

This requires implementing def include?(status) in the StatusCodeMatcher class.

Sounds good!

Ditto to @marcotc comments, and also just suggesting that && matcher is not necessary if we have a default

ericmustin

Some small points of feedback, but very close

One other thing not mentioned is that

Let's move this out of draft (you can select "ready for review")
Let's add a brief update to the GettingStarted.md markdown Grape section that explains the option briefly https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md#grape. We just need to update the options table to include the new option

key (error_responses),
description (brief summary of what the option does. can be a few sentences if needed), and
default value('500-599')

ericmustin · 2020-11-12T10:03:56Z

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

@@ -24,6 +24,12 @@ class Settings < Contrib::Configuration::Settings
          end

          option :service_name, default: Ext::SERVICE_NAME
+          option :error_responses, default: '500-599' do |o|


we don't need default here and in the block, can remove here

ericmustin · 2020-11-12T10:05:40Z

lib/ddtrace/contrib/grape/endpoint.rb

 require 'ddtrace/ext/http'
 require 'ddtrace/ext/errors'
 require 'ddtrace/contrib/analytics'
 require 'ddtrace/contrib/rack/ext'
+require 'ddtrace/contrib/status_code_matcher'


i dont think we need to include the status_code_matcher file here, but in the grape/settings.rb file instead

ericmustin · 2020-11-12T10:07:41Z

lib/ddtrace/contrib/grape/endpoint.rb

+          def exception_is_error?(exception)
+            matcher = datadog_configuration[:error_responses]
+            return false unless exception
+            if exception.respond_to?('status') && matcher && matcher.set_range.include?(exception.status)


Ditto to @marcotc comments, and also just suggesting that && matcher is not necessary if we have a default

ericmustin · 2020-11-12T10:09:58Z

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

@@ -24,6 +24,12 @@ class Settings < Contrib::Configuration::Settings
          end

          option :service_name, default: Ext::SERVICE_NAME
+          option :error_responses, default: '500-599' do |o|
+            o.default { '500-599' }


I believe the default should be Datadog::Contrib::StatusCodeMatcher.new(500-599), if i'm understanding things correctly.

also let's abstract out 500-599 into a constant within the ddtrace/ext/http module, that will help with keeping things DRY

Hmm wondering if it makes sense to just use the ERROR_RANGE that's already provided. With that, I can do something like this:

option :error_responses, default: '500-599' do |o| o.default { Datadog::Contrib::StatusCodeMatcher.new((Datadog::Ext::HTTP::ERROR_RANGE).to_a) } ...

This way, we get functionality from an existing variable rather than defining a new and similar constant

sure i think that works just fine

ericmustin · 2020-11-12T10:12:53Z

lib/ddtrace/contrib/status_code_matcher.rb

+      end
+
+      def to_s
+        @@error_response_range.to_s


why the @@ class variable here instead of the @ instance variable?

ericmustin · 2020-11-12T10:15:28Z

lib/ddtrace/contrib/status_code_matcher.rb

+          end
+        else
+          Datadog.logger.debug('No valid config was provided for :error_responses - falling back to default.')
+          ['500-599'] # Rather than returning an empty array, we need to fallback to default config.


500-599 can be replaced with a sensibly named default constant within, `'ddtrace/ext/http', as discussed previously.

…variable in alignment with RFC

ericmustin

two extremely small changes then lgtm

ericmustin · 2020-11-12T16:33:31Z

docs/GettingStarted.md

@@ -816,6 +816,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. `'500-599'` or `['500-599']` | `500-599` |


small nit, could we also include an example here that shows support multiple ranges, for clarity? It's unclear currently that there can be multiple ranges

lib/ddtrace/contrib/status_code_matcher.rb

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

ericmustin · 2020-11-12T16:45:53Z

ugh the linting gods

Changes have been added

marcotc · 2020-11-12T16:49:42Z

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

+            o.default { Datadog::Contrib::StatusCodeMatcher.new(Datadog::Ext::HTTP::ERROR_RANGE.to_a) }
+            o.setter do |new_value, _old_value|
+              Datadog::Contrib::StatusCodeMatcher.new(new_value)
+            end


Does the default case still work?

I think it will perform:

# from o.default { Datadog::Contrib::StatusCodeMatcher.new(Datadog::Ext::HTTP::ERROR_RANGE.to_a) } default = Datadog::Contrib::StatusCodeMatcher.new(Datadog::Ext::HTTP::ERROR_RANGE.to_a) # from o.setter do |new_value, _old_value| Datadog::Contrib::StatusCodeMatcher.new(default)

Which will likely break when StatusCodeMatcher.new receives an instance of StatusCodeMatcher instead of a string 🤔

ah, woops, that's my bad i had suggested that

...yea let's use default as just Datadog::Ext::HTTP::ERROR_RANGE.to_a

It doesn't break bc @Kyle-Neale wrote good error handling / checking in StatusCodeMatcher but, yea, we should be passing in the raw value, my fault.

spec/ddtrace/contrib/grape/tracer_spec.rb

…matchers Co-authored-by: Marco Costa <mmarcottulio@gmail.com>

Kyle-Neale · 2020-11-12T17:21:09Z

Checks are failing from the changes using custom handles that @marcotc suggested. Why should we be expecting the exception to not throw an error?

marcotc · 2020-11-12T18:06:25Z

@Kyle-Neale I read your statement as the opposite: 86c8c32 (#1238)

expect(spans[0].get_tag('error.stack')).to_not be_nil

should become

expect(spans[0]).to have_error

ericmustin

Left some last mile changes here to ensure we dont introduce breaking changes. should be good to go after this

ericmustin · 2020-11-12T19:03:48Z

spec/ddtrace/contrib/grape/tracer_spec.rb

+            expect(spans[0].get_tag('error.type')).to_not be_nil
+            expect(spans[0].get_tag('error.msg')).to_not be_nil
+          end
+        end


Suggested change

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

docs/GettingStarted.md

ericmustin · 2020-11-12T19:05:18Z

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

+          option :error_statuses do |o|
+            o.default { Datadog::Ext::HTTP::ERROR_RANGE.to_a }
+            o.setter do |new_value, _old_value|
+              Datadog::Contrib::StatusCodeMatcher.new(new_value)
+            end


Unfort we need to default to nil here to ensure we dont introduce a breaking change

Suggested change

option :error_statuses do |o|

o.default { Datadog::Ext::HTTP::ERROR_RANGE.to_a }

o.setter do |new_value, _old_value|

Datadog::Contrib::StatusCodeMatcher.new(new_value)

end

option :error_statuses do |o|

o.default { nil }

o.setter do |new_value, _old_value|

return if new_value.nil?

Datadog::Contrib::StatusCodeMatcher.new(new_value)

end

ericmustin · 2020-11-12T19:07:55Z

lib/ddtrace/contrib/grape/endpoint.rb

+          def exception_is_error?(exception)
+            matcher = datadog_configuration[:error_statuses]
+            return false unless exception
+            if exception.respond_to?('status') && matcher.include?(exception.status)
+              status = exception.status
+            else
+              return true
+            end
+            !status.nil?
+          end


Given that datadog_configuration[:error_statuses] willl now default to nil, we can clean up this logic a bit. currently, by having if exception.respond_to?('status') && matcher.include?(exception.status) in the same if statment, we aren't properly returning false when status exists but matcher.include?(exception.status) returns false. The below updates to account for this

Suggested change

def exception_is_error?(exception)

matcher = datadog_configuration[:error_statuses]

return false unless exception

if exception.respond_to?('status') && matcher.include?(exception.status)

status = exception.status

else

return true

end

!status.nil?

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

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

ericmustin

Some very small nits/typos then this is good to go, approving now, nice work!

lib/ddtrace/ext/http.rb

docs/GettingStarted.md

lib/ddtrace/contrib/status_code_matcher.rb

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

ericmustin

🚀

spec/ddtrace/contrib/grape/tracer_spec.rb

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

Kyle Neale and others added 11 commits October 29, 2020 17:22

Seeing what the pyload exception outputs in an error scenario

3cbfe3c

3d27396

ee870dd

Fixed lint issues

4f54b20

Implemented the handle_status method

50815fa

Implemented set_range method

43c44d5

Updated test spec to pass new configuration option

f104079

Small refactor and added fallback for invalid config when provdided

a8b5a18

Added handling for both arrays and string for :error_responses

6e1007e

Added handling for both arrays and string for :error_responses

b06bad6

Added handling for nil and integers

b92c48a

ericmustin suggested changes Nov 10, 2020

View reviewed changes

Update lib/ddtrace/contrib/grape/endpoint.rb

51e92bb

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

marcotc reviewed Nov 10, 2020

View reviewed changes

marcotc mentioned this pull request Nov 10, 2020

Grape error handling #1231

Closed

Kyle-Neale added 3 commits November 10, 2020 14:28

Added class status_code_matcher to modularize code

598a4d8

aa6197e

changed how set_range function checks for non-nil matcher

4266841

marcotc reviewed Nov 11, 2020

View reviewed changes

ericmustin suggested changes Nov 12, 2020

View reviewed changes

Adding config option to Grape GettingStarted.md as well as redefined …

92fb984

…variable in alignment with RFC

Kyle-Neale marked this pull request as ready for review November 12, 2020 16:11

Kyle-Neale requested a review from a team November 12, 2020 16:11

ericmustin previously requested changes Nov 12, 2020

View reviewed changes

Kyle-Neale and others added 2 commits November 12, 2020 11:41

Update lib/ddtrace/contrib/status_code_matcher.rb

f27ba4a

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

Include examples to support multiple ranges

42804e5

ericmustin added this to the 0.43.0 milestone Nov 12, 2020

ericmustin assigned Kyle-Neale Nov 12, 2020

ericmustin added the feature Involves a product feature label Nov 12, 2020

ericmustin added the integrations Involves tracing integrations label Nov 12, 2020

marcotc reviewed Nov 12, 2020

View reviewed changes

spec/ddtrace/contrib/grape/tracer_spec.rb Show resolved Hide resolved

Kyle-Neale and others added 2 commits November 12, 2020 12:06

Update spec/ddtrace/contrib/grape/tracer_spec.rb to use our customer …

86c8c32

…matchers Co-authored-by: Marco Costa <mmarcottulio@gmail.com>

changed default for :error

2eca396

ericmustin suggested changes Nov 12, 2020

View reviewed changes

Kyle-Neale and others added 3 commits November 12, 2020 14:56

Fixed default behavior when only one invalid response range is set

4388559

Approvided by the lint gods

0119850

updated docs/GettingStarted.md for grape to default to nil

8691369

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

ericmustin previously approved these changes Nov 12, 2020

View reviewed changes

lib/ddtrace/ext/http.rb Outdated Show resolved Hide resolved

docs/GettingStarted.md Outdated Show resolved Hide resolved

lib/ddtrace/contrib/status_code_matcher.rb Outdated Show resolved Hide resolved

lib/ddtrace/contrib/status_code_matcher.rb Outdated Show resolved Hide resolved

remove dev logging

b94f510

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

Kyle-Neale dismissed ericmustin’s stale review via b94f510 November 12, 2020 20:57

Kyle-Neale and others added 3 commits November 12, 2020 15:58

remove dev logging

9731c43

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

Update lib/ddtrace/ext/http.rb

8455477

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

Update docs/GettingStarted.md

37d523b

Co-authored-by: Eric Mustin <mustin.eric@gmail.com>

ericmustin previously approved these changes Nov 12, 2020

View reviewed changes

ericmustin reviewed Nov 12, 2020

View reviewed changes

spec/ddtrace/contrib/grape/tracer_spec.rb Outdated Show resolved Hide resolved

lib/ddtrace/contrib/grape/configuration/settings.rb Outdated Show resolved Hide resolved

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

fe8295e

ericmustin dismissed their stale review via fe8295e November 12, 2020 21:28

Update spec/ddtrace/contrib/grape/tracer_spec.rb

8a972b0

ericmustin approved these changes Nov 12, 2020

View reviewed changes

ericmustin merged commit f4346a6 into DataDog:master Nov 12, 2020

Kyle-Neale deleted the grape_status_codes branch November 12, 2020 22:00

		return datadog_configuration[:error_responses] if datadog_configuration[:error_responses].is_a?(String) && !status.nil?
		datadog_configuration[:error_responses].join(',') if status.is_a?(Array) && !status.empty?

	if exception.respond_to?('status') && matcher && matcher.set_range.include?(exception.status)
	if exception.respond_to?('status') && matcher && matcher.include?(exception.status)

-        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

Grape status codes #1238

Grape status codes #1238

Conversation

Kyle-Neale commented Nov 6, 2020

ericmustin left a comment

Choose a reason for hiding this comment

ericmustin Nov 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc Nov 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Neale Nov 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmustin commented Nov 12, 2020

marcotc Nov 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Neale commented Nov 12, 2020 • edited

marcotc commented Nov 12, 2020

ericmustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmustin left a comment

Choose a reason for hiding this comment

ericmustin left a comment

Choose a reason for hiding this comment

ericmustin Nov 10, 2020 •

edited

marcotc Nov 10, 2020 •

edited

Kyle-Neale Nov 12, 2020 •

edited

marcotc Nov 12, 2020 •

edited

Kyle-Neale commented Nov 12, 2020 •

edited