[rest_client] Integration

[pawelchcki]

Add integration for rest_client

[delner]

Good start to this.

Some questions about tests, Pins, compatibility, and simplifying the trace code a bit (if possible.)

Also, there wasn't any place to add a comment, but we need to add this to the documentation as well.

This also needs to be rebased to address merge conflicts.

[delner]

We added the Rake task here, but we aren't actually running rest_client tests in CI. We should add calls to this task in the ci task below.

[pawelchcki]

Oh! Forgot about that. Thanks for pointing it out!

[delner]

Interesting... why put the Pin on the class, when it could be on the object?

If we put it on the object, would it make it possible for users to configure an individual request to be traced differently? e.g. with a different service_name? Usually this kind of feature is in great demand, so we might want to make sure it's supported.

[delner]

Follow-up to this: I see you defined #datadog_pin in the instance methods for ::RestClient::Request. The pin on the instance should support requests to be individually configured. But why is this pin then added to the class? Is this not redundant?

[delner]

What version of Ruby is supported? prepend is not available in 1.9.3, and this will fail for 2.0 because it's a call to a private method. If we plan on supporting 2.0+, we should change this to send(:prepend, InstanceMethods).

We should also add some kind of compatibility check before we reach this point, presumably in the patcher class.

[pawelchcki]

Added the shim to support ruby 1.9

[delner]

Why do we use this asynchronous form instead of block form? The block form should cover most of what you're doing here implicitly, and permit you to do the customizations you require.

[pawelchcki]

Indeed block form would have been nicer, however the problem is with

rescue ::RestClient::ExceptionWithResponse => e
...
raise e

Since in block form this will be traced as error regardless of the http code.
I.e. RestClient throws this exception on every http code other than 1xx-2xx

[delner]

Could you rescue that error inside the block form, the set the error or re-raise if necessary? We do something like this in Rails. It's not exactly the same, but maybe there's some inspiration to draw from it.

[pawelchcki]

I need to re-raise this error not to break any one use of rest-client.
We could modify tracer.trace do ... end to allow it to somehow skip span.set_error(e) step.

But so far I don't have a good idea how to approach this.

[delner]

Right, that makes sense. Maybe we should consider that as a new feature to add in the future: some kind of configurable error handling block. Something like:

class MyHttpClient
  def get(url)
    Datadog.tracer.trace('my.request', on_error: method(:handle_http_error)) do |span|
      # Do some work, raise an HttpResponseError
    end
  end

  def handle_http_error(span, error)
    # Set errors is you wish, or do any custom logging, etc...
    span.set_error(error) if Ext::HTTP::ERROR_RANGE.cover?(error.http_code)
  end
end

We can consider that another time though. In this PR, maybe let's stick to what you have.

[delner]

This is thoughtful. 👍

[delner]

Is WebMock necessary? Surely we don't want it to make any outgoing HTTP requests, but is there some mock we can put in at the RSpec level to negate the need to depend on this? If not, it's okay, and we can keep it: just wondering if there was a way of keeping this simpler.

[pawelchcki]

We would have to mock quite a few internals in rest_client to do so.
Webmock allows us to treat those as a black box, and write more integration style tests.

[delner]

Hmmm, I prefer unit tests where we're clear about the inputs/outputs (makes it easier to break things apart), however, I can see the value of an integration test here. This is probably okay.

[delner]

Minor non-blocking, but short, single line blocks like this could be reduced to the more compact before { request } form.

[delner]

I like the use of shared examples. 👍

[delner]

You could bump this span = to a lazy let(:span) block, which would not be evaluated until you reference it, allowing you to reduce repetition.

[pawelchcki]

done

[delner]

Minor non-blocking style thing, but this could benefit by breaking into multiple lines e.g.:

Datadog::Pin.new(
  #...
)

[delner]

There's a possible memory issue in here regarding Pins, and we might want to change our strategy accordingly.

[delner]

Thinking harder on this, initializing a Datadog::Pin for every request might be a bad idea, since it might end up allocating memory that gets tossed out anyways, but at scale could be problematic. That considered, we do need to allow a Pin to be added and configured, such that a user could override the behavior for a specific request, if they so chose.

I think maybe we need to change the strategy a little here: above where we use these configuration values from thePin, I think we should decompose and encapsulate each of those options, to use the Pin if it exists, or otherwise to fallback to the global configuration (only if the Pin does not exist, not if the Pin is simply missing a value.)

This might look something like:

def datadog_trace_request
  datadog_configuration(:tracer).trace(REQUEST_TRACE_NAME) do
    # Do some work...
  end
end

def datadog_configuration(key)
   respond_to?(:datadog_pin) ? datadog_pin.send(key) : Datadog.configuration[:rest_client][key]
end

# Do not define #datadog_pin.
# If user does `Pin.new(service_name: 'custom-service').onto(request)`
# then the pin will become available. Otherwise we don't want to create a Pin.
# `Datadog.configure(request, service_name: 'custom-service')` will probably not work,
# because it will never add a pin to an object: only configure an existing one.
# See https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/pin.rb
#     https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/configuration/pin_setup.rb
#     https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace.rb
# def datadog_pin
#   nil
# end

[pawelchcki]

Lets work on this optimization in separate PR's as there are a few approaches around Pin Like PinSetup and Datadog.configure and it would be good to unify them in the future.

[delner]

You're right, we probably shouldn't modify the Pin or PinSetup classes in this PR. However, I don't think creating pins here, even temporarily, is a satisfactory solution, given the possible performance impact.

So I would suggest we drop pin support for now altogether, and use global configuration only instead, until we can get the groundwork in for a satisfactory multiplexing solution in place: either by having optimized the use of Pins, or by leveraging the new integration configuration to drive multiplexing instead.

[delner]

We should add an example of how to reconfigure a Request to be traced differently, using its own settings instead of global ones. This implementation will depend on how our configuration strategy works for this integration. See my other comment for why that might still change.

[delner]

Also, not here, but we should add test examples that demonstrate such configuration works independently of global configuration, without conflict.

[pawelchcki]

@delner This PR is ready for another batch of reviews

[delner]

These changes look good. Thanks for you hard work on this!

Might I suggest either rebasing or squashing these commits though as they merge? There are 41 commits, many of which are quite small, which pollutes commit history a bit. Would be nice to clean these up a bit.