From ad99f5aa0f1979061226cb1c043cc3e3501ccd21 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Mon, 6 Nov 2023 15:20:01 +0000 Subject: [PATCH 01/22] Add a connect span to excon and add more span attributes to the tracer middleware --- .../instrumentation/excon/instrumentation.rb | 15 +- .../excon/middlewares/tracer_middleware.rb | 79 +++++----- .../instrumentation/excon/patches/socket.rb | 60 ++++++++ .../excon/instrumentation_test.rb | 136 ++++++++++++++++++ 4 files changed, 245 insertions(+), 45 deletions(-) create mode 100644 instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb index 1a8a4d334..9ee260fb6 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb +++ b/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 diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 264bf5067..d81525131 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/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 diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb new file mode 100644 index 000000000..472531488 --- /dev/null +++ b/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 diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 2e1ba7bea..5b6059269 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/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,139 @@ _(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 { server.accept } + 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) + _(span_event.attributes['exception.message']).must_match(/nodename nor servname provided, or not known/) + 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 From 997d5cd371ed2cb21ba2803edbb3ce914119231c Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Mon, 6 Nov 2023 15:21:43 +0000 Subject: [PATCH 02/22] Skip matching on the error message as it varies by platform --- .../opentelemetry/instrumentation/excon/instrumentation_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 5b6059269..523d45649 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -239,7 +239,6 @@ _(span_event.name).must_equal 'exception' _(span_event.attributes['exception.type']).must_equal(SocketError.name) - _(span_event.attributes['exception.message']).must_match(/nodename nor servname provided, or not known/) ensure WebMock.disable_net_connect! end From b399dd8ab04af1b9071dde54953e1924ea929281 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Mon, 6 Nov 2023 15:25:48 +0000 Subject: [PATCH 03/22] Rescue the IOError that can occur on accept --- .../instrumentation/excon/instrumentation_test.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 523d45649..c8cf9b40d 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -211,7 +211,12 @@ WebMock.allow_net_connect! TCPServer.open('localhost', 0) do |server| - Thread.start { server.accept } + 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) From 9c4e78799a1c22ced46499cdad5c3a2b33b0c49f Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 14:54:11 +0000 Subject: [PATCH 04/22] Switch to must_be_empty assertion instead of size --- .../instrumentation/excon/instrumentation_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index c8cf9b40d..31722c866 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -40,7 +40,7 @@ end it 'before request' do - _(exporter.finished_spans.size).must_equal 0 + _(exporter.finished_spans).must_be_empty end it 'after request with success code' do @@ -163,12 +163,12 @@ 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 + _(exporter.finished_spans).must_be_empty 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 + _(exporter.finished_spans).must_be_empty end it 'does not create a span on connect when request ignored using a regexp' do @@ -176,7 +176,7 @@ Excon::Socket.new(hostname: uri.host, port: uri.port) - _(exporter.finished_spans.size).must_equal 0 + _(exporter.finished_spans).must_be_empty end it 'creates a span for a non-ignored request' do From 854125b11e12880a7a42d20ca36cae15e035aa25 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 14:56:04 +0000 Subject: [PATCH 05/22] Move allowing and disallowing connect to setup and after respectively. --- .../excon/instrumentation_test.rb | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 31722c866..b014ffda3 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -205,11 +205,14 @@ describe '#connect' do before do instrumentation.install + WebMock.allow_net_connect! end - it 'emits span on connect' do - WebMock.allow_net_connect! + after do + WebMock.disable_net_connect! + end + it 'emits span on connect' do TCPServer.open('localhost', 0) do |server| Thread.start do server.accept @@ -226,13 +229,9 @@ _(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) @@ -244,13 +243,9 @@ _(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) @@ -258,13 +253,9 @@ _(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) @@ -272,13 +263,9 @@ _(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) @@ -286,8 +273,6 @@ _(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 From 8c8eabc6709a8a13c2820aa24512e2e7a2f0d6fb Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 15:03:04 +0000 Subject: [PATCH 06/22] use dig when getting hostname and port of proxy --- .../lib/opentelemetry/instrumentation/excon/patches/socket.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb index 472531488..d3a9e28e9 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb @@ -16,8 +16,8 @@ def connect return super if untraced? if @data[:proxy] - conn_address = @data[:proxy][:hostname] - conn_port = @data[:proxy][:port] + conn_address = @data.dig(:proxy, :hostname) + conn_port = @data.dig(:proxy, :port) else conn_address = @data[:hostname] conn_port = @port From 175818ce2528935ba6585f18ab57318d9896287c Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 15:12:00 +0000 Subject: [PATCH 07/22] Call untraced when we hit an untraced host. --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 6 +++++- .../opentelemetry/instrumentation/excon/patches/socket.rb | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index d81525131..c1896faee 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -22,7 +22,11 @@ class TracerMiddleware < ::Excon::Middleware::Base end.freeze def request_call(datum) - return @stack.request_call(datum) if skip_trace?(datum) + if skip_trace?(datum) + return OpenTelemetry::Common::Utilities.untraced do + @stack.request_call(datum) + end + end http_method = HTTP_METHODS_TO_UPPERCASE[datum[:method]] diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb index d3a9e28e9..f6a1f7ab5 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb @@ -13,7 +13,9 @@ module Socket private def connect - return super if untraced? + if untraced? + return OpenTelemetry::Common::Utilities.untraced { super } + end if @data[:proxy] conn_address = @data.dig(:proxy, :hostname) @@ -44,7 +46,7 @@ def tracer def untraced? address = if @data[:proxy] - @data[:proxy][:hostname] + @data.dig(:proxy, :hostname) else @data[:hostname] end From 6415ba810b428d220b9be4fd5d133d4fe2197a54 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 15:14:14 +0000 Subject: [PATCH 08/22] Switch to using recording? to test whether we should finish a span. --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index c1896faee..9845e7e29 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -80,7 +80,7 @@ def self.around_default_stack def handle_response(datum) datum.delete(:otel_span)&.tap do |span| - return span if span.end_timestamp + return span unless span.recording? if datum.key?(:response) response = datum[:response] From 441f1d0efdc5af46b0fe868a34e8494419686e1b Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 15:19:51 +0000 Subject: [PATCH 09/22] Switch to handle_error instead of debug logging. --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 9845e7e29..608b94b63 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -93,7 +93,7 @@ def handle_response(datum) span.finish end rescue StandardError => e - OpenTelemetry.logger.debug(e.message) + OpenTelemetry.handle_error(e) end def tracer From 55bf3f9609cf17133098f6e6f87395b7e25401ad Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 15:23:52 +0000 Subject: [PATCH 10/22] Record the exception on error --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 5 ++++- .../instrumentation/excon/instrumentation_test.rb | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 608b94b63..48ab9edc6 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -88,7 +88,10 @@ def handle_response(datum) 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) + if datum.key?(:error) + span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") + span.record_exception(datum[:error]) + end span.finish end diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index b014ffda3..36e40cece 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -105,6 +105,10 @@ 'http://example.com/timeout', headers: { 'Traceparent' => "00-#{span.hex_trace_id}-#{span.hex_span_id}-01" } ) + + exception_event = span.events.first + _(exception_event.attributes['exception.type']).must_equal('Excon::Error::Timeout') + _(exception_event.attributes['exception.message']).must_equal('Excon::Error::Timeout') end it 'merges HTTP client context' do From 4bf5b3d6e50bcdcc5002e0fb41dc639b350a7b15 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 18:29:11 +0000 Subject: [PATCH 11/22] Perform the next step in the middleware stack in the context of the current span. --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 48ab9edc6..175974021 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -47,9 +47,9 @@ def request_call(datum) OpenTelemetry::Trace.with_span(datum[:otel_span]) do OpenTelemetry.propagation.inject(datum[:headers]) + + @stack.request_call(datum) end - - @stack.request_call(datum) end def response_call(datum) From 99d8c620fb20a5736af054559c9ccbf8be8a0da8 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Thu, 16 Nov 2023 18:51:34 +0000 Subject: [PATCH 12/22] Add assertions on http spans to connect tests. --- .../excon/instrumentation_test.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 36e40cece..be4a3c63a 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -233,6 +233,8 @@ _(span.name).must_equal 'connect' _(span.attributes['net.peer.name']).must_equal('localhost') _(span.attributes['net.peer.port']).wont_be_nil + + assert_http_spans(target: '/example', exception: 'Excon::Error::Timeout') end it 'captures errors' do @@ -247,6 +249,8 @@ _(span_event.name).must_equal 'exception' _(span_event.attributes['exception.type']).must_equal(SocketError.name) + + assert_http_spans(host: 'invalid.com', target: '/example') end it '[BUG] fails to emit an HTTP CONNECT span when connecting through an SSL proxy for an HTTP service' do @@ -257,6 +261,8 @@ _(span.kind).must_equal(:internal) _(span.attributes['net.peer.name']).must_equal('localhost') _(span.attributes['net.peer.port']).must_equal(443) + + assert_http_spans end it 'emits an HTTP CONNECT span when connecting through an SSL proxy' do @@ -267,6 +273,8 @@ _(span.kind).must_equal(:client) _(span.attributes['net.peer.name']).must_equal('localhost') _(span.attributes['net.peer.port']).must_equal(443) + + assert_http_spans(scheme: "https") end it 'emits a "connect" span when connecting through an non-ssl proxy' do @@ -277,6 +285,26 @@ _(span.kind).must_equal(:internal) _(span.attributes['net.peer.name']).must_equal('localhost') _(span.attributes['net.peer.port']).must_equal(443) + + assert_http_spans(exception: 'Excon::Error::Socket') + end + end + + def assert_http_spans(scheme: 'http', host: 'localhost', target: '/', exception: nil) + exporter.finished_spans[1..].each do |http_span| + _(http_span.name).must_equal 'HTTP GET' + _(http_span.attributes['http.method']).must_equal 'GET' + _(http_span.attributes['http.scheme']).must_equal scheme + _(http_span.attributes['http.host']).must_equal host + _(http_span.attributes['http.target']).must_equal target + _(http_span.status.code).must_equal( + OpenTelemetry::Trace::Status::ERROR + ) + + if exception + exception_event = http_span.events.first + _(exception_event.attributes['exception.type']).must_equal(exception) + end end end end From fd148ef9f5f93f2846545b18fc690f78851acd2a Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 20:17:07 +0000 Subject: [PATCH 13/22] Fix rubocop lints --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- .../lib/opentelemetry/instrumentation/excon/patches/socket.rb | 4 +--- .../instrumentation/excon/instrumentation_test.rb | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 175974021..381b28e4f 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -47,7 +47,7 @@ def request_call(datum) OpenTelemetry::Trace.with_span(datum[:otel_span]) do OpenTelemetry.propagation.inject(datum[:headers]) - + @stack.request_call(datum) end end diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb index f6a1f7ab5..7ce2a42ad 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb @@ -13,9 +13,7 @@ module Socket private def connect - if untraced? - return OpenTelemetry::Common::Utilities.untraced { super } - end + return OpenTelemetry::Common::Utilities.untraced { super } if untraced? if @data[:proxy] conn_address = @data.dig(:proxy, :hostname) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index be4a3c63a..f0ed4a249 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -274,7 +274,7 @@ _(span.attributes['net.peer.name']).must_equal('localhost') _(span.attributes['net.peer.port']).must_equal(443) - assert_http_spans(scheme: "https") + assert_http_spans(scheme: 'https') end it 'emits a "connect" span when connecting through an non-ssl proxy' do From d6f6c2beb7c55f655052ae9129984baf521f247d Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 20:30:31 +0000 Subject: [PATCH 14/22] Remove interpolation from status message on span now that we capture exceptions as an event. --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- .../instrumentation/excon/instrumentation_test.rb | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 381b28e4f..f6655c5e6 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -89,7 +89,7 @@ def handle_response(datum) end if datum.key?(:error) - span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") + span.status = OpenTelemetry::Trace::Status.error('Request has failed') span.record_exception(datum[:error]) end diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index f0ed4a249..1f485c832 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -97,9 +97,7 @@ _(span.status.code).must_equal( OpenTelemetry::Trace::Status::ERROR ) - _(span.status.description).must_equal( - 'Request has failed: Excon::Error::Timeout' - ) + _(span.status.description).must_equal('Request has failed') assert_requested( :get, 'http://example.com/timeout', From 2cdce0b5dbe072df2c3e23dbf6f38d7689e2a34a Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 20:45:50 +0000 Subject: [PATCH 15/22] Switch to attach and detach instead of with_span --- .../excon/middlewares/tracer_middleware.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index f6655c5e6..a0f99d899 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -43,13 +43,15 @@ def request_call(datum) 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) + span = tracer.start_span(HTTP_METHODS_TO_SPAN_NAMES[http_method], attributes: attributes, kind: :client) + ctx = OpenTelemetry::Trace.context_with_span(span) - OpenTelemetry::Trace.with_span(datum[:otel_span]) do - OpenTelemetry.propagation.inject(datum[:headers]) + datum[:otel_span] = span + datum[:otel_token] = OpenTelemetry::Context.attach(ctx) - @stack.request_call(datum) - end + OpenTelemetry.propagation.inject(datum[:headers]) + + @stack.request_call(datum) end def response_call(datum) @@ -94,6 +96,8 @@ def handle_response(datum) end span.finish + + OpenTelemetry::Context.detach(datum.delete(:otel_token)) if datum.include?(:otel_token) end rescue StandardError => e OpenTelemetry.handle_error(e) From d1aa1d478e6b6a60fc87d43f29ee820167154374 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 20:53:01 +0000 Subject: [PATCH 16/22] Include untraced context into untraced? check for the middleware and patch. --- .../excon/middlewares/tracer_middleware.rb | 18 +++++++++++------- .../instrumentation/excon/patches/socket.rb | 4 +++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index a0f99d899..98e5be9a4 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -22,11 +22,7 @@ class TracerMiddleware < ::Excon::Middleware::Base end.freeze def request_call(datum) - if skip_trace?(datum) - return OpenTelemetry::Common::Utilities.untraced do - @stack.request_call(datum) - end - end + return @stack.request_call(datum) if untraced?(datum) http_method = HTTP_METHODS_TO_UPPERCASE[datum[:method]] @@ -107,8 +103,16 @@ def tracer Excon::Instrumentation.instance.tracer end - def skip_trace?(datum) - datum.key?(:otel_span) || Excon::Instrumentation.instance.config[:untraced_hosts].any? do |host| + def untraced?(datum) + untraced_context? || datum.key?(:otel_span) || untraced_host?(datum) + end + + def untraced_context? + OpenTelemetry::Common::Utilities.untraced? + end + + def untraced_host?(datum) + Excon::Instrumentation.instance.config[:untraced_hosts].any? do |host| host.is_a?(Regexp) ? host.match?(datum[:host]) : host == datum[:host] end end diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb index 7ce2a42ad..dc3b7e167 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb @@ -13,7 +13,7 @@ module Socket private def connect - return OpenTelemetry::Common::Utilities.untraced { super } if untraced? + return super if untraced? if @data[:proxy] conn_address = @data.dig(:proxy, :hostname) @@ -43,6 +43,8 @@ def tracer end def untraced? + return true if OpenTelemetry::Common::Utilities.untraced? + address = if @data[:proxy] @data.dig(:proxy, :hostname) else From e3fde0c80a4823bf1ddf97dd1941d9d001630c65 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 20:54:42 +0000 Subject: [PATCH 17/22] Add test for untraced. --- .../instrumentation/excon/instrumentation_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb index 1f485c832..fa166e1e6 100644 --- a/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb +++ b/instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb @@ -286,6 +286,14 @@ assert_http_spans(exception: 'Excon::Error::Socket') end + + it 'emits no spans when untraced' do + OpenTelemetry::Common::Utilities.untraced do + _(-> { Excon.get('http://localhost', proxy: 'https://proxy_user:proxy_pass@localhost') }).must_raise(Excon::Error::Socket) + + _(exporter.finished_spans.size).must_equal(0) + end + end end def assert_http_spans(scheme: 'http', host: 'localhost', target: '/', exception: nil) From 2fbc8e9a38b5846fc00b98503c5d59648ec17eb6 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 21:06:00 +0000 Subject: [PATCH 18/22] Add a module that centralizes the untraced hosts concern. --- .../base/lib/opentelemetry/instrumentation.rb | 1 + .../concerns/untraced_hosts.rb | 34 +++++++++++++++++++ .../instrumentation/excon/instrumentation.rb | 8 ++--- .../excon/middlewares/tracer_middleware.rb | 12 +------ .../instrumentation/excon/patches/socket.rb | 6 +--- 5 files changed, 39 insertions(+), 22 deletions(-) create mode 100644 instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb diff --git a/instrumentation/base/lib/opentelemetry/instrumentation.rb b/instrumentation/base/lib/opentelemetry/instrumentation.rb index c02712b64..2d8ee805f 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation.rb @@ -7,6 +7,7 @@ require 'opentelemetry' require 'opentelemetry-registry' require 'opentelemetry/instrumentation/base' +require 'opentelemetry/instrumentation/concerns/untraced_hosts' module OpenTelemetry # The instrumentation module contains functionality to register and install diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb b/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb new file mode 100644 index 000000000..4b6bd887a --- /dev/null +++ b/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Instrumentation + module Concerns + # The untraced hosts concerns allows instrumentation to skip traces on hostnames in an exclusion list. + module UntracedHosts + def self.included(klass) + klass.instance_eval do + # 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 + end + end + + def untraced?(host) + OpenTelemetry::Common::Utilities.untraced? || untraced_host?(host) + end + + def untraced_host?(host) + config[:untraced_hosts].any? do |rule| + rule.is_a?(Regexp) ? rule.match?(host) : rule == host + end + end + end + end + end +end \ No newline at end of file diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb index 9ee260fb6..1d5a40525 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb @@ -10,6 +10,8 @@ module Excon # The Instrumentation class contains logic to detect and install the Excon # instrumentation class Instrumentation < OpenTelemetry::Instrumentation::Base + include OpenTelemetry::Instrumentation::Concerns::UntracedHosts + install do |_config| require_dependencies add_middleware @@ -22,12 +24,6 @@ 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 diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 98e5be9a4..2de62e21a 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -104,17 +104,7 @@ def tracer end def untraced?(datum) - untraced_context? || datum.key?(:otel_span) || untraced_host?(datum) - end - - def untraced_context? - OpenTelemetry::Common::Utilities.untraced? - end - - def untraced_host?(datum) - Excon::Instrumentation.instance.config[:untraced_hosts].any? do |host| - host.is_a?(Regexp) ? host.match?(datum[:host]) : host == datum[:host] - end + datum.key?(:otel_span) || Excon::Instrumentation.instance.untraced?(datum[:host]) end end end diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb index dc3b7e167..b96ed8833 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb @@ -43,17 +43,13 @@ def tracer end def untraced? - return true if OpenTelemetry::Common::Utilities.untraced? - address = if @data[:proxy] @data.dig(: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 + Excon::Instrumentation.instance.untraced?(address) end end end From 1721eea15fb8c1cd2c96d7157be600f6ef74bb3a Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Tue, 21 Nov 2023 21:08:18 +0000 Subject: [PATCH 19/22] Expand doc comment. --- .../opentelemetry/instrumentation/concerns/untraced_hosts.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb b/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb index 4b6bd887a..ec3aff83c 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb @@ -7,7 +7,8 @@ module OpenTelemetry module Instrumentation module Concerns - # The untraced hosts concerns allows instrumentation to skip traces on hostnames in an exclusion list. + # The untraced hosts concerns allows instrumentation to skip traces on hostnames in an exclusion list. + # When included in a class that extends OpenTelemetry::Instrumentation::Base, this module defines an option named :untraced_hosts. module UntracedHosts def self.included(klass) klass.instance_eval do From c0a2404dfad5ba14cac00f449ba6de94ab089721 Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Wed, 22 Nov 2023 16:33:14 +0000 Subject: [PATCH 20/22] Move module to the excon gem. --- .../base/lib/opentelemetry/instrumentation.rb | 1 - .../concerns/untraced_hosts.rb | 23 +++++++++++-------- .../instrumentation/excon/instrumentation.rb | 2 ++ 3 files changed, 15 insertions(+), 11 deletions(-) rename instrumentation/{base => excon}/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb (54%) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation.rb b/instrumentation/base/lib/opentelemetry/instrumentation.rb index 2d8ee805f..c02712b64 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation.rb @@ -7,7 +7,6 @@ require 'opentelemetry' require 'opentelemetry-registry' require 'opentelemetry/instrumentation/base' -require 'opentelemetry/instrumentation/concerns/untraced_hosts' module OpenTelemetry # The instrumentation module contains functionality to register and install diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb similarity index 54% rename from instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb rename to instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb index ec3aff83c..cba3557ea 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb @@ -8,22 +8,25 @@ module OpenTelemetry module Instrumentation module Concerns # The untraced hosts concerns allows instrumentation to skip traces on hostnames in an exclusion list. + # If the current OpenTelemetry context is untraced, all hosts will be treated as untraced. # When included in a class that extends OpenTelemetry::Instrumentation::Base, this module defines an option named :untraced_hosts. module UntracedHosts def self.included(klass) - klass.instance_eval do - # 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 - end + klass.instance_eval do + # 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 + end end def untraced?(host) - OpenTelemetry::Common::Utilities.untraced? || untraced_host?(host) + OpenTelemetry::Common::Utilities.untraced? || untraced_host?(host) end + private + def untraced_host?(host) config[:untraced_hosts].any? do |rule| rule.is_a?(Regexp) ? rule.match?(host) : rule == host @@ -31,5 +34,5 @@ def untraced_host?(host) end end end - end -end \ No newline at end of file + end +end diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb index 1d5a40525..e20ca9b2d 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/instrumentation.rb @@ -4,6 +4,8 @@ # # SPDX-License-Identifier: Apache-2.0 +require_relative '../concerns/untraced_hosts' + module OpenTelemetry module Instrumentation module Excon From 64011e8ddac12937ac4aa358970c5848d75bd10e Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Wed, 22 Nov 2023 16:36:45 +0000 Subject: [PATCH 21/22] Add doc comment for untraced? in the concern --- .../opentelemetry/instrumentation/concerns/untraced_hosts.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb index cba3557ea..81acdd6a3 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/concerns/untraced_hosts.rb @@ -21,6 +21,9 @@ def self.included(klass) end end + # Checks whether the given host should be treated as untraced. + # If the current OpenTelemetry context is untraced, all hosts will be treated as untraced. + # The given host must be a String. def untraced?(host) OpenTelemetry::Common::Utilities.untraced? || untraced_host?(host) end From 9fb30729cfce2d6e059c4e8e75043ae3c46f4edc Mon Sep 17 00:00:00 2001 From: "Miguel D. Salcedo" Date: Wed, 22 Nov 2023 13:52:14 -0500 Subject: [PATCH 22/22] Update instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb Co-authored-by: Ariel Valentin --- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 2de62e21a..91234fe53 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -78,7 +78,7 @@ def self.around_default_stack def handle_response(datum) datum.delete(:otel_span)&.tap do |span| - return span unless span.recording? + return unless span.recording? if datum.key?(:response) response = datum[:response]