fix:Sinatra nested modular apps possibly leaking spans

[marcotc]

This PR works towards closing #1035
Follow up to #1015

This PR changes how sinatra.request spans are created: in #1015 we introduced a backwards-compatible implementation of sinatra.request to support nested modular Sinatra apps. This implementation was brittle, as it started the span in one location and finished it in another, somewhat unrelated location. This was done in order to maintain maximum backwards compatibility, but it caused the code to have unexpected issues, specially around Sinatra middleware that created spans in between the initialization and closure of sinatra.request spans.

This PR now simplifies the instrumentation of sinatra.request, by reverting it to a much closer approach to the original one, while compromising on having a breaking behaviour on a few specific cases:

• Classic Sinatra applications are not affected.
• Modular Sinatra applications that do not make use of nested Sinatra applications are not affected.
• Modular Sinatra applications that do make use of nested Sinatra applications will have their sinatra.request spans changed in this PR. This should represent the minority of Sinatra users at this moment.

Because sinatra.request is representing a Sinatra application, and nested Sinatra apps represent their own applications, a host application will have many instances of sinatra.request for a single rack.request, which is already how ddtrace works. The change introduced in this PR is regarding Sinatra applications that have not been responsible for handling the request at hand, which will happen when nested applications are used. Because not all information is available for the application not responsible for the request, its sinatra.request span will be more bear, similar to know rack.request is not as rich as sinatra.request.

The sinatra.request for the Sinatra application not been responsible for handling the request at hand is still populated as well as possible with the information available, but it can, in some cases (wildcard requests, where /get/1 would become /get/* after matched, is an example where both sinatra.request spans will mismatch), not have the same resource path as the Sinatra application responsible for handling the request.

In the future, as one can see by the documented blocks left in code, we should introduce a more widespread breaking change to Sinatra, to bring it in line with Rack and Rails integrations regarding non-matching requests (e.g. 404). This PR would greatly benefit from introducing such change, but we'd like to introduce breaking behaviour in a very controlled manner, so we are only introducing the behaviour change specified above.

Example of instrumentation changes

Only nested modular applications are affected. The following screenshots have been captured with the following setup:

Sinatra application code

require 'sinatra/base'

require 'dogstatsd'
require 'ddtrace'

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

class NestedApp < Sinatra::Base
  register Datadog::Contrib::Sinatra::Tracer

  get '/nested' do
    'Nested!'
  end
end

class RootApp < Sinatra::Base
  register Datadog::Contrib::Sinatra::Tracer

  use NestedApp

  get '/' do
    'Root!'
  end
end


app = Rack::Builder.new do
  use Datadog::Contrib::Rack::TraceMiddleware
  run RootApp
end

Rack::Handler::WEBrick.run app, Port: 4567

Description

The bright highlighter colour (unlucky random assignment) is the root rack span. Purple spans are all sinatra spans. When there's 2 sinatra.request spans for the same request, the innermost one (closest to the bottom) is the NestedApp span, while the outermost (closest to the top) is the RootApp. sinatra.route is the matched application route: this span captures the user application code executed when a route is processed. In our case, it's only returning a few string constants, thus these spans are very short.

Because of the way the application is mounted (RootApp mounts NestedApp, then after mounts its own route '/), when a request is performed, the first routes to be verified are the NestedAppones, and then theRootApproutes are verified. This means that theNestedAppis always processed (and thus traced), even for requests that matchRootApp` routes. This is the correct representation of how nested Sinatra applications work: the overhead of mounting multiple nested apps is just now properly traced and captured by ddtrace.

This also means that for requests that match RootApp routes, NestedApp is traced by doesn't actually handle the request, being an effective "route not found" for NestedApp. This is tricky to represent, as it is not actually a 404 until the very end, as other Sinatra apps mounted in the same Rack stack might still handle the request. To maintain maximum compatibility with the current implementation, we populate the fields of NestedApp when the route matched belongs to RootApp, whenever this is possible.

Before (ddtrace 0.39.0)

GET /

GET /nested

GET /no_route

After (changes introduced in this PR)

You can see the introduction of a second, nested sinatra.request. This is to show the true processing pipeline that occurs on a Sinatra nested application. We tried to keep these intermediate spans hidden before, for compatibility reasons, but displaying the true behaviour that the application performs actually provides better visibility over the overhead involved in such applications and allowed us to have a more robust implementation.

GET /

GET /nested

GET /no_route

[ericmustin]

@marcotc this generally seems fine, the tests are hanging for some reason though. Happy to approve this after the specs pass.

The idea is this PR is meant to both resolved the missing/unfinised span issue and also prepare the instrumentation for a later change to the resource naming?

It might be good to have screenshots here demonstrating before/after for future reference, fwiw.

[marcotc]

@ericmustin I've updated the PR description with examples and screenshot of the changes, let me know you think.

[ericmustin]

👍