From 6d9fb2aa9a4b8440d4860e87abd8193d581ab456 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 7 Oct 2024 09:25:54 -0700 Subject: [PATCH] fix: Coerce aggregation_temporality to symbol The OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE env var sets the temporality as a string. However, checks throughout the metrics SDK and the OTLP exporter expect the temporality to be a symbol. Now, when aggregations that use this env var are initialized, the temporality will be coerced into a symbol. --- .../aggregation/explicit_bucket_histogram.rb | 2 +- .../sdk/metrics/aggregation/sum.rb | 2 +- .../explicit_bucket_histogram_test.rb | 21 +++++++++++++++++++ .../sdk/metrics/aggregation/sum_test.rb | 21 +++++++++++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram.rb index 5af469b61..50862c86f 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram.rb @@ -26,7 +26,7 @@ def initialize( record_min_max: true ) @data_points = {} - @aggregation_temporality = aggregation_temporality + @aggregation_temporality = aggregation_temporality.to_sym @boundaries = boundaries && !boundaries.empty? ? boundaries.sort : nil @record_min_max = record_min_max end diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb index 16cbccc43..8973a90cc 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb @@ -15,7 +15,7 @@ class Sum def initialize(aggregation_temporality: ENV.fetch('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE', :delta)) # TODO: the default should be :cumulative, see issue #1555 - @aggregation_temporality = aggregation_temporality + @aggregation_temporality = aggregation_temporality.to_sym @data_points = {} end diff --git a/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram_test.rb b/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram_test.rb index 06c0e7b79..bace00d42 100644 --- a/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram_test.rb +++ b/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/explicit_bucket_histogram_test.rb @@ -21,6 +21,27 @@ let(:start_time) { (Time.now.to_r * 1_000_000_000).to_i } let(:end_time) { ((Time.now + 60).to_r * 1_000_000_000).to_i } + describe '#initialize' do + it 'defaults to the delta aggregation temporality' do + exp = OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :delta + end + + it 'sets parameters from the environment and converts them to symbols' do + exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do + OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new + end + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :potato + end + + it 'prefers explicit parameters rather than the environment and converts them to symbols' do + exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do + OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(aggregation_temporality: 'pickles') + end + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :pickles + end + end + describe '#collect' do it 'returns all the data points' do ebh.update(0, {}) diff --git a/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb b/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb index 3c0a3931e..86b728250 100644 --- a/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb +++ b/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb @@ -14,6 +14,27 @@ let(:start_time) { (Time.now.to_r * 1_000_000_000).to_i } let(:end_time) { ((Time.now + 60).to_r * 1_000_000_000).to_i } + describe '#initialize' do + it 'defaults to the delta aggregation temporality' do + exp = OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :delta + end + + it 'sets parameters from the environment and converts them to symbols' do + exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do + OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new + end + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :potato + end + + it 'prefers explicit parameters rather than the environment and converts them to symbols' do + exp = OpenTelemetry::TestHelpers.with_env('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE' => 'potato') do + OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(aggregation_temporality: 'pickles') + end + _(exp.instance_variable_get(:@aggregation_temporality)).must_equal :pickles + end + end + it 'sets the timestamps' do sum_aggregation.update(0, {}) ndp = sum_aggregation.collect(start_time, end_time)[0]