fix test state reset issue

open-telemetry · Jan 7, 2025 · 28a6af8 · 28a6af8
1 parent c65742e
commit 28a6af8
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 38 deletions.
diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
@@ -390,9 +390,9 @@ def metrics_enabled_by_env_var?
         var_name.upcase!
         var_name.gsub!('::', '_')
         var_name.gsub!('OPENTELEMETRY_', 'OTEL_RUBY_')
-        var_name << 'METRICS_ENABLED'
+        var_name << '_METRICS_ENABLED'
 
-        ENV[var_name] != 'false'
+        ENV.key?(var_name) && ENV[var_name] != 'false'
       end
 
       # Checks to see if the user has passed any environment variables that set options

diff --git a/...q/test/opentelemetry/instrumentation/sidekiq/middlewares/client/tracer_middleware_test.rb b/...q/test/opentelemetry/instrumentation/sidekiq/middlewares/client/tracer_middleware_test.rb
@@ -14,10 +14,20 @@
   let(:spans) { exporter.finished_spans }
   let(:enqueue_span) { spans.first }
   let(:config) { {} }
+  let(:metrics_exporter) { METRICS_EXPORTER }
+
+  with_metrics_sdk do
+    let(:metric_snapshots) do
+      METRICS_EXPORTER.tap(&:pull)
+                      .metric_snapshots.select { |snapshot| snapshot.data_points.any? }
+                      .group_by(&:name)
+    end
+  end
 
   before do
     instrumentation.install(config)
     exporter.reset
+    reset_metrics_exporter
   end
 
   after do
@@ -81,5 +91,34 @@
         _(enqueue_span.attributes['peer.service']).must_equal 'MySidekiqService'
       end
     end
+
+    with_metrics_sdk do
+      it 'yields no metrics if config is not set' do
+        _(instrumentation.metrics_enabled?).must_equal false
+        SimpleJob.perform_async
+        SimpleJob.drain
+
+        _(metric_snapshots).must_be_empty
+      end
+
+      describe 'with metrics enabled' do
+        let(:config) { { metrics: true } }
+
+        it 'metrics processing' do
+          _(instrumentation.metrics_enabled?).must_equal true
+          SimpleJob.perform_async
+          SimpleJob.drain
+
+          sent_messages = metric_snapshots['messaging.client.sent.messages']
+          _(sent_messages.count).must_equal 1
+          _(sent_messages.first.data_points.count).must_equal 1
+          _(sent_messages.first.data_points.first.value).must_equal 1
+          sent_messages_attributes = sent_messages.first.data_points.first.attributes
+          _(sent_messages_attributes['messaging.system']).must_equal 'sidekiq'
+          _(sent_messages_attributes['messaging.destination.name']).must_equal 'default' # FIXME: newer semconv specifies this key
+          _(sent_messages_attributes['messaging.operation.name']).must_equal 'create'
+        end
+      end
+    end
   end
 end
diff --git a/...q/test/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware_test.rb b/...q/test/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware_test.rb
@@ -11,23 +11,24 @@
 describe OpenTelemetry::Instrumentation::Sidekiq::Middlewares::Server::TracerMiddleware do
   let(:instrumentation) { OpenTelemetry::Instrumentation::Sidekiq::Instrumentation.instance }
   let(:exporter) { EXPORTER }
-  let(:metrics_exporter) { METRICS_EXPORTER }
   let(:spans) { exporter.finished_spans }
   let(:enqueuer_span) { spans.first }
   let(:job_span) { spans.last }
   let(:root_span) { spans.find { |s| s.parent_span_id == OpenTelemetry::Trace::INVALID_SPAN_ID } }
   let(:config) { {} }
 
-  with_metrics do
+  with_metrics_sdk do
     let(:metric_snapshots) do
-      METRICS_EXPORTER.tap(&:pull).metric_snapshots.group_by(&:name)
+      METRICS_EXPORTER.tap(&:pull)
+                      .metric_snapshots.select { |snapshot| snapshot.data_points.any? }
+                      .group_by(&:name)
     end
   end
 
   before do
     instrumentation.install(config)
     exporter.reset
-    with_metrics { metrics_exporter.tap(&:pull).reset }
+    reset_metrics_exporter
   end
 
   after do
@@ -59,35 +60,52 @@
       _(job_span.events[1].name).must_equal('enqueued_at')
     end
 
-    it 'metrics processing', with_metrics: true do
-      job_id = SimpleJob.perform_async
-      SimpleJob.drain
+    with_metrics_sdk do
+      # FIXME: still seeing order-dependent failure here
+      it 'yields no metrics if config is not set' do
+        _(OpenTelemetry::Instrumentation::Sidekiq::Instrumentation.instance.metrics_enabled?).must_equal false
+        job_id = SimpleJob.perform_async
+        SimpleJob.drain
 
