feat(excon)! Add a connect span to excon and add more span attributes to the tracer middleware

instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb

@@ -13,6 +13,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
         install do |_config|
           require_dependencies
           add_middleware
+          patch
         end
 
         present do
@@ -21,15 +22,25 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
 
         option :peer_service, default: nil, validate: :string
 
+        # untraced_hosts: if a request's address matches any of the `String`
+        #   or `Regexp` in this array, the instrumentation will not record a
+        #   `kind = :client` representing the request and will not propagate
+        #   context in the request.
+        option :untraced_hosts, default: [], validate: :array
+
         private
 
         def require_dependencies
           require_relative 'middlewares/tracer_middleware'
+          require_relative 'patches/socket'
         end
 
         def add_middleware
-          ::Excon.defaults[:middlewares] =
-            Middlewares::TracerMiddleware.around_default_stack
+          ::Excon.defaults[:middlewares] = Middlewares::TracerMiddleware.around_default_stack
+        end
+
+        def patch
+          ::Excon::Socket.prepend(Patches::Socket)
         end
       end
     end

instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb

@@ -22,23 +22,27 @@ class TracerMiddleware < ::Excon::Middleware::Base
           end.freeze
 
           def request_call(datum)
-            begin
-              unless datum.key?(:otel_span)
-                http_method = HTTP_METHODS_TO_UPPERCASE[datum[:method]]
-                attributes = span_creation_attributes(datum, http_method)
-                tracer.start_span(
-                  HTTP_METHODS_TO_SPAN_NAMES[http_method],
-                  attributes: attributes,
-                  kind: :client
-                ).tap do |span|
-                  datum[:otel_span] = span
-                  OpenTelemetry::Trace.with_span(span) do
-                    OpenTelemetry.propagation.inject(datum[:headers])
-                  end
-                end
-              end
-            rescue StandardError => e
-              OpenTelemetry.logger.debug(e.message)
+            return @stack.request_call(datum) if skip_trace?(datum)
+
+            http_method = HTTP_METHODS_TO_UPPERCASE[datum[:method]]
+
+            attributes = {
+              OpenTelemetry::SemanticConventions::Trace::HTTP_METHOD => http_method,
+              OpenTelemetry::SemanticConventions::Trace::HTTP_SCHEME => datum[:scheme],
+              OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET => datum[:path],
+              OpenTelemetry::SemanticConventions::Trace::HTTP_HOST => datum[:host],
+              OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => datum[:hostname],
+              OpenTelemetry::SemanticConventions::Trace::NET_PEER_PORT => datum[:port]
+            }
+
+            peer_service = Excon::Instrumentation.instance.config[:peer_service]
+            attributes[OpenTelemetry::SemanticConventions::Trace::PEER_SERVICE] = peer_service if peer_service
+            attributes.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)
+
+            datum[:otel_span] = tracer.start_span(HTTP_METHODS_TO_SPAN_NAMES[http_method], attributes: attributes, kind: :client)
+
+            OpenTelemetry::Trace.with_span(datum[:otel_span]) do
+              OpenTelemetry.propagation.inject(datum[:headers])
             end
 
             @stack.request_call(datum)
@@ -71,43 +75,32 @@ def self.around_default_stack
           private
 
           def handle_response(datum)
-            if datum.key?(:otel_span)
-              datum[:otel_span].tap do |span|
-                return span if span.end_timestamp
+            datum.delete(:otel_span)&.tap do |span|
+              return span if span.end_timestamp
 
-                if datum.key?(:response)
-                  response = datum[:response]
-                  span.set_attribute('http.status_code', response[:status])
-                  span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response[:status].to_i)
-                end
+              if datum.key?(:response)
+                response = datum[:response]
+                span.set_attribute(OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE, response[:status])
+                span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response[:status].to_i)
+              end
 
-                span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") if datum.key?(:error)
+              span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") if datum.key?(:error)
 
-                span.finish
-                datum.delete(:otel_span)
-              end
+              span.finish
             end
           rescue StandardError => e
             OpenTelemetry.logger.debug(e.message)
           end
 
