From 044336d9f8956e1d8115b22deaf32ac2000ebc2d Mon Sep 17 00:00:00 2001 From: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com> Date: Wed, 16 Oct 2024 13:06:02 -0700 Subject: [PATCH 1/3] fix: Coerce aggregation_temporality to symbol (#1741) 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 | 3 ++- .../sdk/metrics/aggregation/sum.rb | 2 +- .../explicit_bucket_histogram_test.rb | 21 +++++++++++++++++++ .../sdk/metrics/aggregation/sum_test.rb | 21 +++++++++++++++++++ 4 files changed, 45 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 53ab4ae12..0d357e1d4 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 @@ -25,7 +25,8 @@ def initialize( boundaries: DEFAULT_BOUNDARIES, record_min_max: true ) - @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 f9e98d3e9..c2771b38e 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 end def collect(start_time, end_time, data_points) 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 52ae01926..4666ce37e 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 @@ -22,6 +22,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, {}, data_points) 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 18e07ec32..66e7667ec 100644 --- a/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb +++ b/metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb @@ -15,6 +15,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, {}, data_points) ndp = sum_aggregation.collect(start_time, end_time, data_points)[0] From cb05fa505bab68eef187018d9a248735e6625b0f Mon Sep 17 00:00:00 2001 From: Xuan <112967240+xuan-cao-swi@users.noreply.github.com> Date: Wed, 16 Oct 2024 17:20:59 -0400 Subject: [PATCH 2/3] Update Instrument validation and move to SDK (#1735) Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com> --- .../lib/opentelemetry/metrics/meter.rb | 13 ---- .../test/opentelemetry/metrics/meter_test.rb | 57 ++++++----------- .../lib/opentelemetry/sdk/metrics/meter.rb | 13 ++++ .../opentelemetry/sdk/metrics/meter_test.rb | 61 +++++++++++++++++++ 4 files changed, 92 insertions(+), 52 deletions(-) diff --git a/metrics_api/lib/opentelemetry/metrics/meter.rb b/metrics_api/lib/opentelemetry/metrics/meter.rb index 1335437c0..f44b58611 100644 --- a/metrics_api/lib/opentelemetry/metrics/meter.rb +++ b/metrics_api/lib/opentelemetry/metrics/meter.rb @@ -15,8 +15,6 @@ class Meter UP_DOWN_COUNTER = Instrument::UpDownCounter.new OBSERVABLE_UP_DOWN_COUNTER = Instrument::ObservableUpDownCounter.new - NAME_REGEX = /\A[a-zA-Z][-.\w]{0,62}\z/ - private_constant(:COUNTER, :OBSERVABLE_COUNTER, :HISTOGRAM, :OBSERVABLE_GAUGE, :UP_DOWN_COUNTER, :OBSERVABLE_UP_DOWN_COUNTER) DuplicateInstrumentError = Class.new(OpenTelemetry::Error) @@ -56,23 +54,12 @@ def create_observable_up_down_counter(name, callback:, unit: nil, description: n private def create_instrument(kind, name, unit, description, callback) - raise InstrumentNameError if name.nil? - raise InstrumentNameError if name.empty? - raise InstrumentNameError unless NAME_REGEX.match?(name) - raise InstrumentUnitError if unit && (!unit.ascii_only? || unit.size > 63) - raise InstrumentDescriptionError if description && (description.size > 1023 || !utf8mb3_encoding?(description.dup)) - @mutex.synchronize do OpenTelemetry.logger.warn("duplicate instrument registration occurred for instrument #{name}") if @instrument_registry.include? name @instrument_registry[name] = yield end end - - def utf8mb3_encoding?(string) - string.force_encoding('UTF-8').valid_encoding? && - string.each_char { |c| return false if c.bytesize >= 4 } - end end end end diff --git a/metrics_api/test/opentelemetry/metrics/meter_test.rb b/metrics_api/test/opentelemetry/metrics/meter_test.rb index 493c373b5..ec9b53e6e 100644 --- a/metrics_api/test/opentelemetry/metrics/meter_test.rb +++ b/metrics_api/test/opentelemetry/metrics/meter_test.rb @@ -7,11 +7,6 @@ require 'test_helper' describe OpenTelemetry::Metrics::Meter do - INSTRUMENT_NAME_ERROR = OpenTelemetry::Metrics::Meter::InstrumentNameError - INSTRUMENT_UNIT_ERROR = OpenTelemetry::Metrics::Meter::InstrumentUnitError - INSTRUMENT_DESCRIPTION_ERROR = OpenTelemetry::Metrics::Meter::InstrumentDescriptionError - DUPLICATE_INSTRUMENT_ERROR = OpenTelemetry::Metrics::Meter::DuplicateInstrumentError - let(:meter_provider) { OpenTelemetry::Metrics::MeterProvider.new } let(:meter) { meter_provider.meter('test-meter') } @@ -24,50 +19,34 @@ end end - it 'instrument name must not be nil' do - _(-> { meter.create_counter(nil) }).must_raise(INSTRUMENT_NAME_ERROR) - end - - it 'instument name must not be an empty string' do - _(-> { meter.create_counter('') }).must_raise(INSTRUMENT_NAME_ERROR) - end - - it 'instrument name must have an alphabetic first character' do - _(meter.create_counter('one_counter')) - _(-> { meter.create_counter('1_counter') }).must_raise(INSTRUMENT_NAME_ERROR) - end - - it 'instrument name must not exceed 63 character limit' do - long_name = 'a' * 63 - meter.create_counter(long_name) - _(-> { meter.create_counter(long_name + 'a') }).must_raise(INSTRUMENT_NAME_ERROR) + it 'test create_counter' do + counter = meter.create_counter('test') + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Counter) end - it 'instrument name must belong to alphanumeric characters, _, ., and -' do - meter.create_counter('a_-..-_a') - _(-> { meter.create_counter('a@') }).must_raise(INSTRUMENT_NAME_ERROR) - _(-> { meter.create_counter('a!') }).must_raise(INSTRUMENT_NAME_ERROR) + it 'test create_histogram' do + counter = meter.create_histogram('test') + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Histogram) end - it 'instrument unit must be ASCII' do - _(-> { meter.create_counter('a_counter', unit: 'á') }).must_raise(INSTRUMENT_UNIT_ERROR) + it 'test create_up_down_counter' do + counter = meter.create_up_down_counter('test') + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::UpDownCounter) end - it 'instrument unit must not exceed 63 characters' do - long_unit = 'a' * 63 - meter.create_counter('a_counter', unit: long_unit) - _(-> { meter.create_counter('b_counter', unit: long_unit + 'a') }).must_raise(INSTRUMENT_UNIT_ERROR) + it 'test create_observable_counter' do + counter = meter.create_observable_counter('test', callback: -> {}) + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::ObservableCounter) end - it 'instrument description must be utf8mb3' do - _(-> { meter.create_counter('a_counter', description: '💩'.dup) }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) - _(-> { meter.create_counter('b_counter', description: "\xc2".dup) }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) + it 'test create_observable_gauge' do + counter = meter.create_observable_gauge('test', callback: -> {}) + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::ObservableGauge) end - it 'instrument description must not exceed 1023 characters' do - long_description = 'a' * 1023 - meter.create_counter('a_counter', description: long_description) - _(-> { meter.create_counter('b_counter', description: long_description + 'a') }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) + it 'test create_observable_up_down_counter' do + counter = meter.create_observable_up_down_counter('test', callback: -> {}) + _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::ObservableUpDownCounter) end end end diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb index 8db3ea992..1307817ec 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb @@ -11,6 +11,8 @@ module SDK module Metrics # {Meter} is the SDK implementation of {OpenTelemetry::Metrics::Meter}. class Meter < OpenTelemetry::Metrics::Meter + NAME_REGEX = /\A[a-zA-Z][-.\w]{0,62}\z/ + # @api private # # Returns a new {Meter} instance. @@ -34,6 +36,12 @@ def add_metric_reader(metric_reader) end def create_instrument(kind, name, unit, description, callback) + raise InstrumentNameError if name.nil? + raise InstrumentNameError if name.empty? + raise InstrumentNameError unless NAME_REGEX.match?(name) + raise InstrumentUnitError if unit && (!unit.ascii_only? || unit.size > 63) + raise InstrumentDescriptionError if description && (description.size > 1023 || !utf8mb3_encoding?(description.dup)) + super do case kind when :counter then OpenTelemetry::SDK::Metrics::Instrument::Counter.new(name, unit, description, @instrumentation_scope, @meter_provider) @@ -45,6 +53,11 @@ def create_instrument(kind, name, unit, description, callback) end end end + + def utf8mb3_encoding?(string) + string.force_encoding('UTF-8').valid_encoding? && + string.each_char { |c| return false if c.bytesize >= 4 } + end end end end diff --git a/metrics_sdk/test/opentelemetry/sdk/metrics/meter_test.rb b/metrics_sdk/test/opentelemetry/sdk/metrics/meter_test.rb index 6a6cba2fd..e8415d8d3 100644 --- a/metrics_sdk/test/opentelemetry/sdk/metrics/meter_test.rb +++ b/metrics_sdk/test/opentelemetry/sdk/metrics/meter_test.rb @@ -58,4 +58,65 @@ _(instrument).must_be_instance_of OpenTelemetry::SDK::Metrics::Instrument::ObservableUpDownCounter end end + + describe 'creating an instrument' do + INSTRUMENT_NAME_ERROR = OpenTelemetry::Metrics::Meter::InstrumentNameError + INSTRUMENT_UNIT_ERROR = OpenTelemetry::Metrics::Meter::InstrumentUnitError + INSTRUMENT_DESCRIPTION_ERROR = OpenTelemetry::Metrics::Meter::InstrumentDescriptionError + DUPLICATE_INSTRUMENT_ERROR = OpenTelemetry::Metrics::Meter::DuplicateInstrumentError + + it 'duplicate instrument registration logs a warning' do + OpenTelemetry::TestHelpers.with_test_logger do |log_stream| + meter.create_counter('a_counter') + meter.create_counter('a_counter') + _(log_stream.string).must_match(/duplicate instrument registration occurred for instrument a_counter/) + end + end + + it 'instrument name must not be nil' do + _(-> { meter.create_counter(nil) }).must_raise(INSTRUMENT_NAME_ERROR) + end + + it 'instument name must not be an empty string' do + _(-> { meter.create_counter('') }).must_raise(INSTRUMENT_NAME_ERROR) + end + + it 'instrument name must have an alphabetic first character' do + _(meter.create_counter('one_counter')) + _(-> { meter.create_counter('1_counter') }).must_raise(INSTRUMENT_NAME_ERROR) + end + + it 'instrument name must not exceed 63 character limit' do + long_name = 'a' * 63 + meter.create_counter(long_name) + _(-> { meter.create_counter(long_name + 'a') }).must_raise(INSTRUMENT_NAME_ERROR) + end + + it 'instrument name must belong to alphanumeric characters, _, ., and -' do + meter.create_counter('a_-..-_a') + _(-> { meter.create_counter('a@') }).must_raise(INSTRUMENT_NAME_ERROR) + _(-> { meter.create_counter('a!') }).must_raise(INSTRUMENT_NAME_ERROR) + end + + it 'instrument unit must be ASCII' do + _(-> { meter.create_counter('a_counter', unit: 'á') }).must_raise(INSTRUMENT_UNIT_ERROR) + end + + it 'instrument unit must not exceed 63 characters' do + long_unit = 'a' * 63 + meter.create_counter('a_counter', unit: long_unit) + _(-> { meter.create_counter('b_counter', unit: long_unit + 'a') }).must_raise(INSTRUMENT_UNIT_ERROR) + end + + it 'instrument description must be utf8mb3' do + _(-> { meter.create_counter('a_counter', description: '💩'.dup) }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) + _(-> { meter.create_counter('b_counter', description: "\xc2".dup) }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) + end + + it 'instrument description must not exceed 1023 characters' do + long_description = 'a' * 1023 + meter.create_counter('a_counter', description: long_description) + _(-> { meter.create_counter('b_counter', description: long_description + 'a') }).must_raise(INSTRUMENT_DESCRIPTION_ERROR) + end + end end From 4c9f7e1a942663856cade347d84cff67ac4f0c3d Mon Sep 17 00:00:00 2001 From: Xuan <112967240+xuan-cao-swi@users.noreply.github.com> Date: Wed, 16 Oct 2024 17:33:56 -0400 Subject: [PATCH 3/3] fix: add warning if invalid meter name given (#1734) Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com> --- metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb index 1f3f86705..4538a88db 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb @@ -37,6 +37,7 @@ def meter(name, version: nil) OpenTelemetry.logger.warn 'calling MeterProvider#meter after shutdown, a noop meter will be returned.' OpenTelemetry::Metrics::Meter.new else + OpenTelemetry.logger.warn "Invalid meter name provided: #{name.nil? ? 'nil' : 'empty'} value" if name.to_s.empty? @mutex.synchronize { @meter_registry[Key.new(name, version)] ||= Meter.new(name, version, self) } end end