Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: adding is_monotonic flag to sum #1793

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+62 −8
Open

fix: adding is_monotonic flag to sum #1793

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5d670b
fix: add is_monotonic flag to sum
garoazinha Jan 15, 2025
11c07ab
add monotonic flags to otlp exporter
garoazinha Jan 15, 2025
f8edb27
base is_monotonic on metric_data
garoazinha Jan 15, 2025
46a5505
dry and test
garoazinha Jan 15, 2025
c02ec91
implement suggestions
garoazinha Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/metrics_exporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,8 @@ def as_otlp_metrics(metrics)
aggregation_temporality: as_otlp_aggregation_temporality(metrics.aggregation_temporality),
data_points: metrics.data_points.map do |ndp|
number_data_point(ndp)
end
end,
is_monotonic: metrics.is_monotonic
)
)

Expand Down
24 changes: 24 additions & 0 deletions exporter/otlp-metrics/test/opentelemetry/exporter/otlp/metrics/metrics_exporter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,9 @@
counter = meter.create_counter('test_counter', unit: 'smidgen', description: 'a small amount of something')
counter.add(5, attributes: { 'foo' => 'bar' })

up_down_counter = meter.create_up_down_counter('test_up_down_counter', unit: 'smidgen', description: 'a small amount of something')
up_down_counter.add(5, attributes: { 'foo' => 'bar' })

histogram = meter.create_histogram('test_histogram', unit: 'smidgen', description: 'a small amount of something')
histogram.record(10, attributes: { 'oof' => 'rab' })

Expand Down Expand Up @@ -623,6 +626,27 @@
exemplars: nil
)
],
is_monotonic: true,
aggregation_temporality: Opentelemetry::Proto::Metrics::V1::AggregationTemporality::AGGREGATION_TEMPORALITY_DELTA
)
),
Opentelemetry::Proto::Metrics::V1::Metric.new(
name: 'test_up_down_counter',
description: 'a small amount of something',
unit: 'smidgen',
sum: Opentelemetry::Proto::Metrics::V1::Sum.new(
data_points: [
Opentelemetry::Proto::Metrics::V1::NumberDataPoint.new(
attributes: [
Opentelemetry::Proto::Common::V1::KeyValue.new(key: 'foo', value: Opentelemetry::Proto::Common::V1::AnyValue.new(string_value: 'bar'))
],
as_int: 5,
start_time_unix_nano: 1_699_593_427_329_946_585,
time_unix_nano: 1_699_593_427_329_946_586,
exemplars: nil
)
],
is_monotonic: false,
aggregation_temporality: Opentelemetry::Proto::Metrics::V1::AggregationTemporality::AGGREGATION_TEMPORALITY_DELTA
)
),
Expand Down
7 changes: 5 additions & 2 deletions metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/sum.rb
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this suppose to be instance variable @is_monotonic?

monotonic also need to check with aggregation_temporality

For delta monotonic sums, this means the reader SHOULD expect non-negative values.
For cumulative monotonic sums, this means the reader SHOULD expect values that are not less than the previous value.

We also need to move this logic before ndp = data_points[attributes] || data_points[attributes] = NumberDataPoint.new; because update doesn't return anything; it will update direct on data_points hash.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checking if increment is negative fulfills the specs for cumulative sums. for delta sums it might be the case that we need to check if the sum (value + increment) is negative? only checking if increment is negative ensures the sum value is never negative but might reject some valid increments. is my logic correct?

Copy link
Contributor

@xuan-cao-swi xuan-cao-swi Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checking if increment is negative fulfills the specs for cumulative sums.

Yes, I think your logic is correct (also checked other implementation as your referenced).

The specification is somewhat confusing, but In this case of metrics, this means the sum is nominally increasing should reduce confusion. (Now I think the specification is actually wrong regarding "for delta" and "for cum," and they should be swapped.)

for delta sums it might be the case that we need to check if the sum (value + increment) is negative?

For delta sum, it's kind tricky because the sum will be zeroed depends on when the instrument's value got exported. I see other implementations don't care about it. I would suggest follow their approach.


ndp.value += increment
nil
end
Expand Down
2 changes: 1 addition & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/instrument/counter.rb
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ask that is there any spec to define the default for monotonic, but found the spec that explain it.

end
end
end
Expand Down
2 changes: 1 addition & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/instrument/up_down_counter.rb
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_data.rb
View file
Open in desktop
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
5 changes: 4 additions & 1 deletion metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_stream.rb
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return false if aggregator doesn't respond to is_monotonic. Should we use nil instead of false?

is_monotonic = aggregator.respond_to?(:is_monotonic) ? aggregator.is_monotonic : nil


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
24 changes: 23 additions & 1 deletion metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/sum_test.rb
View file
Open in desktop
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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want more test case once the logic for mixing up temporality and monotonic is there

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
Loading