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.

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 SumAggregation #3390

Merged
merged 40 commits into from
Oct 13, 2023
Merged

Fix SumAggregation #3390

merged 40 commits into from
Oct 13, 2023

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jul 24, 2023

Fixes #3268

@aabmass this is the PR for the issue I mentioned in the last SIG, PTAL ✌️

@ocelotl ocelotl self-assigned this Jul 24, 2023
@ocelotl ocelotl requested a review from a team July 24, 2023 10:22
@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 24, 2023

This is a relevant example.

@ocelotl ocelotl force-pushed the issue_3268 branch 2 times, most recently from be26c92 to 0b6a4b6 Compare August 18, 2023 12:12
@ocelotl ocelotl requested a review from aabmass August 22, 2023 11:58
@ocelotl ocelotl marked this pull request as draft August 24, 2023 16:30
@ocelotl ocelotl changed the title Fix SumAggregation for delta temporality Fix SumAggregation Aug 31, 2023
@ocelotl ocelotl marked this pull request as ready for review August 31, 2023 17:38
@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 6, 2023

Moving here this comment from @aabmass:

That makes sense 👍 one other question, do we keep the oldest start time that had data in this case? I believe we should. Here's a scenario:

  • 1st collection at t1, a delta counter has value 5
  • 2nd collection at t2, the counter has None as no observations have been made
  • 3rd collection at t3, the counter has value 2.

Does the 3rd collection send the time/start_time as (t1, t3) or (t2, t3)?

@aabmass
Copy link
Member

aabmass commented Sep 6, 2023

@ocelotl

Does the 3rd collection send the time/start_time as (t1, t3) or (t2, t3)?

Looks like this PR saves the oldest previous time to get 1st option, is that right?

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 6, 2023

@ocelotl

Does the 3rd collection send the time/start_time as (t1, t3) or (t2, t3)?

Looks like this PR saves the oldest previous time to get 1st option, is that right?

Yes, that's right, but TBH, I'm not sure if we should do that. The first option would produce a point whose difference between start and end time is larger than "normal". If a periodic metric reader is used, it can be weird to have a point that is "longer" than a collection cycle, right?

@aabmass
Copy link
Member

aabmass commented Sep 6, 2023

I think this is the expected behavior, see here

For subsequent points in an unbroken sequence:

  • For points with delta aggregation temporality, the StartTimeUnixNano of each point matches the TimeUnixNano of the preceding point

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 7, 2023

I think this is the expected behavior, see here

For subsequent points in an unbroken sequence:

  • For points with delta aggregation temporality, the StartTimeUnixNano of each point matches the TimeUnixNano of the preceding point

All right, seems like we are doing the right thing, then. PTAL at the rest of the PR when you have a chance 👍

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 8, 2023

After discussing with @lzchen it looks like we also need to take into consideration the instrument monotonicity in collect.

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 8, 2023

This may help:

Here is one way of choosing the correct instrument:

  • I want to count something (by recording a delta value):
    • If the value is monotonically increasing (the delta value is always non-negative) - use a Counter.
    • If the value is NOT monotonically increasing (the delta value can be positive, negative or zero) - use an UpDownCounter.
  • I want to record or time something, and the statistics about this thing are likely to be meaningful - use a Histogram.
  • I want to measure something (by reporting an absolute value):
    • If the measurement values are non-additive, use an Asynchronous Gauge.
    • If the measurement values are additive:
      • If the value is monotonically increasing - use an Asynchronous Counter.
      • If the value is NOT monotonically increasing - use an Asynchronous UpDownCounter.

@ocelotl ocelotl enabled auto-merge (squash) October 13, 2023 23:08
@ocelotl ocelotl merged commit d054dff into open-telemetry:main Oct 13, 2023
110 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObservableCounter with AggregationTemporality.DELTA is incorrect
2 participants