-
Notifications
You must be signed in to change notification settings - Fork 598
fix: add validation not to accept negative values for monotonic counters #3260
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3260 +/- ##
======================================
Coverage 80.8% 80.8%
======================================
Files 129 129
Lines 23203 23330 +127
======================================
+ Hits 18750 18863 +113
- Misses 4453 4467 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
31cb1f0 to
4adf9b8
Compare
| fn call(&self, measurement: T, attrs: &[KeyValue]) { | ||
| // Validate monotonic counter increment is non-negative | ||
| if self.monotonic && measurement < T::default() { | ||
| otel_warn!( |
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.
I am not sure if the idea of producing one log per invalid metric measurement is a good idea. For now, let's keep it to debug.
A better idea could be to have an Sdk internal metric, that'd track all measurement_drops... We can discuss this as a future improvement.
| /// Counters are used to measure values that only increase over time, such as the number | ||
| /// of requests received, bytes sent, or errors encountered. Only non-negative values | ||
| /// should be recorded. Negative values violate the monotonic property and will be | ||
| /// dropped by the SDK with a warning. |
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.
while its true that the spec says API should not validate this, and instead it is upto the implementations (i.e SDK) to do this validation, the SDK spec does not explicitly state what should it do when the value is negative. I'd love to get a spec clarification before implementing this.
I did take this route long ago (open-telemetry/opentelemetry-dotnet#2519) but never got the time to clarify if this is the right behavior to do in the sdk.
Fixes #3037
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes