Replace deprecated moving_avg by moving_fn aggregation #25386
Conversation
timroes
added
release_note:enhancement
Feature:Aggregations
|
Pinging @elastic/kibana-app |
💔 Build Failed |
|
Jenkins, test this |
This comment has been minimized.
This comment has been minimized.
💔 Build Failed |
|
Jenkins, test this |
💚 Build Succeeded |
There was a problem hiding this comment.
I've tested with exactly the same data on this PR and on 6.4
The visual results are different when we don't have continuous buckets on all intervals.
Changing the getValue to return null values instead of 0 will fix the problem. (also because 0 is anyway a value, and null means 'we cannot compute that value')
viz in 6.4
Viz in current PR
ps: you can see another intersting bug on 6.4 where the moving average metric, collapsed without a title on the editor :(
💚 Build Succeeded |
|
code lgtm. i tested the PR and compared with current master using the same data. i noticed some slight differences still; see below. on master it seems the moving avg starts at 0 at the beginning of the data set, which affects the first few points due to the size of the moving avg window. and in this PR there simply isn't a moving avg entry for the first point. it seems to me the behavior in this PR is what we would want anyway, but just wanted to highlight the differences. this pr master |
| * here, that preserves the null value, to be closer aligned with | ||
| * the previous implementation. | ||
| */ | ||
| return bucket[agg.id] ? bucket[agg.id].value : 0; |
There was a problem hiding this comment.
@timroes I don't think this is what we specified on our call.
The problem here is the following:
- the current default, 7.0,
gap_policyof the moving function is toskip. This means that the function add zeros to every null value on that window, and than return the average. It's not what you wrote here, that return a zero value for buckets with an empty value. A bucket is always a average representation of N values, if the average of these N values is 0 than the bucket value is 0, if not it will be some other number. If thegap_policyis configured toskip, than it will skip the bucket calculation, returning a null bucket, that doesn't have to be confused with a 0 value average.
This is your implementation (that use the skip gap policy)
This is the one with the line configured as we spoked:
return bucket[agg.id] ? bucket[agg.id].value : null;
As you can see the values are the same expect that you bring down the curve on the empty bucket, instead of leaving the curve to skip that bucket and continue to the next.
This is your implementation with inserting zeros
And this is my implementation with inserting zeros that are exactly the same as yours:
💔 Build Failed |
|
Looks like |
💚 Build Succeeded |
|
As @lukeelmers mentioned, we need to pick this up again. @markov00 I am a bit lost on our last decisions here. Do you still remember which implementation we aimed for now? |
|
Will be closing in favor of another PR, Marco is doing. |
Summary
Fixes the first part of #24702 (TSVB still needs to be converted)
This will replace the now deprecated
moving_avgaggregation by the newmoving_fnbut use the same script and window size that we used beforehand as default.QA: This PR should not change any functionality.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriatelyThis includes a feature addition or change that requires a release note and was labeled appropriately