address todos by zeitlinger · Pull Request #1832 · prometheus/client_java · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -962,11 +962,18 @@ public Builder nativeMaxNumberOfBuckets(int nativeMaxBuckets) {
* <p>Default is no reset.
*/
public Builder nativeResetDuration(long duration, TimeUnit unit) {
// TODO: reset interval isn't tested yet
if (duration <= 0) {
throw new IllegalArgumentException(duration + ": value > 0 expected");
}
nativeResetDurationSeconds = unit.toSeconds(duration);
long seconds = unit.toSeconds(duration);
if (seconds == 0) {
throw new IllegalArgumentException(
duration
+ " "
+ unit
+ ": duration must be at least 1 second. Sub-second durations are not supported.");
}
nativeResetDurationSeconds = seconds;
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,16 @@
* <p>It is implemented in a generic way so that 3rd party libraries can use it for implementing
* sliding windows.
*
* <p>TODO: The current implementation is {@code synchronized}. There is likely room for
* optimization.
* <p><b>Thread Safety:</b> This class uses coarse-grained {@code synchronized} methods for
* simplicity and correctness. All public methods ({@link #current()} and {@link #observe(double)})
* are synchronized, which ensures thread-safe access to the ring buffer and rotation logic.
*
* <p><b>Performance Note:</b> The synchronized approach may cause contention under high-frequency
* observations.
*
* <p>However, given that Summary metrics are less commonly used (Histogram is generally preferred),
* and the observation frequency is typically lower than Counter increments, the current
* implementation provides an acceptable trade-off between simplicity and performance.
*/
public class SlidingWindow<T> {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,13 @@ private void doObserve(double amount) {
private SummarySnapshot.SummaryDataPointSnapshot collect(Labels labels) {
return buffer.run(
expectedCount -> count.sum() == expectedCount,
// TODO Exemplars (are hard-coded as empty in the line below)
// Note: Exemplars are currently hard-coded as empty for Summary metrics.
// While exemplars are sampled during observe() and observeWithExemplar() calls
// via the exemplarSampler field, they are not included in the snapshot to maintain
// consistency with the buffering mechanism. The buffer.run() ensures atomic
// collection of count, sum, and quantiles. Adding exemplars would require
// coordination between the buffer and exemplarSampler, which could impact
// performance. Consider using Histogram instead if exemplars are needed.
() ->
new SummarySnapshot.SummaryDataPointSnapshot(
count.sum(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1384,7 +1384,7 @@ void testObserve() {

@Test
// See https://github.com/prometheus/client_java/issues/646
public void testNegativeAmount() {
void testNegativeAmount() {
Histogram histogram =
Histogram.builder()
.name("histogram")
Expand Down Expand Up @@ -1526,6 +1526,70 @@ void testObserveMultithreaded()
assertThat(executor.awaitTermination(5, TimeUnit.SECONDS)).isTrue();
}

@Test
void testNativeResetDuration() {
// Test that nativeResetDuration can be configured without error and the histogram
// functions correctly. The reset duration schedules internal reset behavior but
// is not directly observable in the snapshot.
Histogram histogram =
Histogram.builder()
.name("test_histogram_with_reset")
.nativeOnly()
.nativeResetDuration(24, TimeUnit.HOURS)
.build();

histogram.observe(1.0);
histogram.observe(2.0);
histogram.observe(3.0);

HistogramSnapshot snapshot = histogram.collect();
assertThat(snapshot.getDataPoints()).hasSize(1);
HistogramSnapshot.HistogramDataPointSnapshot dataPoint = snapshot.getDataPoints().get(0);
assertThat(dataPoint.hasNativeHistogramData()).isTrue();
assertThat(dataPoint.getCount()).isEqualTo(3);
assertThat(dataPoint.getSum()).isEqualTo(6.0);
}

@Test
void testNativeResetDurationNegativeValue() {
assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(
() ->
Histogram.builder()
.name("test_histogram")
.nativeOnly()
.nativeResetDuration(-1, TimeUnit.HOURS)
.build())
.withMessageContaining("value > 0 expected");
}

@Test
void testNativeResetDurationZeroValue() {
assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(
() ->
Histogram.builder()
.name("test_histogram")
.nativeOnly()
.nativeResetDuration(0, TimeUnit.HOURS)
.build())
.withMessageContaining("value > 0 expected");
}

@Test
void testNativeResetDurationSubSecond() {
// Sub-second durations should be rejected as they truncate to 0 seconds
assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(
() ->
Histogram.builder()
.name("test_histogram")
.nativeOnly()
.nativeResetDuration(500, TimeUnit.MILLISECONDS)
.build())
.withMessageContaining("duration must be at least 1 second");
}

private HistogramSnapshot.HistogramDataPointSnapshot getData(
Histogram histogram, String... labels) {
return histogram.collect().getDataPoints().stream()
Expand Down