Change http proxy span pattern

docs/GettingStarted.md

@@ -2308,7 +2308,7 @@ end
 
 Traces that originate from HTTP requests can be configured to include the time spent in a frontend web server or load balancer queue before the request reaches the Ruby application.
 
-This feature is disabled by default. To activate it, you must add an `X-Request-Start` or `X-Queue-Start` header from your web server (i.e., Nginx). The following is an Nginx configuration example:
+This feature is disabled by default. To activate it, you must add an `X-Request-Start` or `X-Queue-Start` header from your web server (i.e., Nginx) before enabling the request queuing feature. The following is an Nginx configuration example:
 
 ```
 # /etc/nginx/conf.d/ruby_service.conf
@@ -2321,14 +2321,6 @@ server {
     }
 }
 ```
-
-Then you must enable the request queuing feature. The following options are available for the `:request_queuing` configuration:
-
-| Option             | Description |
-| ------------------ | ----------- |
-| `:include_request` | A `http_server.queue` span will be the root span of a trace, including the total time spent processing the request *in addition* to the time spent waiting for the request to begin being processed. This is the behavior when the configuration is set to `true`. This is the selected configuration when set to `true`. |
-| `:exclude_request` | A `http.proxy.request` span will be the root span of a trace, with the `http.proxy.queue` child span duration representing only the time spent waiting for the request to begin being processed. *This is an experimental feature!* |
-
 For Rack-based applications, see the [documentation](#rack) for details.
 
 ### Processing Pipeline

integration/apps/rails-seven/config/initializers/datadog.rb

@@ -11,7 +11,7 @@
   if Datadog::DemoEnv.feature?('tracing')
     c.tracing.analytics.enabled = true if Datadog::DemoEnv.feature?('analytics')
 
-    c.tracing.instrument :rails, request_queuing: :exclude_request
+    c.tracing.instrument :rails, request_queuing: true
     c.tracing.instrument :redis, service_name: 'acme-redis'
     c.tracing.instrument :resque
   end

lib/datadog/tracing/contrib/rack/configuration/settings.rb

@@ -39,6 +39,7 @@ class Settings < Contrib::Configuration::Settings
             option :middleware_names, default: false, type: :bool
             option :quantize, default: {}, type: :hash
             option :request_queuing do |o|
+              o.type :bool
               o.default false
             end
 

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

@@ -13,7 +13,6 @@ module Ext
           RACK_ENV_REQUEST_SPAN = 'datadog.rack_request_span'
           SPAN_HTTP_PROXY_REQUEST = 'http.proxy.request'
           SPAN_HTTP_PROXY_QUEUE = 'http.proxy.queue'
-          SPAN_HTTP_SERVER_QUEUE = 'http_server.queue'
           SPAN_REQUEST = 'rack.request'
           TAG_COMPONENT = 'rack'
           TAG_COMPONENT_HTTP_PROXY = 'http_proxy'

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

@@ -11,6 +11,7 @@
 require_relative 'header_collection'
 require_relative 'header_tagging'
 require_relative 'request_queue'
+require_relative 'trace_proxy_middleware'
 
 module Datadog
   module Tracing
@@ -28,43 +29,6 @@ def initialize(app)
             @app = app
           end
 
-          def compute_queue_time(env)
-            return unless configuration[:request_queuing]
-
-            # parse the request queue time
-            request_start = Contrib::Rack::QueueTime.get_request_start(env)
-            return if request_start.nil?
-
-            case configuration[:request_queuing]
-            when true, :include_request # DEV: Switch `true` to `:exclude_request` in v2.0
-              queue_span = trace_http_server(Ext::SPAN_HTTP_SERVER_QUEUE, start_time: request_start)
-              # Tag this span as belonging to Rack
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT)
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_HTTP_SERVER_QUEUE)
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_KIND, Tracing::Metadata::Ext::SpanKind::TAG_SERVER)
-              queue_span
-            when :exclude_request
-              request_span = trace_http_server(Ext::SPAN_HTTP_PROXY_REQUEST, start_time: request_start)
-              request_span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT_HTTP_PROXY)
-              request_span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_HTTP_PROXY_REQUEST)
-              request_span.set_tag(Tracing::Metadata::Ext::TAG_KIND, Tracing::Metadata::Ext::SpanKind::TAG_PROXY)
-
-              queue_span = trace_http_server(Ext::SPAN_HTTP_PROXY_QUEUE, start_time: request_start)
-
-              # Measure service stats
-              Contrib::Analytics.set_measured(queue_span)
-
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT_HTTP_PROXY)
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_HTTP_PROXY_QUEUE)
-              queue_span.set_tag(Tracing::Metadata::Ext::TAG_KIND, Tracing::Metadata::Ext::SpanKind::TAG_PROXY)
-              # finish the `queue` span now to record only the time spent *in queue*,
-              # excluding the time spent processing the request itself
-              queue_span.finish
-
-              request_span
-            end
-          end
-
           def call(env)
             # Find out if this is rack within rack
             previous_request_span = env[Ext::RACK_ENV_REQUEST_SPAN]
