jerryshao commented on code in PR #12946:
URL: https://github.com/apache/gravitino/pull/12946#discussion_r3954850530
##########
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:
Good catch — verified independently: `exp(-0.015·t)` underflows
`Double.MIN_VALUE` at `t ≈ 49626s ≈ 13.78h`, matching your repro almost exactly.
Pushed a follow-up commit that:
- Narrows the PR description and the
`gravitino.metrics.timeSlidingWindowSecs` doc string to state the actual
retention boundary (~13.8h) instead of implying duration data never disappears.
- Adds `MetricsSource.newReservoir()`, a protected seam so tests can inject
a `Clock`-controllable reservoir, and rewrites the regression tests to go
through the real `getTimer()`/`getHistogram()` production methods instead of
constructing reservoir classes directly — including a new test that reproduces
your ~14h finding through that same path, advancing the clock in 30s increments
with periodic `getSnapshot()` calls in between to mirror actual Prometheus
scraping.
- Adds an equivalent wiring test for `HttpServerMetricsSource`'s Jersey
listener reservoir supplier.
Agree this is a known, accepted limitation rather than something to chase
further in this PR — happy to file a follow-up issue if we ever want a stronger
guarantee (e.g. `SlidingWindowReservoir(N)`), but as discussed that trades away
the time-decay weighting for count-based retention instead.
--
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]
