Squash nested sinatra/rack request span

docs/GettingStarted.md

@@ -1936,7 +1936,7 @@ end
 
 The Sinatra integration traces requests and template rendering.
 
-To start using the tracing client, make sure you import `ddtrace` and `use :sinatra` after either `sinatra` or `sinatra/base`, and before you define your application/routes:
+To start using the tracing client, make sure you import `ddtrace` and `instrument :sinatra` after either `sinatra` or `sinatra/base`, and before you define your application/routes:
 
 #### Classic application
 
@@ -1964,16 +1964,12 @@ Datadog.configure do |c|
 end
 
 class NestedApp < Sinatra::Base
-  register Datadog::Tracing::Contrib::Sinatra::Tracer
-
   get '/nested' do
     'Hello from nested app!'
   end
 end
 
 class App < Sinatra::Base
-  register Datadog::Tracing::Contrib::Sinatra::Tracer
-
   use NestedApp
 
   get '/' do
@@ -1982,8 +1978,6 @@ class App < Sinatra::Base
 end
 ```
 
-Ensure you register `Datadog::Tracing::Contrib::Sinatra::Tracer` as a middleware before you mount your nested applications.
-
 #### Instrumentation options
 
 `options` are the following keyword arguments:

integration/apps/sinatra2-classic/app/acme.rb

@@ -1,6 +1,6 @@
 require 'sinatra'
 require 'ddtrace'
-
+require 'pry'
 Datadog.configure do |c|
   c.service = 'acme-sinatra2-classic'
   c.diagnostics.debug = true if Datadog::DemoEnv.feature?('debug')

integration/apps/sinatra2-classic/docker-compose.yml

@@ -25,8 +25,11 @@ services:
       # - DD_DEMO_ENV_GEM_REF_DDTRACE=f233336994315bfa04dac581387a8152bab8b85a
       # Enable building the profiling native extension
       - DD_DEMO_ENV_BUILD_PROFILING_EXTENSION=true
+      # - DD_TRACE_DEBUG=true
     expose:
       - "80"
+    # ports:
+    #   - "80:80"
     stdin_open: true
     tty: true
     volumes:

integration/apps/sinatra2-modular/app/acme.rb

@@ -1,6 +1,7 @@
 require 'sinatra/base'
 require 'sinatra/router'
 require 'ddtrace'
+require 'pry'
 # require 'ddtrace/auto_instrument'
 
 Datadog.configure do |c|
@@ -28,8 +29,6 @@
 require_relative './health'
 
 class Acme < Sinatra::Base
-  # register Datadog::Tracing::Contrib::Sinatra::Tracer
-
   # # Use Sinatra App as middleware
   # use Health
   # use Basic

integration/apps/sinatra2-modular/app/basic.rb

@@ -1,9 +1,9 @@
 require 'sinatra/base'
 require 'ddtrace'
+require_relative 'parent'
 
-class Basic < Sinatra::Base
-  # register Datadog::Tracing::Contrib::Sinatra::Tracer
-
+# Inherit from another app to verify middleware/extension inheritance
+class Basic < Parent
   get '/basic/default' do
     200
   end

integration/apps/sinatra2-modular/app/parent.rb

@@ -0,0 +1,5 @@
+require 'sinatra/base'
+require 'ddtrace'
+
+class Parent < Sinatra::Base
+end

integration/apps/sinatra2-modular/config.ru

@@ -1,6 +1,4 @@
 require 'ddtrace'
 require_relative 'app/acme'
 
-use Datadog::Tracing::Contrib::Rack::TraceMiddleware
-
 run Acme

integration/apps/sinatra2-modular/docker-compose.yml

@@ -24,9 +24,12 @@ services:
       # - DD_DEMO_ENV_GEM_GIT_DDTRACE=https://github.com/DataDog/dd-trace-rb.git
       # - DD_DEMO_ENV_GEM_REF_DDTRACE=f233336994315bfa04dac581387a8152bab8b85a
       # Enable building the profiling native extension
-      # - DD_DEMO_ENV_BUILD_PROFILING_EXTENSION=true
+      - DD_DEMO_ENV_BUILD_PROFILING_EXTENSION=true
+      # - DD_TRACE_DEBUG=true
     expose:
       - "80"
+    # ports:
+    #   - "80:80"
     stdin_open: true
     tty: true
     volumes:

lib/datadog/tracing/contrib/rack/middlewares.rb

@@ -56,16 +56,18 @@ def call(env)
             # Find out if this is rack within rack
             previous_request_span = env[Ext::RACK_ENV_REQUEST_SPAN]
 
+            return @app.call(env) if previous_request_span
+
             # Extract distributed tracing context before creating any spans,
             # so that all spans will be added to the distributed trace.
-            if configuration[:distributed_tracing] && previous_request_span.nil?
+            if configuration[:distributed_tracing]
               trace_digest = Tracing::Propagation::HTTP.extract(env)
               Tracing.continue_trace!(trace_digest)
             end
 
             # Create a root Span to keep track of frontend web servers
             # (i.e. Apache, nginx) if the header is properly set
-            frontend_span = compute_queue_time(env) if previous_request_span.nil?
+            frontend_span = compute_queue_time(env)
 
             trace_options = { span_type: Tracing::Metadata::Ext::HTTP::TYPE_INBOUND }
             trace_options[:service] = configuration[:service_name] if configuration[:service_name]

lib/datadog/tracing/contrib/sinatra/env.rb

@@ -13,14 +13,12 @@ module Sinatra
         module Env
           module_function
 
-          def datadog_span(env, app)
-            request_span = env[Ext::RACK_ENV_REQUEST_SPAN]
-            request_span && request_span[app]
+          def datadog_span(env)
+            env[Ext::RACK_ENV_SINATRA_REQUEST_SPAN]
           end
 
-          def set_datadog_span(env, app, span)
-            hash = (env[Ext::RACK_ENV_REQUEST_SPAN] ||= {})
-            hash[app] = span
+          def set_datadog_span(env, span)
+            env[Ext::RACK_ENV_SINATRA_REQUEST_SPAN] = span
           end
 
           def request_header_tags(env, headers)
@@ -40,24 +38,15 @@ def header_to_rack_header(name)
             "HTTP_#{name.to_s.upcase.gsub(/[-\s]/, '_')}"
           end
 
-          # Was a Sinatra already traced in this request?
-          # We don't want to create spans for intermediate Sinatra
-          # middlewares that don't match the request at hand.
-          def middleware_traced?(env)
-            env[Ext::RACK_ENV_MIDDLEWARE_TRACED]
-          end
-
-          def set_middleware_traced(env, bool)
-            env[Ext::RACK_ENV_MIDDLEWARE_TRACED] = bool
-          end
+          def route_path(env, use_script_names: Datadog.configuration.tracing[:sinatra][:resource_script_names])
+            return unless env['sinatra.route']
 
-          # The start time of the top-most Sinatra middleware.
-          def middleware_start_time(env)
-            env[Ext::RACK_ENV_MIDDLEWARE_START_TIME]
-          end
-
-          def set_middleware_start_time(env, time = Time.now.utc)
-            env[Ext::RACK_ENV_MIDDLEWARE_START_TIME] = time
+            _, path = env['sinatra.route'].split(' ', 2)
+            if use_script_names
+              env['SCRIPT_NAME'].to_s + path
+            else
+              path
+            end
           end
         end
       end

lib/datadog/tracing/contrib/sinatra/ext.rb

@@ -10,9 +10,7 @@ module Ext
           ENV_ENABLED = 'DD_TRACE_SINATRA_ENABLED'.freeze
           ENV_ANALYTICS_ENABLED = 'DD_TRACE_SINATRA_ANALYTICS_ENABLED'.freeze
           ENV_ANALYTICS_SAMPLE_RATE = 'DD_TRACE_SINATRA_ANALYTICS_SAMPLE_RATE'.freeze
-          RACK_ENV_REQUEST_SPAN = 'datadog.sinatra_request_span'.freeze
-          RACK_ENV_MIDDLEWARE_START_TIME = 'datadog.sinatra_middleware_start_time'.freeze
-          RACK_ENV_MIDDLEWARE_TRACED = 'datadog.sinatra_middleware_traced'.freeze
+          RACK_ENV_SINATRA_REQUEST_SPAN = 'datadog.sinatra_request_span'.freeze
           SPAN_RENDER_TEMPLATE = 'sinatra.render_template'.freeze
           SPAN_REQUEST = 'sinatra.request'.freeze
           SPAN_ROUTE = 'sinatra.route'.freeze
@@ -25,6 +23,12 @@ module Ext
           TAG_SCRIPT_NAME = 'sinatra.script_name'.freeze
           TAG_TEMPLATE_ENGINE = 'sinatra.template_engine'.freeze
           TAG_TEMPLATE_NAME = 'sinatra.template_name'.freeze
+
+          # === Deprecated: To be removed ===
+          RACK_ENV_REQUEST_SPAN = 'datadog.sinatra_request_span'.freeze
+          RACK_ENV_MIDDLEWARE_START_TIME = 'datadog.sinatra_middleware_start_time'.freeze
+          RACK_ENV_MIDDLEWARE_TRACED = 'datadog.sinatra_middleware_traced'.freeze
+          # === Deprecated: To be removed ===
         end
       end
     end

lib/datadog/tracing/contrib/sinatra/patcher.rb

@@ -1,4 +1,4 @@
-# typed: true
+# typed: false
 
 require_relative '../../../core/utils/only_once'
 require_relative '../patcher'
@@ -58,7 +58,7 @@ def patch
           end
 
           def register_tracer
-            ::Sinatra.send(:register, Contrib::Sinatra::Tracer)
+            ::Sinatra::Base.register(Contrib::Sinatra::Tracer)
             ::Sinatra::Base.prepend(Sinatra::Tracer::Base)
           end
 

lib/datadog/tracing/contrib/sinatra/tracer.rb

@@ -17,69 +17,12 @@ module Sinatra
         # Datadog::Tracing::Contrib::Sinatra::Tracer is a Sinatra extension which traces
         # requests.
         module Tracer
-          def route(verb, action, *)
-            # Keep track of the route name when the app is instantiated for an
-            # incoming request.
-            condition do
-              # If the option to prepend script names is enabled, then
-              # prepend the script name from the request onto the action.
-              #
-              # DEV: env['sinatra.route'] already exists with very similar information,
-              # DEV: but doesn't account for our `resource_script_names` logic.
-              #
-              @datadog_route = if Datadog.configuration.tracing[:sinatra][:resource_script_names]
-                                 "#{request.script_name}#{action}"
-                               else
-                                 action
-                               end
-            end
-
-            super
-          end
-
           def self.registered(app)
             app.use TracerMiddleware, app_instance: app
-
-            app.after do
-              next unless Tracing.enabled?
-
-              span = Sinatra::Env.datadog_span(env, app)
-
-              # TODO: `route` should *only* be populated if @datadog_route is defined.
-              # TODO: If @datadog_route is not defined, then this Sinatra app is not responsible
-              # TODO: for handling this request.
-              # TODO:
-              # TODO: This change would be BREAKING for any Sinatra app (classic or modular),
-              # TODO: as it affects the `resource` value for requests not handled by the Sinatra app.
-              # TODO: Currently we use "#{method} #{path}" in such aces, but `path` is the raw,
-              # TODO: high-cardinality HTTP path, and can contain PII.
-              # TODO:
-              # TODO: The value we should use as the `resource` when the Sinatra app is not
-              # TODO: responsible for the request is a tricky subject.
-              # TODO: The best option is a value that clearly communicates that this app did not
-              # TODO: handle this request. It's important to keep in mind that an unhandled request
-              # TODO: by this Sinatra app might still be handled by another Rack middleware (which can
-              # TODO: be a Sinatra app itself) or it might just 404 if not handled at all.
-              # TODO:
-              # TODO: A possible value for `resource` could set a high level description, e.g.
-              # TODO: `request.request_method`, given we don't have the response object available yet.
-              route = if defined?(@datadog_route)
-                        @datadog_route
-                      else
-                        # Fallback in case no routes have matched
-                        request.path
-                      end
-
-              span.resource = "#{request.request_method} #{route}"
-              span.set_tag(Ext::TAG_ROUTE_PATH, route)
-            end
           end
 
           # Method overrides for Sinatra::Base
           module Base
-            MISSING_REQUEST_SPAN_ONLY_ONCE = Core::Utils::OnlyOnce.new
-            private_constant :MISSING_REQUEST_SPAN_ONLY_ONCE
-
             def render(engine, data, *)
               return super unless Tracing.enabled?
 
@@ -102,19 +45,21 @@ def render(engine, data, *)
 
             # Invoked when a matching route is found.
             # This method yields directly to user code.
-            # rubocop:disable Metrics/MethodLength
             def route_eval
               configuration = Datadog.configuration.tracing[:sinatra]
               return super unless Tracing.enabled?
 
+              datadog_route = Sinatra::Env.route_path(env)
+
               Tracing.trace(
                 Ext::SPAN_ROUTE,
                 service: configuration[:service_name],
                 span_type: Tracing::Metadata::Ext::HTTP::TYPE_INBOUND,
-                resource: "#{request.request_method} #{@datadog_route}",
+                resource: "#{request.request_method} #{datadog_route}",
               ) do |span, trace|
                 span.set_tag(Ext::TAG_APP_NAME, settings.name || settings.superclass.name)
-                span.set_tag(Ext::TAG_ROUTE_PATH, @datadog_route)
+                span.set_tag(Ext::TAG_ROUTE_PATH, datadog_route)
+
                 if request.script_name && !request.script_name.empty?
                   span.set_tag(Ext::TAG_SCRIPT_NAME, request.script_name)
                 end
@@ -124,32 +69,15 @@ def route_eval
 
                 trace.resource = span.resource
 
-                sinatra_request_span =
-                  if self.class <= ::Sinatra::Application # Classic style (top-level) application
-                    Sinatra::Env.datadog_span(env, ::Sinatra::Application)
-                  else
-                    Sinatra::Env.datadog_span(env, self.class)
-                  end
-                if sinatra_request_span
-                  sinatra_request_span.resource = span.resource
-                else
-                  MISSING_REQUEST_SPAN_ONLY_ONCE.run do
-                    Datadog.logger.warn do
-                      'Sinatra integration is misconfigured, reported traces will be missing request metadata ' \
-                      'such as path and HTTP status code. ' \
-                      'Did you forget to add `register Datadog::Tracing::Contrib::Sinatra::Tracer` to your ' \
-                      '`Sinatra::Base` subclass? ' \
-                      'See <https://docs.datadoghq.com/tracing/setup_overview/setup/ruby/#sinatra> for more details.'
-                    end
-                  end
-                end
+                sinatra_request_span = Sinatra::Env.datadog_span(env)
+
+                sinatra_request_span.resource = span.resource
 
                 Contrib::Analytics.set_measured(span)
 
                 super
               end
             end
-            # rubocop:enable Metrics/MethodLength
           end
         end
       end