fix: Fixing missing OpenTelemetry::Context detach on Excon instrumentation (open-telemetry#1725)

* fix: Fixing missing OpenTelemetry::Context detach on Excon instrumentation

When a span is not recorded, for example when you are sampling traces,
the OpenTelemetry::Context attached on `request_call` is never detached.
This happens because `handle_response` is doing nothing if
`span.recording?` is false.

Here is example of the error that the missing detach generated:
```
ERROR -- : OpenTelemetry error: calls to detach should match corresponding calls to attach.
```

* Fixing tests following action_pack strategy to change the sampler

* Improving datum handling

---------

Co-authored-by: Kayla Reopelle <[email protected]>

Author: 2 people

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

@@ -84,6 +84,8 @@ def self.around_default_stack
 
             def handle_response(datum)
               datum.delete(:otel_span)&.tap do |span|
+                token = datum.delete(:otel_token)
+                OpenTelemetry::Context.detach(token) if token
                 return unless span.recording?
 
                 if datum.key?(:response)
@@ -99,7 +101,6 @@ 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)

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

@@ -76,6 +76,8 @@ def self.around_default_stack
 
             def handle_response(datum)
               datum.delete(:otel_span)&.tap do |span|
+                token = datum.delete(:otel_token)
+                OpenTelemetry::Context.detach(token) if token
                 return unless span.recording?
 
                 if datum.key?(:response)
@@ -90,7 +92,6 @@ 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)

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

@@ -76,6 +76,8 @@ def self.around_default_stack
 
             def handle_response(datum)
               datum.delete(:otel_span)&.tap do |span|
+                token = datum.delete(:otel_token)
+                OpenTelemetry::Context.detach(token) if token
                 return unless span.recording?
 
                 if datum.key?(:response)
@@ -90,7 +92,6 @@ 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)

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

@@ -199,6 +199,19 @@
 
       _(span.attributes['peer.service']).must_equal 'example:custom'
     end
+
+    describe 'context detachment with non-recording spans' do
+      it 'detaches context when span is not recorded' do
+        with_sampler(OpenTelemetry::SDK::Trace::Samplers::ALWAYS_OFF) do
+          initial_context = OpenTelemetry::Context.current
+          Excon.get('http://example.com/success')
+          final_context = OpenTelemetry::Context.current
+
+          _(final_context).must_equal initial_context
+          _(exporter.finished_spans).must_be_empty
+        end
+      end
+    end
   end
 
   describe 'untraced?' do

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

@@ -158,6 +158,19 @@
 
       _(span.attributes['peer.service']).must_equal 'example:custom'
     end
+
+    describe 'context detachment with non-recording spans' do
+      it 'detaches context when span is not recorded' do
+        with_sampler(OpenTelemetry::SDK::Trace::Samplers::ALWAYS_OFF) do
+          initial_context = OpenTelemetry::Context.current
+          Excon.get('http://example.com/success')
+          final_context = OpenTelemetry::Context.current
+
+          _(final_context).must_equal initial_context
+          _(exporter.finished_spans).must_be_empty
+        end
+      end
+    end
   end
 
   describe 'untraced?' do

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

@@ -172,6 +172,19 @@
 
       _(span.attributes['peer.service']).must_equal 'example:custom'
     end
+
+    describe 'context detachment with non-recording spans' do
+      it 'detaches context when span is not recorded' do
+        with_sampler(OpenTelemetry::SDK::Trace::Samplers::ALWAYS_OFF) do
+          initial_context = OpenTelemetry::Context.current
+          Excon.get('http://example.com/success')
+          final_context = OpenTelemetry::Context.current
+
+          _(final_context).must_equal initial_context
+          _(exporter.finished_spans).must_be_empty
+        end
+      end
+    end
   end
 
   describe 'untraced?' do

instrumentation/excon/test/test_helper.rb

@@ -20,3 +20,11 @@
   c.logger = Logger.new($stderr, level: ENV.fetch('OTEL_LOG_LEVEL', 'fatal').to_sym)
   c.add_span_processor span_processor
 end
+
+def with_sampler(sampler)
+  previous_sampler = OpenTelemetry.tracer_provider.sampler
+  OpenTelemetry.tracer_provider.sampler = sampler
+  yield
+ensure
+  OpenTelemetry.tracer_provider.sampler = previous_sampler
+end