yuqi1129 commented on code in PR #12946:
URL: https://github.com/apache/gravitino/pull/12946#discussion_r3949803169
##########
core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java:
##########
@@ -108,11 +95,7 @@ public Counter getCounter(String name) {
*/
public Histogram getHistogram(String name) {
return this.metricRegistry.histogram(
- name,
- () ->
- new Histogram(
- new SlidingTimeWindowArrayReservoir(
- getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
+ name, () -> new Histogram(new ExponentiallyDecayingReservoir()));
Review Comment:
[P2] Long-idle operations still lose all duration samples
`ExponentiallyDecayingReservoir` improves the 61-second case, but the pinned
metrics-core 4.2.25 implementation still clears samples after a sufficiently
long idle period. Its rescaling code removes samples whose decayed weight
underflows to zero (or clears the reservoir when the scaling factor becomes
zero). This also affects the reservoir supplied to the Jersey listener in
`HttpServerMetricsSource`.
I reproduced this against the actual 4.2.25 dependency with an injected
`Clock`, recording one value of 1000:
```text
After 61 seconds: count=1, samples=1, max=1000, mean=1000, p99=1000
After 14 hours: count=1, samples=0, max=0, mean=0, p99=0
```
The same result occurs when `getSnapshot()` is called every 30 simulated
seconds throughout the idle period, so regular Prometheus scraping does not
prevent it. Daily or less frequent operations therefore still exhibit the issue
this PR says it eliminates. The new regression advances only 61 seconds and
misses this rescaling boundary.
Could we clarify the retention contract and cover this before merging? If
the requirement is to retain the last observed distribution regardless of idle
time, a bounded `SlidingWindowReservoir(N)` is a small alternative for both
creation paths: it retains the last N observations without time-based expiry,
at the cost of replacing exponential time weighting with observation-count
semantics. If exponential weighting is required, retaining the last non-empty
snapshot would need an explicit implementation instead. Otherwise, the
documentation and PR claim should be narrowed to the shorter-idle improvement.
Please add deterministic coverage for prolonged idle periods with regular
snapshots, followed by the first new observation. It would also help to cover
the actual MetricsSource/Jersey timer construction: the current new test
directly constructs the third-party reservoirs and would still pass if the
production wiring reverted.
Validation: the existing core metrics tests (19) and server-common web tests
(103) all passed; the long-idle reproduction above is additional coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
