Skip to content

Commit

Permalink
fix: add is_monotonic flag to sum
Browse files Browse the repository at this point in the history
  • Loading branch information
garoazinha committed Jan 15, 2025
1 parent baeda39 commit c5d670b
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 7 deletions.
7 changes: 5 additions & 2 deletions metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ module Aggregation
# Contains the implementation of the Sum aggregation
# https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#sum-aggregation
class Sum
attr_reader :aggregation_temporality
attr_reader :aggregation_temporality, :is_monotonic

def initialize(aggregation_temporality: ENV.fetch('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE', :delta))
def initialize(aggregation_temporality: ENV.fetch('OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE', :delta), is_monotonic: false)
# TODO: the default should be :cumulative, see issue #1555
@aggregation_temporality = aggregation_temporality.to_sym
@is_monotonic = is_monotonic
end

def collect(start_time, end_time, data_points)
Expand Down Expand Up @@ -47,6 +48,8 @@ def update(increment, attributes, data_points)
nil
)

return if is_monotonic && increment < 0

ndp.value += increment
nil
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def add(increment, attributes: {})
private

def default_aggregation
OpenTelemetry::SDK::Metrics::Aggregation::Sum.new
OpenTelemetry::SDK::Metrics::Aggregation::Sum.new(is_monotonic: true)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def add(amount, attributes: {})
private

def default_aggregation
OpenTelemetry::SDK::Metrics::Aggregation::Sum.new
OpenTelemetry::SDK::Metrics::Aggregation::Sum.new(is_monotonic: false)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ module State
:data_points, # Hash{Hash{String => String, Numeric, Boolean, Array<String, Numeric, Boolean>} => Numeric}
:aggregation_temporality, # Symbol
:start_time_unix_nano, # Integer nanoseconds since Epoch
:time_unix_nano) # Integer nanoseconds since Epoch
:time_unix_nano, # Integer nanoseconds since Epoch
:is_monotonic) # Boolean
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ def update(value, attributes)

def aggregate_metric_data(start_time, end_time, aggregation: nil)
aggregator = aggregation || @default_aggregation
is_monotonic = aggregator.respond_to?(:is_monotonic) && aggregator.is_monotonic

MetricData.new(
@name,
@description,
Expand All @@ -77,7 +79,8 @@ def aggregate_metric_data(start_time, end_time, aggregation: nil)
aggregator.collect(start_time, end_time, @data_points),
aggregator.aggregation_temporality,
start_time,
end_time
end_time,
is_monotonic
)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@

describe OpenTelemetry::SDK::Metrics::Aggregation::Sum do
let(:data_points) { {} }
let(:sum_aggregation) { OpenTelemetry::SDK::Metrics::Aggregation::Sum.new(aggregation_temporality: aggregation_temporality) }
let(:sum_aggregation) { OpenTelemetry::SDK::Metrics::Aggregation::Sum.new(aggregation_temporality: aggregation_temporality, is_monotonic: is_monotonic) }
let(:aggregation_temporality) { :delta }
let(:is_monotonic) { false }

# Time in nano
let(:start_time) { (Time.now.to_r * 1_000_000_000).to_i }
Expand Down Expand Up @@ -58,6 +59,14 @@
_(ndps[1].attributes).must_equal('foo' => 'bar')
end

it 'aggregates and collects negative values' do
sum_aggregation.update(1, {}, data_points)
sum_aggregation.update(-2, {}, data_points)

ndps = sum_aggregation.collect(start_time, end_time, data_points)
_(ndps[0].value).must_equal(-1)
end

it 'does not aggregate between collects' do
sum_aggregation.update(1, {}, data_points)
sum_aggregation.update(2, {}, data_points)
Expand Down Expand Up @@ -94,4 +103,17 @@
_(ndps[0].value).must_equal(4)
end
end

describe 'when sum type is monotonic' do
let(:aggregation_temporality) { :not_delta }
let(:is_monotonic) { true }

it 'does not allow negative values to accumulate' do
sum_aggregation.update(1, {}, data_points)
sum_aggregation.update(-2, {}, data_points)
ndps = sum_aggregation.collect(start_time, end_time, data_points)

_(ndps[0].value).must_equal(1)
end
end
end

0 comments on commit c5d670b

Please sign in to comment.