@@ -80,58 +44,55 @@ def call(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)
-
-            trace_options = { type: Tracing::Metadata::Ext::HTTP::TYPE_INBOUND }
-            trace_options[:service] = configuration[:service_name] if configuration[:service_name]
-
-            # start a new request span and attach it to the current Rack environment;
-            # we must ensure that the span `resource` is set later
-            request_span = Tracing.trace(Ext::SPAN_REQUEST, **trace_options)
-            request_span.resource = nil
-            request_trace = Tracing.active_trace
-            env[Ext::RACK_ENV_REQUEST_SPAN] = request_span
-
-            # Copy the original env, before the rest of the stack executes.
-            # Values may change; we want values before that happens.
-            original_env = env.dup
-
-            # call the rest of the stack
-            status, headers, response = @app.call(env)
-            [status, headers, response]
-
-          # rubocop:disable Lint/RescueException
-          # Here we really want to catch *any* exception, not only StandardError,
-          # as we really have no clue of what is in the block,
-          # and it is user code which should be executed no matter what.
-          # It's not a problem since we re-raise it afterwards so for example a
-          # SignalException::Interrupt would still bubble up.
-          rescue Exception => e
-            # catch exceptions that may be raised in the middleware chain
-            # Note: if a middleware catches an Exception without re raising,
-            # the Exception cannot be recorded here.
-            request_span.set_error(e) unless request_span.nil?
-            raise e
-          ensure
-            env[Ext::RACK_ENV_REQUEST_SPAN] = previous_request_span if previous_request_span
-
-            if request_span
-              # Rack is a really low level interface and it doesn't provide any
-              # advanced functionality like routers. Because of that, we assume that
-              # the underlying framework or application has more knowledge about
-              # the result for this request; `resource` and `tags` are expected to
-              # be set in another level but if they're missing, reasonable defaults
-              # are used.
-              set_request_tags!(request_trace, request_span, env, status, headers, response, original_env || env)
-
-              # ensure the request_span is finished and the context reset;
-              # this assumes that the Rack middleware creates a root span
-              request_span.finish
+            TraceProxyMiddleware.call(env, configuration) do
+              trace_options = { type: Tracing::Metadata::Ext::HTTP::TYPE_INBOUND }
+              trace_options[:service] = configuration[:service_name] if configuration[:service_name]
+
+              # start a new request span and attach it to the current Rack environment;
+              # we must ensure that the span `resource` is set later
+              request_span = Tracing.trace(Ext::SPAN_REQUEST, **trace_options)
+              request_span.resource = nil
+              request_trace = Tracing.active_trace
+              env[Ext::RACK_ENV_REQUEST_SPAN] = request_span
+
+              # Copy the original env, before the rest of the stack executes.
+              # Values may change; we want values before that happens.
+              original_env = env.dup
+
+              # call the rest of the stack
+              status, headers, response = @app.call(env)
+
+              [status, headers, response]
+
+            # rubocop:disable Lint/RescueException
+            # Here we really want to catch *any* exception, not only StandardError,
+            # as we really have no clue of what is in the block,
+            # and it is user code which should be executed no matter what.
+            # It's not a problem since we re-raise it afterwards so for example a
+            # SignalException::Interrupt would still bubble up.
+            rescue Exception => e
+              # catch exceptions that may be raised in the middleware chain
+              # Note: if a middleware catches an Exception without re raising,
+              # the Exception cannot be recorded here.
+              request_span.set_error(e) unless request_span.nil?
+              raise e
+            ensure
+              env[Ext::RACK_ENV_REQUEST_SPAN] = previous_request_span if previous_request_span
+
+              if request_span
+                # Rack is a really low level interface and it doesn't provide any
+                # advanced functionality like routers. Because of that, we assume that
+                # the underlying framework or application has more knowledge about
+                # the result for this request; `resource` and `tags` are expected to
+                # be set in another level but if they're missing, reasonable defaults
+                # are used.
+                set_request_tags!(request_trace, request_span, env, status, headers, response, original_env || env)
+
+                # ensure the request_span is finished and the context reset;
+                # this assumes that the Rack middleware creates a root span
+                request_span.finish
+              end
             end
