From c703e360714f8f4e8152e0a3cdf6757fdfb84b52 Mon Sep 17 00:00:00 2001 From: Percy Grunwald Date: Fri, 22 Nov 2024 11:05:46 -0800 Subject: [PATCH] Fix: handle float value in NumberDataPoint It's currently possible to have a `Counter` with a `Float` value, but this fails to export because the `NumberDataPoint` is always assumed to be an `Integer` for the purposes of ProtoBuf encoding. The `.proto` for a `NumberDataPoint` allows for a float value in the `as_double` key, so there should be no reason that we should not be able to support `Float` `Counter`s. This commit adds support for `Float` values in `NumberDataPoint`, which in turn adds support for `Float` `Counter`s. I also updated the documentation in the structs for both `NumberDataPoint.value` and `HistogramDataPoint.sum` to match the expected type based on the `.proto`. Adds a test to confirm that the metrics_exporter handles both `Integer` and `Float` values for a `NumberDataPoint`. --- .../exporter/otlp/metrics/metrics_exporter.rb | 13 ++++++++++--- .../otlp/metrics/metrics_exporter_test.rb | 17 +++++++++++++++++ .../metrics/aggregation/histogram_data_point.rb | 2 +- .../metrics/aggregation/number_data_point.rb | 2 +- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/metrics_exporter.rb b/exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/metrics_exporter.rb index a6150fefb..36705bbf7 100644 --- a/exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/metrics_exporter.rb +++ b/exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/metrics_exporter.rb @@ -284,13 +284,20 @@ def histogram_data_point(hdp) end def number_data_point(ndp) - Opentelemetry::Proto::Metrics::V1::NumberDataPoint.new( + args = { attributes: ndp.attributes.map { |k, v| as_otlp_key_value(k, v) }, - as_int: ndp.value, start_time_unix_nano: ndp.start_time_unix_nano, time_unix_nano: ndp.time_unix_nano, exemplars: ndp.exemplars # exemplars not implemented yet from metrics sdk - ) + } + + if ndp.value.is_a?(Float) + args[:as_double] = ndp.value + else + args[:as_int] = ndp.value + end + + Opentelemetry::Proto::Metrics::V1::NumberDataPoint.new(**args) end # may not need this diff --git a/exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics/metrics_exporter_test.rb b/exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics/metrics_exporter_test.rb index 93176ce33..6076a80a2 100644 --- a/exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics/metrics_exporter_test.rb +++ b/exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics/metrics_exporter_test.rb @@ -463,6 +463,23 @@ OpenTelemetry.logger = logger end + it 'is able to encode NumberDataPoint with Integer or Float value' do + stub_request(:post, 'http://localhost:4318/v1/metrics').to_return(status: 200) + + [1, 0.1234].each do |value| + ndp = OpenTelemetry::SDK::Metrics::Aggregation::NumberDataPoint.new + ndp.attributes = { 'a' => 'b' } + ndp.start_time_unix_nano = 0 + ndp.time_unix_nano = 0 + ndp.value = value + + metrics_data = create_metrics_data(data_points: [ndp]) + + result = exporter.export([metrics_data]) + _(result).must_equal(METRICS_SUCCESS) + end + end + it 'logs rpc.Status on bad request' do log_stream = StringIO.new logger = OpenTelemetry.logger diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/histogram_data_point.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/histogram_data_point.rb index 212c5ccd0..4ae361cc7 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/histogram_data_point.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/histogram_data_point.rb @@ -14,7 +14,7 @@ module Aggregation :start_time_unix_nano, # Integer nanoseconds since Epoch :time_unix_nano, # Integer nanoseconds since Epoch :count, # Integer count is the number of values in the population. Must be non-negative. - :sum, # Integer sum of the values in the population. If count is zero then this field then this field must be zero + :sum, # Float sum of the values in the population. If count is zero then this field then this field must be zero :bucket_counts, # optional Array[Integer] field contains the count values of histogram for each bucket. :explicit_bounds, # Array[Float] specifies buckets with explicitly defined bounds for values. :exemplars, # optional List of exemplars collected from measurements that were used to form the data point diff --git a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/number_data_point.rb b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/number_data_point.rb index ae6e37eac..f76de98a9 100644 --- a/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/number_data_point.rb +++ b/metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/number_data_point.rb @@ -11,7 +11,7 @@ module Aggregation NumberDataPoint = Struct.new(:attributes, # Hash{String => String, Numeric, Boolean, Array} :start_time_unix_nano, # Integer nanoseconds since Epoch :time_unix_nano, # Integer nanoseconds since Epoch - :value, # Integer + :value, # Numeric :exemplars) # optional List of exemplars collected from measurements that were used to form the data point end end