-      queue_latency = metric_snapshots['messaging.queue.latency']
-      _(queue_latency.count).must_equal 1
-      _(queue_latency.first.data_points.count).must_equal 1
-      queue_latency_attributes = queue_latency.first.data_points.first.attributes
-      _(queue_latency_attributes['messaging.system']).must_equal 'sidekiq'
-      _(queue_latency_attributes['messaging.destination.name']).must_equal 'default' # FIXME: newer semconv specifies this key
-
-      process_duration = metric_snapshots['messaging.process.duration']
-      _(process_duration.count).must_equal 1
-      _(process_duration.first.data_points.count).must_equal 1
-      process_duration_attributes = process_duration.first.data_points.first.attributes
-      _(process_duration_attributes['messaging.system']).must_equal 'sidekiq'
-      _(process_duration_attributes['messaging.operation.name']).must_equal 'process'
-      _(process_duration_attributes['messaging.destination.name']).must_equal 'default'
-
-      process_duration_data_point = process_duration.first.data_points.first
-      _(process_duration_data_point.count).must_equal 1
-
-      consumed_messages = metric_snapshots['messaging.client.consumed.messages']
-      _(consumed_messages.count).must_equal 1
-      _(consumed_messages.first.data_points.count).must_equal 1
-      consumed_messages_attributes = queue_latency.first.data_points.first.attributes
-      _(consumed_messages_attributes['messaging.system']).must_equal 'sidekiq'
-      _(consumed_messages_attributes['messaging.destination.name']).must_equal 'default' # FIXME: newer semconv specifies this key
-      _(consumed_messages.first.data_points.first.value).must_equal 1
+        _(exporter.finished_spans.size).must_equal 2
+        _(metric_snapshots).must_be_empty
+      end
+
+      describe 'with metrics enabled' do
+        let(:config) { { metrics: true } }
+
+        it 'metrics processing' do
+          _(OpenTelemetry::Instrumentation::Sidekiq::Instrumentation.instance.metrics_enabled?).must_equal true
+          job_id = SimpleJob.perform_async
+          SimpleJob.drain
+
+          queue_latency = metric_snapshots['messaging.queue.latency']
+          _(queue_latency.count).must_equal 1
+          _(queue_latency.first.data_points.count).must_equal 1
+          queue_latency_attributes = queue_latency.first.data_points.first.attributes
+          _(queue_latency_attributes['messaging.system']).must_equal 'sidekiq'
+          _(queue_latency_attributes['messaging.destination.name']).must_equal 'default' # FIXME: newer semconv specifies this key
+
+          process_duration = metric_snapshots['messaging.process.duration']
+          _(process_duration.count).must_equal 1
+          _(process_duration.first.data_points.count).must_equal 1
+          process_duration_attributes = process_duration.first.data_points.first.attributes
+          _(process_duration_attributes['messaging.system']).must_equal 'sidekiq'
+          _(process_duration_attributes['messaging.operation.name']).must_equal 'process'
+          _(process_duration_attributes['messaging.destination.name']).must_equal 'default'
+
+          process_duration_data_point = process_duration.first.data_points.first
+          _(process_duration_data_point.count).must_equal 1
+
+          consumed_messages = metric_snapshots['messaging.client.consumed.messages']
+          _(consumed_messages.count).must_equal 1
+          _(consumed_messages.first.data_points.count).must_equal 1
+          consumed_messages_attributes = queue_latency.first.data_points.first.attributes
+          _(consumed_messages_attributes['messaging.system']).must_equal 'sidekiq'
+          _(consumed_messages_attributes['messaging.destination.name']).must_equal 'default' # FIXME: newer semconv specifies this key
+          _(consumed_messages.first.data_points.first.value).must_equal 1
+        end
+      end
     end
 
     it 'traces when enqueued with Active Job' do

diff --git a/instrumentation/sidekiq/test/test_helper.rb b/instrumentation/sidekiq/test/test_helper.rb
@@ -56,6 +56,22 @@ def sdk_loaded?
   end
 end
 
+# NOTE: this isn't currently used, but it may be useful to fully reset state between tests
+def reset_meter_provider
+  return unless LoadedMetricsFeatures.sdk_loaded?
+
+  resource = OpenTelemetry.meter_provider.resource
+  OpenTelemetry.meter_provider = OpenTelemetry::SDK::Metrics::MeterProvider.new(resource: resource)
+  OpenTelemetry.meter_provider.add_metric_reader(METRICS_EXPORTER)
+end
+
+def reset_metrics_exporter
+  return unless LoadedMetricsFeatures.sdk_loaded?
+
+  METRICS_EXPORTER.pull
+  METRICS_EXPORTER.reset
+end
+
 if LoadedMetricsFeatures.sdk_loaded?
   METRICS_EXPORTER = OpenTelemetry::SDK::Metrics::Export::InMemoryMetricPullExporter.new
   OpenTelemetry.meter_provider.add_metric_reader(METRICS_EXPORTER)
@@ -70,13 +86,22 @@ def self.prepended(base)
     base.extend(self)
   end
 
-  def with_metrics
+  def with_metrics_sdk
     yield if LoadedMetricsFeatures.sdk_loaded?
   end
 
-  def it(desc = 'anonymous', with_metrics: false, &block)
-    return super(desc, &block) unless with_metrics
-    return unless LoadedMetricsFeatures.sdk_loaded?
+  # FIXME: unclear if this is ever needed
+  def without_metrics_sdk
+    yield unless LoadedMetricsFeatures.sdk_loaded?
+  end
+
+  def it(desc = 'anonymous', with_metrics_sdk: false, without_metrics_sdk: false, &block)
+    return super(desc, &block) unless with_metrics_sdk || without_metrics_sdk
+
+    raise ArgumentError, 'without_metrics_sdk and with_metrics_sdk must be mutually exclusive' if without_metrics_sdk && with_metrics_sdk
+
+    return if with_metrics_sdk && !LoadedMetricsFeatures.sdk_loaded?
+    return if without_metrics_sdk && LoadedMetricsFeatures.sdk_loaded?
 
     super(desc, &block)
   end