[grape] add Grape integration for API endpoints #117
Conversation
| return if Thread.current[KEY_RUN] | ||
|
|
||
| # retrieve the tracer from the PIN object | ||
| pin = Datadog::Pin.get_from(::Grape) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yup, not worth the complexity of attaching the PIN to instances, I'm just saying -> let's keep aware we have this limitation.
There was a problem hiding this comment.
Yea, considering that the standard case is having one Grape application with namespaces. We still not cover some corner case by the way.
| # 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 |
There was a problem hiding this comment.
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.
| span.start_time = start | ||
| span.set_tag('grape.route.endpoint', api_view) | ||
| span.set_tag('grape.route.path', path) | ||
| span.finish_at(finish) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Definitely! forgot to add this enforcing, thanks!
| span.set_error(payload[:exception_object]) unless payload[:exception_object].nil? | ||
|
|
||
| span.set_tag('grape.filter.type', type.to_s) | ||
| span.start_time = start |
lib/ddtrace/contrib/grape/patcher.rb
Outdated
| module_function | ||
|
|
||
| def patch | ||
| if !@patched && defined?(::Grape) |
There was a problem hiding this comment.
Thanks for using that patcher pattern, 👍
lib/ddtrace/contrib/grape/patcher.rb
Outdated
| end | ||
|
|
||
| def unpatch | ||
| # TODO: implement this (revert aliasing) |
| require 'ddtrace/contrib/redis/patcher' | ||
| require 'ddtrace/contrib/http/patcher' |
There was a problem hiding this comment.
Was there an issue with the order? (just curious, I'm not requiring any change here)
There was a problem hiding this comment.
oh no, it's just cosmetic :D
test/contrib/grape/app.rb
Outdated
| sleep(0.01) | ||
| end | ||
|
|
||
| desc 'Returns an error' |
There was a problem hiding this comment.
Is that really the right comment?
test/monkey_test.rb
Outdated
| 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()) |
There was a problem hiding this comment.
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.
palazzem
force-pushed
the
palazzem/rails-grape
branch
from
28b7d69
to
fba59e6
Compare
… Rack instrumentation
…hod is available at definition time
palazzem
force-pushed
the
palazzem/rails-grape
branch
from
65eb477
to
45dc54b
Compare
What it does
Add Grape auto instrumentation using the
Patchermethod. Grape classes are patched so that new instrumentation is available before the task is executed. It requiresActiveSupportand relies on Grape signals to:beforeandafterfiltersEndpointbodyThis 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