diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index ae8aa60f4..7c6424f09 100644 --- 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/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/client/tracer_middleware_test.rb b/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/client/tracer_middleware_test.rb index a2de3d05d..25a5e6b2f 100644 --- a/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/client/tracer_middleware_test.rb +++ b/instrumentation/sidekiq/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/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware_test.rb b/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware_test.rb index 6fed53731..c0d51b715 100644 --- a/instrumentation/sidekiq/test/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware_test.rb +++ b/instrumentation/sidekiq/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 index 66f6be4d0..154064701 100644 --- 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