fix: adding is_monotonic flag to sum #1793
Conversation
garoazinha
force-pushed
the
issues-1783
branch
from
0c47cd2 to
bfcdafb
Compare
garoazinha
force-pushed
the
issues-1783
branch
from
bfcdafb to
c5d670b
Compare
garoazinha
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd and
kaylareopelle
as code owners
garoazinha
changed the title
| @@ -47,6 +48,8 @@ def update(increment, attributes, data_points) | |||
| nil | |||
| ) | |||
|
|
|||
| return if is_monotonic && increment < 0 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
| @@ -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.
I was going to ask that is there any spec to define the default for monotonic, but found the spec that explain it.
| @@ -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 | |||
There was a problem hiding this comment.
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
| ndps = sum_aggregation.collect(start_time, end_time, data_points) | ||
| _(ndps[0].value).must_equal(-1) | ||
| end | ||
|
|
There was a problem hiding this comment.
We may want more test case once the logic for mixing up temporality and monotonic is there
The is_monotonic flag for sum aggregation otlp model is required by the specs
The SDK should reject negative inputs to monotonic cumulative metrics, but it is not required to.
Implementations such as python and go appear to not enforce monotonicity, but the C++ and js implementations do.
js impl
C++ impl
Issue: #1783