This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new cf33ce8f6b [#12944] fix(core): Use ExponentiallyDecayingReservoir so
infrequent operations don't report zero duration (#12946)
cf33ce8f6b is described below
commit cf33ce8f6bf445bd1d8a7406d9f34f2dd853feca
Author: Jerry Shao <[email protected]>
AuthorDate: Tue Sep 8 16:38:01 2026 +0800
[#12944] fix(core): Use ExponentiallyDecayingReservoir so infrequent
operations don't report zero duration (#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
**Known limitation:** `ExponentiallyDecayingReservoir` mitigates this
but does not fully eliminate it. Its `rescale()` periodically rescales
each sample's decayed weight by `exp(-alpha * elapsedSeconds)` (default
`alpha=0.015`), and once that factor underflows to `0.0` in
double-precision arithmetic — after roughly 13.8 hours of inactivity —
it clears every sample outright, reproducing the same "count survives,
duration reads zero" symptom the old reservoir showed at 60 seconds.
This still represents a large improvement (60s → ~14h) and is expected
to be a non-issue for endpoints seeing regular production traffic; see
the discussion on this PR for context.
### 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, and infrequently-called operations keep reporting a real
duration for far longer (on the order of half a day, per the known
limitation above) instead of reading zero after 60 seconds of
inactivity. `gravitino.metrics.timeSlidingWindowSecs` is deprecated and
no longer affects reservoir behavior, but is kept (not removed) for
backward compatibility.
### How was this patch tested?
- `TestReservoirIdleBehavior`: a deterministic regression suite (using
an injectable `Clock`, no real sleeping) covering:
- The original bug reproduced against the old reservoir (count survives,
`max` goes to `0` after a simulated 61s idle period).
- The new reservoir surviving that same 61s idle period.
- The actual `MetricsSource.getTimer()`/`getHistogram()` production
methods (via a `MetricsSource` subclass overriding the new
`newReservoir()` seam), not just the raw reservoir classes, defaulting
to `ExponentiallyDecayingReservoir`.
- The known ~14h limitation reproduced through that same production
path, advancing the clock in 30s increments with periodic
`getSnapshot()` calls in between to mirror real Prometheus scraping.
- `TestHttpServerMetricsSource`: added a wiring test asserting the
Jersey listener's reservoir supplier produces an
`ExponentiallyDecayingReservoir` instance.
- Ran the existing `core` metrics and `server-common` web test suites —
all pass.
---------
Co-authored-by: Claude Sonnet 5 <[email protected]>
---
.../main/java/org/apache/gravitino/Configs.java | 10 +-
.../gravitino/metrics/source/MetricsSource.java | 41 ++---
.../metrics/source/TestReservoirIdleBehavior.java | 171 +++++++++++++++++++++
docs/gravitino-server-config.md | 6 +-
.../server/web/HttpServerMetricsSource.java | 10 +-
.../server/web/TestHttpServerMetricsSource.java | 31 ++++
6 files changed, 229 insertions(+), 40 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/Configs.java
b/core/src/main/java/org/apache/gravitino/Configs.java
index ed47ef8b5a..c638288c39 100644
--- a/core/src/main/java/org/apache/gravitino/Configs.java
+++ b/core/src/main/java/org/apache/gravitino/Configs.java
@@ -420,8 +420,16 @@ public class Configs {
public static final int DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS = 60;
public static final ConfigEntry<Integer> METRICS_TIME_SLIDING_WINDOW_SECONDS
=
new ConfigBuilder("gravitino.metrics.timeSlidingWindowSecs")
- .doc("The seconds of Gravitino metrics time sliding window")
+ .doc(
+ "The seconds of Gravitino metrics time sliding window. No longer
used: timers and "
+ + "histograms use an ExponentiallyDecayingReservoir, which
decays samples "
+ + "instead of expiring them on a fixed window, so
infrequently-invoked "
+ + "operations keep reporting a real duration for far longer
(on the order of "
+ + "half a day with the default decay rate) instead of
reading zero after 60 "
+ + "seconds of inactivity. An operation idle for longer than
that will still "
+ + "eventually report a duration of zero.")
.version(ConfigConstants.VERSION_0_5_1)
+ .deprecated()
.intConf()
.createWithDefault(DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS);
diff --git
a/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
b/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
index 1d079f49ae..e920d8efaa 100644
--- a/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
+++ b/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
@@ -20,16 +20,13 @@
package org.apache.gravitino.metrics.source;
import com.codahale.metrics.Counter;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
+import com.codahale.metrics.Reservoir;
import com.codahale.metrics.Timer;
-import java.util.concurrent.TimeUnit;
-import org.apache.gravitino.Config;
-import org.apache.gravitino.Configs;
-import org.apache.gravitino.GravitinoEnv;
/**
* MetricsSource provides utilities to collect specified kind metrics, all
metrics must create with
@@ -47,19 +44,10 @@ public abstract class MetricsSource {
public static final String JVM_METRIC_NAME = "jvm";
private final MetricRegistry metricRegistry;
private final String metricsSourceName;
- private final int timeSlidingWindowSeconds;
protected MetricsSource(String name) {
this.metricsSourceName = name;
metricRegistry = new MetricRegistry();
- Config config = GravitinoEnv.getInstance().config();
- if (config != null) {
- this.timeSlidingWindowSeconds =
- config.get(Configs.METRICS_TIME_SLIDING_WINDOW_SECONDS).intValue();
- } else {
- // Couldn't get config when testing
- this.timeSlidingWindowSeconds =
Configs.DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS;
- }
}
/**
@@ -107,12 +95,7 @@ public abstract class MetricsSource {
* @return a new or pre-existing Histogram
*/
public Histogram getHistogram(String name) {
- return this.metricRegistry.histogram(
- name,
- () ->
- new Histogram(
- new SlidingTimeWindowArrayReservoir(
- getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
+ return this.metricRegistry.histogram(name, () -> new
Histogram(newReservoir()));
}
/**
@@ -122,12 +105,7 @@ public abstract class MetricsSource {
* @return a new or pre-existing Timer
*/
public Timer getTimer(String name) {
- return this.metricRegistry.timer(
- name,
- () ->
- new Timer(
- new SlidingTimeWindowArrayReservoir(
- getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
+ return this.metricRegistry.timer(name, () -> new Timer(newReservoir()));
}
/**
@@ -140,7 +118,14 @@ public abstract class MetricsSource {
return this.metricRegistry.meter(name);
}
- protected int getTimeSlidingWindowSeconds() {
- return timeSlidingWindowSeconds;
+ /**
+ * Creates the {@link Reservoir} backing new timers and histograms.
Package-visible so tests can
+ * override it to inject a reservoir with a controllable {@link
com.codahale.metrics.Clock}
+ * instead of exercising real wall-clock time.
+ *
+ * @return a new {@link ExponentiallyDecayingReservoir}
+ */
+ protected Reservoir newReservoir() {
+ return new ExponentiallyDecayingReservoir();
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/metrics/source/TestReservoirIdleBehavior.java
b/core/src/test/java/org/apache/gravitino/metrics/source/TestReservoirIdleBehavior.java
new file mode 100644
index 0000000000..a9098cbe2f
--- /dev/null
+++
b/core/src/test/java/org/apache/gravitino/metrics/source/TestReservoirIdleBehavior.java
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.metrics.source;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
+import com.codahale.metrics.Timer;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression test for {@code MetricsSource} recording a nonzero count but a
zero duration once an
+ * endpoint or method hasn't been invoked within the reservoir's window: {@link
+ * SlidingTimeWindowArrayReservoir} discards every sample once it falls
outside its fixed window, so
+ * {@link Timer} and {@link Histogram} then report count-only. {@link
+ * MetricsSource#getTimer(String)} and {@link
MetricsSource#getHistogram(String)} now use {@link
+ * ExponentiallyDecayingReservoir} instead, which decays sample weight over
time rather than
+ * expiring it on a fixed window, so infrequently-invoked operations keep
reporting a real duration
+ * for far longer (on the order of half a day with the default decay rate)
than the old 60-second
+ * window.
+ *
+ * <p>This is a mitigation, not a complete fix: {@link
ExponentiallyDecayingReservoir} rescales its
+ * samples' decayed weights periodically, and once that scaling factor
underflows to zero in
+ * double-precision arithmetic (in practice, after roughly 13.8 hours of
inactivity with the default
+ * decay rate), it clears every sample outright, reproducing the same "count
survives, duration
+ * reads zero" symptom the old reservoir showed at 60 seconds. {@link
+ * #metricsSourceTimerEventuallyZeroesOutAfterVeryLongIdlePeriod()} documents
that known, unresolved
+ * boundary through the actual {@link MetricsSource} timer-creation path.
+ */
+public class TestReservoirIdleBehavior {
+
+ /** A {@link Clock} whose tick/time only move when {@link #advance} is
called. */
+ private static class ManualClock extends Clock {
+ private final AtomicLong nanos = new AtomicLong(0);
+
+ @Override
+ public long getTick() {
+ return nanos.get();
+ }
+
+ @Override
+ public long getTime() {
+ return TimeUnit.NANOSECONDS.toMillis(nanos.get());
+ }
+
+ void advance(long duration, TimeUnit unit) {
+ nanos.addAndGet(unit.toNanos(duration));
+ }
+ }
+
+ /**
+ * A {@code MetricsSource} whose reservoirs are driven by a {@link
ManualClock} instead of real
+ * wall-clock time, so idle periods can be simulated deterministically while
still going through
+ * the production {@link MetricsSource#getTimer(String)}/{@link
+ * MetricsSource#getHistogram(String)} methods rather than constructing
reservoirs directly.
+ */
+ private static class ManualClockMetricsSource extends MetricsSource {
+ private final ManualClock clock;
+
+ ManualClockMetricsSource(ManualClock clock) {
+ super("test-manual-clock");
+ this.clock = clock;
+ }
+
+ @Override
+ protected Reservoir newReservoir() {
+ return new ExponentiallyDecayingReservoir(1028, 0.015, clock);
+ }
+ }
+
+ @Test
+ void slidingTimeWindowReservoirZeroesOutAfterIdlePeriod() {
+ ManualClock clock = new ManualClock();
+ Histogram histogram =
+ new Histogram(new SlidingTimeWindowArrayReservoir(60,
TimeUnit.SECONDS, clock));
+
+ for (int i = 1; i <= 10; i++) {
+ histogram.update(i * 100L);
+ }
+ Assertions.assertTrue(histogram.getSnapshot().getMax() > 0);
+
+ // Simulate the endpoint going quiet for longer than the 60-second window.
+ clock.advance(61, TimeUnit.SECONDS);
+
+ Assertions.assertEquals(10, histogram.getCount(), "count must survive the
idle period");
+ Assertions.assertEquals(
+ 0,
+ histogram.getSnapshot().getMax(),
+ "this is the bug: the old reservoir silently zeroes out duration stats
once idle");
+ }
+
+ @Test
+ void exponentiallyDecayingReservoirSurvivesShortIdlePeriod() {
+ ManualClock clock = new ManualClock();
+ Histogram histogram = new Histogram(new
ExponentiallyDecayingReservoir(1028, 0.015, clock));
+
+ for (int i = 1; i <= 10; i++) {
+ histogram.update(i * 100L);
+ }
+ Assertions.assertTrue(histogram.getSnapshot().getMax() > 0);
+
+ // Same idle period the old reservoir already failed at; the new one must
not go to zero here.
+ clock.advance(61, TimeUnit.SECONDS);
+
+ Assertions.assertEquals(10, histogram.getCount());
+ Assertions.assertTrue(
+ histogram.getSnapshot().getMax() > 0,
+ "ExponentiallyDecayingReservoir must keep reporting real duration data
after a short "
+ + "idle period instead of collapsing to zero");
+ }
+
+ @Test
+ void metricsSourceTimerUsesExponentiallyDecayingReservoirByDefault() {
+ MetricsSource metricsSource = new ManualClockMetricsSource(new
ManualClock());
+ Timer timer = metricsSource.getTimer("op.total");
+ timer.update(100, TimeUnit.MILLISECONDS);
+
+ // Exercised through the real production method, not a
directly-constructed reservoir, so this
+ // would fail if MetricsSource ever reverted to
SlidingTimeWindowArrayReservoir.
+ Assertions.assertTrue(timer.getSnapshot().getMax() > 0);
+ }
+
+ @Test
+ void metricsSourceTimerEventuallyZeroesOutAfterVeryLongIdlePeriod() {
+ ManualClock clock = new ManualClock();
+ MetricsSource metricsSource = new ManualClockMetricsSource(clock);
+ Timer timer = metricsSource.getTimer("op.total");
+
+ for (int i = 1; i <= 10; i++) {
+ timer.update(i * 100L, TimeUnit.MILLISECONDS);
+ }
+ Assertions.assertTrue(timer.getSnapshot().getMax() > 0);
+
+ // Advance in increments, calling getSnapshot() along the way, to mirror
periodic Prometheus
+ // scraping every 30s during the idle period rather than a single large
jump.
+ for (int i = 0; i < 1680; i++) { // 1680 * 30s = 14h
+ clock.advance(30, TimeUnit.SECONDS);
+ timer.getSnapshot();
+ }
+
+ Assertions.assertEquals(10, timer.getCount(), "count must still survive
the idle period");
+ Assertions.assertEquals(
+ 0,
+ timer.getSnapshot().getMax(),
+ "known limitation: ExponentiallyDecayingReservoir's rescale() clears
every sample once "
+ + "its decayed weight underflows to zero, so a sufficiently long
idle period (roughly "
+ + "half a day with the default decay rate) still reproduces the
original bug");
+ }
+}
diff --git a/docs/gravitino-server-config.md b/docs/gravitino-server-config.md
index bc05401fc2..8b0725382f 100644
--- a/docs/gravitino-server-config.md
+++ b/docs/gravitino-server-config.md
@@ -248,9 +248,9 @@ line with catalog count, plugin count, and query
concurrency: `-Xms4g -Xmx4g
#### Metrics
-| Configuration Item | Description
| Default Value |
-|-------------------------------------------|------------------------------------------------------|---------------|
-| `gravitino.metrics.timeSlidingWindowSecs` | Width in seconds of the metrics
time sliding window. | `60` |
+| Configuration Item | Description
| Default Value |
+|-------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|
+| `gravitino.metrics.timeSlidingWindowSecs` | Deprecated, no longer used.
Duration timers and histograms now use an exponentially-decaying reservoir
instead of a fixed time window, so infrequently-invoked operations keep
reporting a real duration for far longer (on the order of half a day) instead
of reading zero after 60 seconds of inactivity. Operations idle longer than
that will still eventually report a duration of zero. | `60` |
### Storing Metadata
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpServerMetricsSource.java
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpServerMetricsSource.java
index 437cb30090..386f5b9bdc 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpServerMetricsSource.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpServerMetricsSource.java
@@ -20,9 +20,8 @@
package org.apache.gravitino.server.web;
import com.codahale.metrics.Clock;
-import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import
com.codahale.metrics.jersey2.InstrumentedResourceMethodApplicationListener;
-import java.util.concurrent.TimeUnit;
import org.apache.gravitino.metrics.MetricNames;
import org.apache.gravitino.metrics.source.MetricsSource;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
@@ -34,12 +33,7 @@ public class HttpServerMetricsSource extends MetricsSource {
super(name);
resourceConfig.register(
new InstrumentedResourceMethodApplicationListener(
- getMetricRegistry(),
- Clock.defaultClock(),
- false,
- () ->
- new SlidingTimeWindowArrayReservoir(
- getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
+ getMetricRegistry(), Clock.defaultClock(), false,
ExponentiallyDecayingReservoir::new));
// Register QueuedThreadPool specific metrics with instance checks
ThreadPool threadPool = server.getThreadPool();
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpServerMetricsSource.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpServerMetricsSource.java
index 7ed99b8219..8b36a7336a 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpServerMetricsSource.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpServerMetricsSource.java
@@ -20,6 +20,7 @@
package org.apache.gravitino.server.web;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
@@ -27,9 +28,13 @@ import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
import
com.codahale.metrics.jersey2.InstrumentedResourceMethodApplicationListener;
+import java.lang.reflect.Field;
+import java.util.function.Supplier;
import org.apache.gravitino.metrics.MetricNames;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.eclipse.jetty.util.thread.ThreadPool;
@@ -203,6 +208,32 @@ public class TestHttpServerMetricsSource {
assertNotNull(listener);
}
+ @Test
+ void testReservoirSupplierProducesExponentiallyDecayingReservoir() throws
Exception {
+ // Regression guard: HttpServerMetricsSource wires an
ExponentiallyDecayingReservoir supplier
+ // into the Jersey listener so infrequently-called endpoints don't report
a zero duration.
+ // This reaches into the listener's private field because
dropwizard-metrics-jersey2 exposes
+ // no getter for it; it would fail if the supplier ever reverted to a
+ // SlidingTimeWindowArrayReservoir (or any other reservoir type).
+ QueuedThreadPool realQueuedThreadPool = new QueuedThreadPool(10, 5, 60000);
+ when(mockJettyServer.getThreadPool()).thenReturn(realQueuedThreadPool);
+
+ metricsSource = new HttpServerMetricsSource("test-server",
mockResourceConfig, mockJettyServer);
+
+ ArgumentCaptor<InstrumentedResourceMethodApplicationListener> captor =
+
ArgumentCaptor.forClass(InstrumentedResourceMethodApplicationListener.class);
+ verify(mockResourceConfig).register(captor.capture());
+
+ Field reservoirSupplierField =
+
InstrumentedResourceMethodApplicationListener.class.getDeclaredField("reservoirSupplier");
+ reservoirSupplierField.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ Supplier<Reservoir> reservoirSupplier =
+ (Supplier<Reservoir>) reservoirSupplierField.get(captor.getValue());
+
+ assertInstanceOf(ExponentiallyDecayingReservoir.class,
reservoirSupplier.get());
+ }
+
@Test
void testMetricsSourceName() {
// Arrange
