Roda Integration

[wantsui]

What does this PR do?

Continues on the great work started by @randy-girard in #718 by working on a Roda Integration that works with dd-trace-rb 1.x .

Code to trigger automatic instrumentation

require "roda"
require "ddtrace"
class SampleApp < Roda
  use Datadog::Tracing::Contrib::Rack::TraceMiddleware
  Datadog.configure do |c|
    c.tracing.instrument :roda, **options
  end
  route do |r|
    r.root do
      r.get do
        'Hello World!'
      end
    end
  end
end

Available Configuration Options

• enabled
• service_name

Example trace:

Motivation

Add support for Roda

Additional Notes

Addressed Notes:

1. About error statuses:

• While testing this, it was found that if an application leads to a 500 internal server error due to a thrown error, Roda doesn't have access to the status code. To account for this, I updated this to default to a 500 status code.

2. What version(s) should be compatible?

• To be safe, support is up until 3.x. Whenever 4.x comes out, this integration will need to be revisited.

3. About Rack:
   It is highly recommended to load in Rack through use Datadog::Tracing::Contrib::Rack::TraceMiddleware because Rack handles the context propagation for distributed tracing.

How to test the change?

See Checks ran.

[TonyCTHsu]

Can move the error handling logic to the caller

          def call
            instrument(Ext::SPAN_REQUEST) do |span|
              super
            rescue => e
              # ... Handle error
            end
          end

So you could be confident yielding in the block without nested begin/rescue.

response = yield span

[wantsui]

It looks like in the Roda code, dispatch can land on one of these methods (seen in this conditional)

• call
• _roda_handle_main_route

Would I need to duplicate the rescue block and handle the error in both since either one of these can handle the dispatch?

[wantsui]

As an update to this, I wasn't able to move the error handling logic anywhere else because it interfered with the errors produced by the underlying app.

[marcotc]

Looks pretty good! 👍

[marcotc]

I update this branch. The errors now look like regular PR test errors.

[delner]

Looking clean, well organized as I'd expect a good integration to be!

Some minor tweaks on edge cases involving errors/versions worth examining. Please see comments for more details.

Where relevant or useful, I would suggest also adding code comment documentation as it might help an unfamiliar developer wrap their head around instrumentation behavior.

[delner]

Generally recommend classifying what kind of error is caught: StandardError if you don't want to trap kill signals, Exception if you want all signals (have be to very careful when catching Exception). StandardError is likely correct here.

To clarify, the purpose here is to add additional tags in case of an error? I think it might be better to provide an on_error argument to the #trace call, as it will handle the exception and what not for you. this way you can eliminate the begin/rescue in the ensure. Bonus is that it will handle Exception for you.

[marcotc]

I paired with Wan on this one as well, and the on_error would not fit well for this case because we need the local closure variable request_method in the error handler.

If we extract the error handling to on_error: method(:handle_error), then def handle_error does not have access to request_method.
Alternatively, we could declare it inlince on_error: ->{...} but the method is pretty large.

I think we can keep it as in this file, as the alternatives are less legible.

[marcotc]

I paired with Wan on this one as well, and the on_error would not fit well for this case because we need the local closure variable request_method in the error handler.

If we extract the error handling to on_error: method(:handle_error), then def handle_error does not have access to request_method.
Alternatively, we could declare it inlince on_error: ->{...} but the method is pretty large.

I think we can keep it as in this file, as the alternatives are less legible.

[delner]

Possibly strange edge case here where request.request_method.to_s.upcase raised an error, hit the ensure block, request raised an error, caught by this rescue block, then request_method will likely be undefined, possibly raising another error.

[wantsui]

I'm not sure how this error can be thrown because all routes should be defined with a type from the start. Do you have a suggestion for handling this scenario?

[marcotc]

I paired with Wan and it's not possible for request.request_method.to_s.upcase to fail, as it comes straight from Rack and it's part of the Rack contract. This is the same behaviour of our other Rack-ralated integrations (Sinatra, Rails, Hanami, Grace)

[delner]

Good to know this maximum. Do we know we don't support Roda 4.0.0? If so, why don't we?

Also "maximum" version as a name is slightly misleading in that its exclusive (we don't actually support 4.0.0.)

[wantsui]

At the time of this comment: Roda 4.x isn't out. The latest version is 3.65.0.

Given the changes that occurred between 1.x and 2.x, I'm just being cautious by setting a maximum of 4.x for now. That way, in our public docs, we can be clear that this logic is intended for 2.x and 3.x.

Whenever 4.x comes out, it'll require visiting to make sure it still functions as expected.

[delner]

If there's an upper limit on versions we support, we should add that here as well.

[wantsui]

Great catch! I was being cautious by hardcoding a limit of 4.x, so I should update this Getting Started doc too!

[wantsui]

In the latest commit, this now gives a range ending <4 (since it works with 3.x):

| Roda                       | `roda`                     | `>= 2.1, <4`             | `>= 2.1, <4`              | *[Link](#roda)*                     | *[Link](https://github.com/jeremyevans/roda)*   

[marcotc]

We should remove this top-level call and instead, due to lack of a good way to automatically instrument Roda+Rack, we should add this manual step to the readme.

[marcotc]

Thank you so much, @wantsui! 🙇