Remove time unit from rate aggregation #126497
Conversation
dnhatn
changed the title
dnhatn
force-pushed
the
remove-rate-unit
branch
2 times, most recently
from
14e3b97 to
286f847
Compare
dnhatn
added
:StorageEngine/TSDB
elasticsearchmachine
added
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| @@ -40,26 +49,26 @@ max_cost: double | |||
|
|
|||
| maxRateAndBytes | |||
| required_capability: metrics_command | |||
| TS k8s | STATS max(rate(network.total_bytes_in, 1minute)), max(network.bytes_in); | |||
| TS k8s | STATS max(60 * rate(network.total_bytes_in)), max(network.bytes_in); | |||
There was a problem hiding this comment.
I see the point, however I must say rate(network.total_bytes_in, 1minute) reads a little better, especially for longer intervals (such as day).
Should we remove it from the aggregator for simplicity, but have org.elasticsearch.xpack.esql.expression.SurrogateExpression to introduce an optional second argument for time window?
There was a problem hiding this comment.
In most cases we'd have something like:
TS metrics | STATS max(rate(foo, 1minute)) BY TBUCKET(1minute), hostname
i.e. the rate window matches the bucketing timespan. The second arg should be optional in this case. In the future, we want to support different values between the two; we'll think about the right syntax for that when time comes.
There was a problem hiding this comment.
Yes, that's why I proposed removing it now and reintroducing it once we have settled on the syntax.
| @@ -219,14 +219,14 @@ record RateKey(String cluster, String host) { | |||
| assertThat((double) values.get(0).get(0), closeTo(rates.stream().mapToDouble(d -> d).max().orElse(0.0), 0.1)); | |||
| assertThat((double) values.get(0).get(1), closeTo(rates.stream().mapToDouble(d -> d).min().orElse(0.0), 0.1)); | |||
| } | |||
| try (var resp = run("TS hosts | STATS max(rate(request_count)), avg(rate(request_count)), max(rate(request_count, 1minute))")) { | |||
| try (var resp = run("TS hosts | STATS max(rate(request_count)), avg(rate(request_count)), max(60 * rate(request_count))")) { | |||
There was a problem hiding this comment.
Nit: do we need the 60 * factor? It hurts readability somewhat, can we skip it and update results?
|
Thanks friends! |
Currently, the rate aggregation accepts two parameters: the first specifies the counter field, and the second specifies the time unit of the rate. The time unit parameter was introduced to conveniently compute requests per minute or per hour. However, this can be replaced easily - for example,
rate(field, 1minute)with60 * rate(field).This change removes the time unit parameter and reserves it for potential future usage, such as introducing a sliding window unit. If we decide on other options later, we can reintroduce it. Removing it now avoids breaking changes while the rate aggregation is not yet available.