[grape] add Grape integration for API endpoints #117

palazzem · 2017-04-21T08:47:11Z

What it does

Add Grape auto instrumentation using the Patcher method. Grape classes are patched so that new instrumentation is available before the task is executed. It requires ActiveSupport and relies on Grape signals to:

trace the whole endpoint execution
trace before and after filters
trace the Endpoint body

This auto instrumentation works standalone, or using the Rack and/or Rails auto instrumentation. If Rails is used, both Rack and Grape integrations are activated.

What's missing

documentation
integration test and instrumentation for Rails

ufoot

Looks really good at this stage, waiting for you final signal for definitive review, but 👍

ufoot · 2017-04-21T08:52:42Z

lib/ddtrace/contrib/grape/endpoint.rb

+          return if Thread.current[KEY_RUN]
+
+          # retrieve the tracer from the PIN object
+          pin = Datadog::Pin.get_from(::Grape)


Doing it this way limits us to "one Grape service per process" as this is shared app wide AFAIK. We can live with it, but it's worth noting. I suspect in most cases it's OK, several apps running on the same server could still override the default with their own respective services names, and after all, it looks like Grape is like this by design.

You usually have one Grape for each application (and so process). If this is not true, it means that you're mounting 2 different Grape applications in the same application. If we want to support that, it means that we should even support having 2 Rails applications running together in the same process.

The major issue, is that supporting this is not so easy. We don't have instances here and adding PIN references in all endpoints and subscribers is quite complex I think.

Yup, not worth the complexity of attaching the PIN to instances, I'm just saying -> let's keep aware we have this limitation.

Yea, considering that the standard case is having one Grape application with namespaces. We still not cover some corner case by the way.

ufoot · 2017-04-21T08:58:29Z

lib/ddtrace/contrib/grape/endpoint.rb

+      # Endpoint module includes a list of subscribers to create
+      # traces when a Grape endpoint is hit
+      module Endpoint
+        KEY_RUN = 'datadog_grape_endpoint_run'.freeze


Thanks for using consts here instead of copy/pasting the keys. Adds a lookup for humans reading the code but much more typo safe OTOH.

ufoot · 2017-04-21T09:04:49Z

lib/ddtrace/contrib/grape/endpoint.rb

+          span.start_time = start
+          span.set_tag('grape.route.endpoint', api_view)
+          span.set_tag('grape.route.path', path)
+          span.finish_at(finish)


This is globally fine. I'm nitpicking but would still probably harden it like in https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/contrib/rails/action_controller.rb#L71 by adding an ensure clause around start_time = start and finish. Just, to make sure it makes its way down the pipeline. The heuristic I use here is quite dumb -> if there are at least 10 lines of code, however simple they mike look, the probability it yields an exception "some day in the future" is high.

Definitely! forgot to add this enforcing, thanks!

ufoot · 2017-04-21T09:05:17Z

lib/ddtrace/contrib/grape/endpoint.rb

+          span.set_error(payload[:exception_object]) unless payload[:exception_object].nil?
+
+          span.set_tag('grape.filter.type', type.to_s)
+          span.start_time = start


Same as above, ensure.

ufoot · 2017-04-21T09:05:47Z

lib/ddtrace/contrib/grape/patcher.rb

+        module_function
+
+        def patch
+          if !@patched && defined?(::Grape)


Thanks for using that patcher pattern, 👍

ufoot · 2017-04-21T09:05:59Z

lib/ddtrace/contrib/grape/patcher.rb

+        end
+
+        def unpatch
+          # TODO: implement this (revert aliasing)


ufoot · 2017-04-21T09:08:29Z

lib/ddtrace/monkey.rb

 require 'ddtrace/contrib/redis/patcher'
+require 'ddtrace/contrib/http/patcher'


