-
Notifications
You must be signed in to change notification settings - Fork 250
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.
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
c5d670b
11c07ab
f8edb27
46a5505
c02ec91
1b8f73e
ff3b1f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -47,6 +48,8 @@ def update(increment, attributes, data_points) | |
nil | ||
) | ||
|
||
return if is_monotonic && increment < 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this suppose to be instance variable monotonic also need to check with aggregation_temporality
We also need to move this logic before There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think your logic is correct (also checked other implementation as your referenced). The specification is somewhat confusing, but
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my current implementation is very similar to the js one. Do you think there is something else that needs to be implemented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM; may need other maintainers take a look too. |
||
|
||
ndp.value += increment | ||
nil | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 } | ||
|
@@ -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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuan-cao-swi typically I would think of these kinds of attributes hidden behind predicates.
Would it not make sense to make the public interface of this attribute
monotonic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value in the protobuf is
is_monotonic
, so I think the goal was to match. I also found it jarring* with Ruby's predicates.opentelemetry-ruby/exporter/otlp/lib/opentelemetry/proto/metrics/v1/metrics_pb.rb
Line 11 in 289b4aa
But, I believe some agents (Js?) just reduce this to
monotonic
in the SDK, so there's precedent to go the other way.