Include unit support for time series rate aggregation #96605
Conversation
elasticsearchmachine added needs:triage 
salvatore-campagna added >enhancement :StorageEngine/TSDB 
| Pinging @elastic/es-analytics-geo (Team:Analytics) |
elasticsearchmachine added the Team:Analytics | Hi @salvatore-campagna, I've created a changelog YAML for you. |
salvatore-campagna changed the title | Hi @salvatore-campagna, I've updated the changelog YAML for you. |
| Will fix the issue with |
| @Override | ||
| public double value() { | ||
| return (endValue - startValue + resetCompensation) / (endTime - startTime); | ||
| long rateUnitSeconds = rateUnit == null |
There was a problem hiding this comment.
Can rateUnit be null? Looks like this change falls back to use Rounding.DateTimeUnit.SECOND_OF_MINUTE when no rate is specified.
There was a problem hiding this comment.
Yes it can be...after deserialization for instance...especially if deserializing from the previous version...right?
There was a problem hiding this comment.
On line 67 the rateUnit field is initialised with Rounding.DateTimeUnit.SECOND_OF_MINUTE if from older versions, right?
...nalytics/src/main/java/org/elasticsearch/xpack/analytics/rate/InternalResetTrackingRate.java Outdated Show resolved Hide resolved
| assertThat(reduced, instanceOf(Rate.class)); | ||
| assertThat(((Rate) reduced).getValue(), equalTo(0.01)); | ||
| public void testReductionSecond() { | ||
| testReduction(Rounding.DateTimeUnit.SECOND_OF_MINUTE, 0.01); |
There was a problem hiding this comment.
This is the existing test returning 0.01 when unit = second (the default).
All other tests just check the result multiplying by 60, 60, 24 and so on...
| this.startTimes = bigArrays().newLongArray(1, true); | ||
| this.endTimes = bigArrays().newLongArray(1, true); | ||
| this.resetCompensations = bigArrays().newDoubleArray(1, true); | ||
| this.rateUnit = rateUnit; |
There was a problem hiding this comment.
Maybe just do rateUnit == null ? Rounding.DateTimeUnit.SECOND_OF_MINUTE : rateUnit here?
| I do think the main branch need to be merged in and a new transport version needs to be added. |
Yes...but I prefer waiting right before merging otherwise it might end up that I need to do it multiple times just because someone else is changing it. |
salvatore-campagna added auto-backport-and-merge v8.8.2 and removed auto-backport-and-merge v8.8.2 labels 3 participants
Here we include support for the
unitparameter to the time series rate aggregation. Parsing the fieldwas already there, we just make use of it so that calculation of the rate is done using the right temporal
unit, instead of just using seconds. Before this change the rate was calculated using the number of milliseconds
at the denominator later scaled to seconds. Now we scale that interval to whatever unit the user provides as
unit. Supported values include:second,minute,hour,day,week,month,quarterandyear.We fix a bug too. Results were returned as
value / time interval (milliseconds)while all tests were written as ifresult was returned as
value / time interval (seconds). We need to multiply everything by 1000 (milliseconds in 1 second).Resolves #94630