Fix registration with modular sinatra applications

[NobodysNightmare]

Problem to be solved

This (very reduced) example application will not generate any monitoring:

Datadog.configure do |c|
  c.use :sinatra
end

class MyApp < Sinatra::Base
  get 'hello_world' do
    [200, {}, ['Hello World!']]
  end
end

Solution

According to Sinatra documentation Sinatra.register will only affect "classic" style sinatra applications (the ones without a class, that you define on the top-level).

To make datadog available for all kinds of sinatra applications (e.g. also modular style apps inheriting from Sinatra::Base) automatically, the Tracer needs to be registered on Sinatra::Base, which should still leave it available on "classic" style applications, since those inherit from Sinatra::Base.

See: http://sinatrarb.com/extensions.html

Note/Disclaimer

Reading the sinatra documentation it is arguable, that we should not automatically register datadog with Sinatra::Base, but leave it to the user to manually register it with all subclasses.

However, the ddtrace docs don't mention that I need to register something manually, so I assume it is intended to be automatic. Also the user is kind of "opting in" to using Datadog for sinatra by calling c.use :sinatra. I don't think there is a use case for only including ddtrace in one of two modular applications in the same code base...

[NobodysNightmare]

Hmmm those CI failures are interesting... sadly I did not yet get the specs running on my local system, but at least the failures look very much like a problem that I am currently investigating for our applications: No path gets reported.

Investigation of that problem is the initial reason for me to open this pull request, because I looked at all the things that might be odd about the way we include datadog...

[delner]

Gotcha, this is a good consideration to expand support for Sinatra. Looking forward to seeing more from this!

[NobodysNightmare]

Just some feedback because of the long silence: I am still investigating those CI failures.

The good thing: Apparently by proposing my fix, I just triggered the exact problem that we had when I started investigating ddtrace: No request details are reported, I just know that "sinatra received requests", but for example I see no path.

The bad thing: Something is apparently broken for modular sinatra apps that worked before by including the tracer in the modular application. I have to dig deeper.

[NobodysNightmare]

I am wondering: What is the failing spec (sinatra/multi_app_spec.rb) supposed to test?

Apparently it is about applications that contain multiple sinatra applications? However: You would normally not inherit from Sinatra::Application (as the spec does), since Sinatra::Application is the base class for classic style sinatra apps. This means apps that you immediately run as a script:

# my_app.rb
get '/hello_world' do
  # response goes here
end

There is no supported way to have multiple of these and you should not inherit from it: https://www.rubydoc.info/gems/sinatra/Sinatra/Application

If you want to have multiple sinatra applications, you would inherit from Sinatra::Base. I think if the spec did that it would (correctly) fail for not only my changes, but also for the master branch.

I will propose that change as part of this pull request.

[ZimbiX]

@NobodysNightmare The missing route problem you mentioned sounds similar to: https://github.com/DataDog/dd-trace-rb/issues/913

[genebean]

I am interested in this too as I found it odd, based on the docs, that I had tor add the register in my code.

[marcotc]

👋, I wanted to let everyone know that we have been working on Sinatra improvements for the last few releases, and automatic registration of the Datadog middleware is in our roadmap. We focused on the correct support of modular applications (specially nested modular applications) at first, but we are wrapping that work up and will shift towards improving onboarding for Sinatra.

I tried the solution in this PR against our current master and it's a little bit more complicated to have it working correctly, considering all Sinatra mounting combinations, plus ensuring there's no double instrumentation for clients that already have the ddtrace middleware explicitly mounted today.

I'll be sure to post an update here when we have further developments regarding this specific issue.