fix: add validation not to accept negative values for monotonic counters#3260
fix: add validation not to accept negative values for monotonic counters#3260taisho6339 wants to merge 4 commits intoopen-telemetry:mainfrom
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.
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.
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