-          def span_creation_attributes(datum, http_method)
-            instrumentation_attrs = {
-              'http.host' => datum[:host],
-              'http.method' => http_method,
-              'http.scheme' => datum[:scheme],
-              'http.target' => datum[:path]
-            }
-            config = Excon::Instrumentation.instance.config
-            instrumentation_attrs['peer.service'] = config[:peer_service] if config[:peer_service]
-            instrumentation_attrs.merge!(
-              OpenTelemetry::Common::HTTP::ClientContext.attributes
-            )
-          end
-
           def tracer
             Excon::Instrumentation.instance.tracer
           end
+
+          def skip_trace?(datum)
+            datum.key?(:otel_span) || Excon::Instrumentation.instance.config[:untraced_hosts].any? do |host|
+              host.is_a?(Regexp) ? host.match?(datum[:host]) : host == datum[:host]
+            end
+          end
         end
       end
     end

instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+# Copyright The OpenTelemetry Authors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+module OpenTelemetry
+  module Instrumentation
+    module Excon
+      module Patches
+        # Module to prepend to an Excon Socket for instrumentation
+        module Socket
+          private
+
+          def connect
+            return super if untraced?
+
+            if @data[:proxy]
+              conn_address = @data[:proxy][:hostname]
+              conn_port = @data[:proxy][:port]
+            else
+              conn_address = @data[:hostname]
+              conn_port = @port
+            end
+
+            attributes = { OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => conn_address, OpenTelemetry::SemanticConventions::Trace::NET_PEER_PORT => conn_port }.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)
+
+            if is_a?(::Excon::SSLSocket) && @data[:proxy]
+              span_name = 'HTTP CONNECT'
+              span_kind = :client
+            else
+              span_name = 'connect'
+              span_kind = :internal
+            end
+
+            tracer.in_span(span_name, attributes: attributes, kind: span_kind) do
+              super
+            end
+          end
+
+          def tracer
+            Excon::Instrumentation.instance.tracer
+          end
+
+          def untraced?
+            address = if @data[:proxy]
+                        @data[:proxy][:hostname]
+                      else
+                        @data[:hostname]
+                      end
+
+            Excon::Instrumentation.instance.config[:untraced_hosts].any? do |host|
+              host.is_a?(Regexp) ? host.match?(address) : host == address
+            end
+          end
+        end
+      end
+    end
+  end
+end

instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb

@@ -8,6 +8,7 @@
 
 require_relative '../../../../lib/opentelemetry/instrumentation/excon'
 require_relative '../../../../lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware'
+require_relative '../../../../lib/opentelemetry/instrumentation/excon/patches/socket'
 
 describe OpenTelemetry::Instrumentation::Excon::Instrumentation do
   let(:instrumentation) { OpenTelemetry::Instrumentation::Excon::Instrumentation.instance }
@@ -150,4 +151,143 @@
       _(span.attributes['peer.service']).must_equal 'example:custom'
     end
   end
+
+  describe 'untraced?' do
+    before do
+      instrumentation.install(untraced_hosts: ['foobar.com', /bazqux\.com/])
+
+      stub_request(:get, 'http://example.com/body').to_return(status: 200)
+      stub_request(:get, 'http://foobar.com/body').to_return(status: 200)
+      stub_request(:get, 'http://bazqux.com/body').to_return(status: 200)
+    end
+
+    it 'does not create a span when request ignored using a string' do
+      Excon.get('http://foobar.com/body')
+      _(exporter.finished_spans.size).must_equal 0
+    end
+
+    it 'does not create a span when request ignored using a regexp' do
+      Excon.get('http://bazqux.com/body')
+      _(exporter.finished_spans.size).must_equal 0
+    end
+
+    it 'does not create a span on connect when request ignored using a regexp' do
+      uri = URI.parse('http://bazqux.com')
+
+      Excon::Socket.new(hostname: uri.host, port: uri.port)
+
+      _(exporter.finished_spans.size).must_equal 0
+    end
+
+    it 'creates a span for a non-ignored request' do
+      Excon.get('http://example.com/body')
+
+      _(exporter.finished_spans.size).must_equal 1
+      _(span.name).must_equal 'HTTP GET'
+      _(span.attributes['http.method']).must_equal 'GET'
+      _(span.attributes['http.host']).must_equal 'example.com'
+    end
+
+    it 'creates a span on connect for a non-ignored request' do
+      uri = URI.parse('http://example.com')
+
+      Excon::Socket.new(hostname: uri.host, port: uri.port)
+
+      _(exporter.finished_spans.size).must_equal 1
+      _(span.name).must_equal('connect')
+      _(span.kind).must_equal(:internal)
+      _(span.attributes['net.peer.name']).must_equal('example.com')
+      _(span.attributes['net.peer.port']).must_equal(80)
+    end
+  end
+
+  # NOTE: WebMock introduces an extra HTTP request span due to the way the mocking is implemented.
+  describe '#connect' do
+    before do
+      instrumentation.install
+    end
+
+    it 'emits span on connect' do
+      WebMock.allow_net_connect!
+
+      TCPServer.open('localhost', 0) do |server|
+        Thread.start do
+          server.accept
+        rescue IOError
+          nil
+        end
+
+        port = server.addr[1]
+
+        _(-> { Excon.get("http://localhost:#{port}/example", read_timeout: 0) }).must_raise(Excon::Error::Timeout)
+      end
+
+      _(exporter.finished_spans.size).must_equal(3)
+      _(span.name).must_equal 'connect'
+      _(span.attributes['net.peer.name']).must_equal('localhost')
+      _(span.attributes['net.peer.port']).wont_be_nil
+    ensure
+      WebMock.disable_net_connect!
+    end
+
+    it 'captures errors' do
+      WebMock.allow_net_connect!
+
+      _(-> { Excon.get('http://invalid.com:99999/example') }).must_raise
+
+      _(exporter.finished_spans.size).must_equal(3)
+      _(span.name).must_equal 'connect'
+      _(span.attributes['net.peer.name']).must_equal('invalid.com')
+      _(span.attributes['net.peer.port']).must_equal(99_999)
+
+      span_event = span.events.first
+
+      _(span_event.name).must_equal 'exception'
+      _(span_event.attributes['exception.type']).must_equal(SocketError.name)
+    ensure
+      WebMock.disable_net_connect!
+    end
+
+    it '[BUG] fails to emit an HTTP CONNECT span when connecting through an SSL proxy for an HTTP service' do
+      WebMock.allow_net_connect!
+
+      _(-> { Excon.get('http://localhost/', proxy: 'https://proxy_user:proxy_pass@localhost') }).must_raise(Excon::Error::Socket)
+
+      _(exporter.finished_spans.size).must_equal(3)
+      _(span.name).must_equal 'connect'
+      _(span.kind).must_equal(:internal)
+      _(span.attributes['net.peer.name']).must_equal('localhost')
+      _(span.attributes['net.peer.port']).must_equal(443)
+    ensure
+      WebMock.disable_net_connect!
+    end
+
+    it 'emits an HTTP CONNECT span when connecting through an SSL proxy' do
+      WebMock.allow_net_connect!
+
+      _(-> { Excon.get('https://localhost/', proxy: 'https://proxy_user:proxy_pass@localhost') }).must_raise(Excon::Error::Socket)
+
+      _(exporter.finished_spans.size).must_equal(3)
+      _(span.name).must_equal 'HTTP CONNECT'
+      _(span.kind).must_equal(:client)
+      _(span.attributes['net.peer.name']).must_equal('localhost')
+      _(span.attributes['net.peer.port']).must_equal(443)
+    ensure
+      WebMock.disable_net_connect!
+    end
+
+    it 'emits a "connect" span when connecting through an non-ssl proxy' do
+      WebMock.allow_net_connect!
+
+      _(-> { Excon.get('http://localhost', proxy: 'https://proxy_user:proxy_pass@localhost') }).must_raise(Excon::Error::Socket)
+
+      _(exporter.finished_spans.size).must_equal(3)
+      _(span.name).must_equal 'connect'
+      _(span.kind).must_equal(:internal)
+      _(span.attributes['net.peer.name']).must_equal('localhost')
+      _(span.attributes['net.peer.port']).must_equal(443)
+    ensure
+      WebMock.disable_net_connect!
+    end
+  end
 end