jerryshao opened a new pull request, #12946: URL: https://github.com/apache/gravitino/pull/12946
### What changes were proposed in this pull request? Timers and histograms in `MetricsSource` (used by `gravitino-server`, `gravitino-relational-store`, `iceberg-rest-server`, and catalog metrics) and `HttpServerMetricsSource`'s Jersey `@Timed` listener were backed by `com.codahale.metrics.SlidingTimeWindowArrayReservoir`, with a 60-second window by default (`gravitino.metrics.timeSlidingWindowSecs`). This PR switches both to `ExponentiallyDecayingReservoir` (dropwizard's own default reservoir), which decays sample weight over time instead of hard-expiring it. It also deprecates `gravitino.metrics.timeSlidingWindowSecs`, which is no longer consulted, and updates the config docs accordingly. ### Why are the changes needed? `SlidingTimeWindowArrayReservoir.getSnapshot()` calls `trim()` first, discarding any sample older than the window. If nothing landed inside the window, the snapshot is built from an empty array, and dropwizard's `UniformSnapshot` hard-codes `0.0`/`0` for `getMean()`/`getMax()`/`getValue(quantile)` when there are no values. `count`, however, comes from `Histogram`'s own separate, never-expiring `LongAdder`, so it stays accurate. In practice this means nearly every real API endpoint — anything not called at least once every 60 seconds — reports a nonzero request count but `mean`/`p95`/`p99`/`max` of exactly `0.0`, making `/metrics` unusable for latency alerting or slow-operation investigation. Only endpoints invoked more frequently than the window (e.g. health-check probes) showed correct data. Fix: #12944 ### Does this PR introduce _any_ user-facing change? Yes, an intentional one: the statistical semantics of exposed `mean`/`p95`/`p99`/`max` change from a strict "last N seconds" window to an exponentially-decayed distribution biased toward roughly the last 5 minutes. This is necessary to stop those values from going to zero between infrequent calls. `gravitino.metrics.timeSlidingWindowSecs` is deprecated and no longer affects reservoir behavior, but is kept (not removed) for backward compatibility. ### How was this patch tested? - Added `TestReservoirIdleBehavior`, a deterministic regression test (using an injectable `Clock`, no real sleeping) that reproduces the bug against the old reservoir (count survives, `max` goes to `0` after a simulated 61s idle period) and proves the new reservoir does not (count and `max` both survive the same idle period). - Ran the existing `core` metrics and `server-common` web test suites — all pass. -- 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]