-
-            frontend_span.finish if frontend_span
           end
           # rubocop:enable Lint/RescueException
 
@@ -247,15 +208,6 @@ def configuration
             Datadog.configuration.tracing[:rack]
           end
 
-          def trace_http_server(span_name, start_time:)
-            Tracing.trace(
-              span_name,
-              type: Tracing::Metadata::Ext::HTTP::TYPE_PROXY,
-              start_time: start_time,
-              service: configuration[:web_service_name]
-            )
-          end
-
           def parse_url(env, original_env)
             request_obj = ::Rack::Request.new(env)
 

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

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require_relative '../../metadata/ext'
+require_relative '../analytics'
+require_relative 'ext'
+require_relative 'request_queue'
+
+module Datadog
+  module Tracing
+    module Contrib
+      module Rack
+        # Module to create virtual proxy span
+        module TraceProxyMiddleware
+          module_function
+
+          def call(env, configuration)
+            return yield unless configuration[:request_queuing]
+
+            # parse the request queue time
+            start_time = Contrib::Rack::QueueTime.get_request_start(env)
+            return yield unless start_time
+
+            options = {
+              service: configuration[:web_service_name],
+              start_time: start_time,
+              type: Tracing::Metadata::Ext::HTTP::TYPE_PROXY
+            }
+
+            request_span = Tracing.trace(Ext::SPAN_HTTP_PROXY_REQUEST, **options)
+
+            request_span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT_HTTP_PROXY)
+            request_span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_HTTP_PROXY_REQUEST)
+            request_span.set_tag(Tracing::Metadata::Ext::TAG_KIND, Tracing::Metadata::Ext::SpanKind::TAG_PROXY)
+
+            queue_span = Tracing.trace(Ext::SPAN_HTTP_PROXY_QUEUE, **options)
+
+            queue_span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT_HTTP_PROXY)
+            queue_span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_HTTP_PROXY_QUEUE)
+            queue_span.set_tag(Tracing::Metadata::Ext::TAG_KIND, Tracing::Metadata::Ext::SpanKind::TAG_PROXY)
+
+            Contrib::Analytics.set_measured(queue_span)
+            # finish the `queue` span now to record only the time spent *in queue*,
+            # excluding the time spent processing the request itself
+            queue_span.finish
+
+            yield.tap { request_span.finish }
+          end
+        end
+      end
+    end
+  end
+end

lib/datadog/tracing/contrib/rails/configuration/settings.rb

@@ -51,6 +51,7 @@ def initialize(options = {})
             option :distributed_tracing, default: true, type: :bool
 
             option :request_queuing do |o|
+              o.type :bool
               o.default false
             end
 

sig/datadog/tracing/contrib/rack/ext.rbs

@@ -15,8 +15,6 @@ module Datadog
 
           SPAN_HTTP_PROXY_QUEUE: "http.proxy.queue"
 
-          SPAN_HTTP_SERVER_QUEUE: "http_server.queue"
-
           SPAN_REQUEST: "rack.request"
 
           TAG_COMPONENT: "rack"
@@ -29,8 +27,6 @@ module Datadog
 
           TAG_OPERATION_HTTP_PROXY_QUEUE: "queue"
 
