[FLINK-30090] Support timespan for TimerGauges #21374
Conversation
| /** Circular array containing the history of values. */ | ||
| private final long[] values; | ||
| /** The index in the array for the current time. */ | ||
| private int time = 0; |
There was a problem hiding this comment.
I think idx would be a better name for this. Time is confusing because this actually gets reset to zero when the array is full.
| long totalTime = 0; | ||
| for (int i = 0; i < maxIndex; i++) { | ||
| totalTime += values[i]; | ||
| } |
There was a problem hiding this comment.
I appreciate the attention to detail here on properly calculating the totalTime based on the number of available observations :) Also, I very much prefer it over only releasing the Gauge once we have all observations. That way, this feature shouldn't cause any regressions because the Gauge as quickly as it does now but its accuracy will improve after a minute.
| this.timeSpanInSeconds = | ||
| Math.max( | ||
| timeSpanInSeconds - (timeSpanInSeconds % UPDATE_INTERVAL_SECONDS), | ||
| UPDATE_INTERVAL_SECONDS); |
There was a problem hiding this comment.
It would be kind of nice not having to adjust this but we are constrained by the UPDATE_INTERVAL_SECONDS which we can't easily change. So looks good.
|
@flinkbot run azure |
What is the purpose of the change
Allow TimerGauges to define a time window similar to how MeterViews work in Flink. This allows us to collect more relevant metrics without losing information based on the metrics collection interval
Brief change log
Verifying this change
Extended existing unit tests to cover different timespan.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation