-
Notifications
You must be signed in to change notification settings - Fork 932
Change GAUGE to be CUMULATIVE in deltaPreferred() #7634
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
Conversation
The committers listed above are authorized under a signed CLA. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7634 +/- ##
============================================
+ Coverage 89.99% 90.00% +0.01%
- Complexity 7079 7080 +1
============================================
Files 803 803
Lines 21412 21412
Branches 2086 2086
============================================
+ Hits 19269 19272 +3
+ Misses 1479 1477 -2
+ Partials 664 663 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I believe this behavior is intentional. Do you have evidence to the contrary? |
|
Although it's an unfortunate part of the data model, temporality is effectively irrelevant for gauge metric data points. They're just a measurement. They don't represent a delta over time nor do they represent a cumulative aggregation. Just a value. The first sentence here about temporality describes it as relevant for "additive quantities", which a gauge isn't. It also sounds like you might be using a synchronous gauge and expecting it to report/export data even when you haven't performed a measurement? Is that the case? |
Yes that is correct. I was expecting the Synchronous and Asynchronous GAUGE will be exported the same way (ie last set data to be exported always) and the only difference would be how the value is set (sync vs async). The 2 alternatives I considered to get there was
As 2 worked (emits last set data every export interval) and as Synchronous Gauge is added at a later point in time in OpenTelemetry, I wanted to check to see if this is by design or a latent bug. |
I defer to the maintainers, but I do think this is the intentional design.
If you haven't recorded a measurement, I think it would be a mistake for the sdk to continue recording the previous measurement. It sounds like you have your head around the situation, tho. 👍🏻 |
@jkwatson I was expecting the Synchronous and Asynchronous GAUGE will be exported the same way (ie last set data to be exported always) and the only difference would be how the value is set (sync vs async). Testing this PR locally helped me get there ie the last data set() for a synchronous gauge was exported every minute. So, wanted to check if its by design or a latent bug. |
Yep, I understand, but I was looking for evidence from somewhere (hopefully the specification) that this wasn't the intended behavior (as it is now). I don't want to go changing the behavior of this, when it might be what is intended, and, indeed, expected by the majority of users. |
|
Thanks @jkwatson and @breedx-splk. |
|
Thanks @m-nagarajan. For future researchers, we concluded in a slack thread that this section of the spec is what governs this behavior: https://github.com/open-telemetry/opentelemetry-specification/blob/4b630d89d43a07d426878866a22b91f7230c00d9/specification/metrics/sdk.md?plain=1#L1300-L1312 |


Problem:
In
AggregationTemporalitySelector#deltaPreferred():GAUGEseems to fallback to default i,eDELTA, which results in no data getting exported if aGAUGEinstrument is not updated in that export interval.Solution:
Make
GAUGEto beCUMULATIVEto consistently export the last set data.