[query] Optional value decrease tolerance in M3TSZ decoder #3876
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3876 +/- ##
======================================
Coverage 57.0% 57.0%
======================================
Files 552 552
Lines 63263 63279 +16
======================================
+ Hits 36062 36099 +37
+ Misses 23996 23966 -30
- Partials 3205 3214 +9
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| !first && currValue < prevValue && currValue > prevValue*(1-decreaseTolerance) { | ||
| currValue = prevValue | ||
| } | ||
| it.curr.Value = currValue |
There was a problem hiding this comment.
nit: to me a more readable approach would be:
decreaseTolerance, toleranceUntil := it.opts.ValueDecreaseTolerance()
if decreaseTolerance > 0 {
// do new logic
} else {
it.curr.Value = currValue
}
I guess it's not idiomatic Go code, but it clearly separates 2 code paths - when tolerance is set and when it is not.
| @@ -53,6 +53,9 @@ type options struct { | |||
| iStreamReaderSizeM3TSZ int | |||
| iStreamReaderSizeProto int | |||
| metrics Metrics | |||
|
|
|||
| valueDecreaseTolerance float64 | |||
| valueDecreaseToleranceUntil xtime.UnixNano | |||
There was a problem hiding this comment.
nit: do you think maybe wrapping these under struct would be better? And make it optional. So that if its nil then decrease tolerance logic is not applied? Otherwise if its specified then it must not be 0 for both fields.
| currValue := convertFromIntFloat(it.intVal, it.mult) | ||
| decreaseTolerance, toleranceUntil := it.opts.ValueDecreaseTolerance() | ||
| if decreaseTolerance > 0 && it.curr.TimestampNanos.Before(toleranceUntil) && | ||
| !first && currValue < prevValue && currValue > prevValue*(1-decreaseTolerance) { |
There was a problem hiding this comment.
is there some advantage of using prevValue*(1-decreaseTolerance) and not simply delta? Like if current value is smaller then prev value by 0.00001 we say it's the same value?
With multiplication depending on how large the value is different can be bigger. Maybe that's the intention?
There was a problem hiding this comment.
Absolute delta would not work when your actual values themselves are smaller than that delta.
| SetValueDecreaseTolerance(value float64) Options | ||
|
|
||
| // SetValueDecreaseToleranceUntil sets the timestamp (exclusive) until which the tolerance applies. | ||
| SetValueDecreaseToleranceUntil(value xtime.UnixNano) Options |
There was a problem hiding this comment.
Why do we need to have a timestamp until the tolerance applies? Does this mean that it should be set on each iteration?
There was a problem hiding this comment.
This can be set to the date when #3872 is deployed.
| it.curr.Value = convertFromIntFloat(it.intVal, it.mult) | ||
| prevValue := it.curr.Value | ||
| currValue := convertFromIntFloat(it.intVal, it.mult) | ||
| decreaseTolerance, toleranceUntil := it.opts.ValueDecreaseTolerance() |
There was a problem hiding this comment.
nit: You may want to allow toleranceUntil to be zero and if so then always apply? Just thinking instead of needing to set it to MaxTime or something if you want it on for good.
e.g.
(toleranceUntil.IsZero() || it.curr.TimestampNanos.Before(toleranceUntil))
There was a problem hiding this comment.
Can be set to something like 2099-12-31 for such use cases. Don't want to add any more conditionals there.
What this PR does / why we need it:
Allows setting relative decrease tolerance up to which value decreases are eliminated while decoding time series data.
Special notes for your reviewer:
See #3872.
Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE
Does this PR require updating code package or user-facing documentation?:
NONE