Was there an issue with the order? (just curious, I'm not requiring any change here)

oh no, it's just cosmetic :D

ufoot · 2017-04-21T09:11:11Z

test/contrib/grape/app.rb

+      sleep(0.01)
+    end
+
+    desc 'Returns an error'


Is that really the right comment?

ufoot

Waiting for CI, but else, LGTM.

ufoot · 2017-04-21T09:56:24Z

test/monkey_test.rb

    assert_equal(false, Datadog::Contrib::ActiveRecord::Patcher.patched?)
-    assert_equal({ elasticsearch: true, http: true, redis: true, active_record: false }, Datadog::Monkey.get_patched_modules())
+    assert_equal({ elasticsearch: true, http: true, redis: true, grape: true, active_record: false }, Datadog::Monkey.get_patched_modules())


I think we don't run the tests for Grape with old Ruby versions, but I would assume this global monkey patching test is run with oldies such as 1.9. So I'm wondering if we should keep this and just drop it, probably removing the require 'grape' in this test would be enough. Let's wait for the CI advice, what I'm saying is: for a first release, if Grape does not work with 1.9, let's save this for later and choose whatever path makes it straight to the release, we don't want to delay 0.7.0 for such a corner case. If all tests pass -> forget this remark.

… Rack instrumentation

…hod is available at definition time

palazzem added the integrations Involves tracing integrations label Apr 21, 2017

palazzem added this to the 0.7.0 milestone Apr 21, 2017

palazzem requested a review from ufoot April 21, 2017 08:47

ufoot reviewed Apr 21, 2017

View reviewed changes

ufoot approved these changes Apr 21, 2017

View reviewed changes

palazzem force-pushed the palazzem/rails-grape branch from 28b7d69 to fba59e6 Compare April 21, 2017 10:03

Emanuele Palazzetti added 16 commits April 21, 2017 17:26

[grape] add appraisal grape

6236baa

[grape] provide the first draft implementation

92c6560

[grape] bootstrap testing suite for standalone Grape

5d7acff

[grape] provide patch() implementation; Monkey module patches Grape

f1673dc

[grape] instrumentation uses the PIN object; Grape works even without…

9fa4b73

… Rack instrumentation

[grape] handle exceptions and errors in Grape endpoints

ff6aa96

[grape] patch Grape before defining tests; ensure that the render met…

f18c765

…hod is available at definition time

[grape] Grape integration plays well with the Rack middleware

ef0f288

[grape] rubocop cosmetics

65e4bc5

[grape] support for Grape 0.19 requires Ruby > 2.1

289de6a

[rails] auto_instrument grape if enabled

76e596b

[grape] ensure that spans are closed

4aa47e2

[grape] update Monkey tests

1c39800

[grape] change endpoint comment

3f8afa4

[grape] add Grape documentation

d2334b3

[docs] moved Grape to other libraries

45dc54b

palazzem force-pushed the palazzem/rails-grape branch from 65eb477 to 45dc54b Compare April 21, 2017 15:28

palazzem merged commit c26b009 into master Apr 21, 2017

palazzem deleted the palazzem/rails-grape branch April 21, 2017 15:49

palazzem mentioned this pull request Apr 24, 2017

Trace exceptions through Rack middleware #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grape] add Grape integration for API endpoints #117

[grape] add Grape integration for API endpoints #117

palazzem commented Apr 21, 2017 •

edited

Loading

ufoot left a comment

ufoot Apr 21, 2017

palazzem Apr 21, 2017

ufoot Apr 21, 2017

palazzem Apr 21, 2017

ufoot Apr 21, 2017

ufoot Apr 21, 2017

palazzem Apr 21, 2017

ufoot Apr 21, 2017

ufoot Apr 21, 2017

ufoot Apr 21, 2017

ufoot Apr 21, 2017

palazzem Apr 21, 2017

ufoot Apr 21, 2017

palazzem Apr 21, 2017

ufoot left a comment

ufoot Apr 21, 2017

palazzem Apr 21, 2017

		require 'ddtrace/contrib/redis/patcher'
		require 'ddtrace/contrib/http/patcher'

[grape] add Grape integration for API endpoints #117

[grape] add Grape integration for API endpoints #117

Conversation

palazzem commented Apr 21, 2017 • edited Loading

What it does

What's missing

ufoot 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

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

Choose a reason for hiding this comment

ufoot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palazzem commented Apr 21, 2017 •

edited

Loading