-          TAG_OPERATION_HTTP_SERVER_QUEUE: "queue"
-
           WEBSERVER_APP: "webserver"
 
           DEFAULT_PEER_WEBSERVER_SERVICE_NAME: "web-server"

spec/datadog/tracing/contrib/rack/configuration_spec.rb

@@ -66,46 +66,7 @@
       let(:queue_value) { nil }
     end
 
-    shared_examples_for 'a Rack request with queuing including the request' do
-      let(:configuration_options) { super().merge(request_queuing: :include_request) }
-
-      it 'produces a queued Rack trace' do
-        is_expected.to be_ok
-
-        expect(trace.resource).to eq('GET 200')
-
-        expect(spans).to have(2).items
-
-        server_queue_span = spans[0]
-        rack_span = spans[1]
-
-        web_service_name = Datadog.configuration.tracing[:rack][:web_service_name]
-
-        expect(server_queue_span.name).to eq('http_server.queue')
-        expect(server_queue_span.type).to eq('proxy')
-        expect(server_queue_span.service).to eq(web_service_name)
-        expect(server_queue_span.start_time.to_i).to eq(queue_time)
-        expect(server_queue_span.get_tag(Datadog::Core::Runtime::Ext::TAG_LANG)).to be_nil
-        expect(server_queue_span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)).to eq('rack')
-        expect(server_queue_span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION)).to eq('queue')
-
-        expect(rack_span.name).to eq('rack.request')
-        expect(rack_span.type).to eq('web')
-        expect(rack_span.service).to eq(Datadog.configuration.service)
-        expect(rack_span.resource).to eq('GET 200')
-        expect(rack_span.get_tag('http.method')).to eq('GET')
-        expect(rack_span.get_tag('http.status_code')).to eq('200')
-        expect(rack_span.get_tag('http.url')).to eq('/')
-        expect(rack_span.status).to eq(0)
-        expect(rack_span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)).to eq('rack')
-        expect(rack_span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION)).to eq('request')
-        expect(rack_span.parent_id).to eq(server_queue_span.id)
-      end
-    end
-
-    shared_examples_for 'a Rack request with queuing excluding the request' do
-      let(:configuration_options) { super().merge(request_queuing: :exclude_request) }
-
+    shared_examples_for 'a Rack request with queuing' do
       it 'produces a queued Rack trace' do
         is_expected.to be_ok
 
@@ -177,7 +138,7 @@
     end
 
     context 'when enabled' do
-      let(:configuration_options) { super().merge(request_queuing: :exclude_request) }
+      let(:configuration_options) { super().merge(request_queuing: true) }
 
       context 'and a request is received' do
         include_context 'an incoming HTTP request'
@@ -187,24 +148,13 @@
             let(:queue_header) { 'X-Request-Start' }
           end
 
-          it_behaves_like 'a Rack request with queuing including the request'
-          it_behaves_like 'a Rack request with queuing excluding the request'
+          it_behaves_like 'a Rack request with queuing'
 
           context 'given a custom web service name' do
             let(:configuration_options) { super().merge(web_service_name: web_service_name) }
             let(:web_service_name) { 'nginx' }
 
-            it_behaves_like 'a Rack request with queuing including the request' do
-              it 'sets the custom service name' do
-                is_expected.to be_ok
-
-                queue_span = spans.find { |s| s.name == Datadog::Tracing::Contrib::Rack::Ext::SPAN_HTTP_SERVER_QUEUE }
-
-                expect(queue_span.service).to eq(web_service_name)
-              end
-            end
-
-            it_behaves_like 'a Rack request with queuing excluding the request' do
+            it_behaves_like 'a Rack request with queuing' do
               it 'sets the custom service name' do
                 is_expected.to be_ok
 
@@ -221,8 +171,7 @@
             let(:queue_header) { 'X-Queue-Start' }
           end
 
-          it_behaves_like 'a Rack request with queuing including the request'
-          it_behaves_like 'a Rack request with queuing excluding the request'
+          it_behaves_like 'a Rack request with queuing'
         end
 
         # Ensure a queuing Span is NOT created if there is a clock skew