address todos #1832
address todos #1832
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
There was a problem hiding this comment.
Pull request overview
Replaces several TODOs with clearer inline documentation/Javadoc and adds a small set of unit tests to cover Histogram.Builder#nativeResetDuration(...) validation/configuration.
Changes:
- Expanded TODO comments into explanatory notes (OpenTelemetry exporter filtering, Summary exemplars, SlidingWindow thread-safety/perf).
- Added tests for
nativeResetDuration(...)positive/zero/negative inputs. - Added class and builder-method Javadocs for
ExporterOpenTelemetryProperties.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| prometheus-metrics-exporter-opentelemetry/src/main/java/io/prometheus/metrics/exporter/opentelemetry/PrometheusMetricProducer.java | Replaces TODO with guidance on how filtering could be added in the future. |
| prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/HistogramTest.java | Adds tests around nativeResetDuration(...). |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java | Documents current behavior/limitations for Summary exemplars. |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/SlidingWindow.java | Improves class-level documentation about thread safety and performance. |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java | Removes a TODO related to missing reset-duration tests. |
| prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/ExporterOpenTelemetryProperties.java | Adds comprehensive Javadoc for properties and builder methods. |
Comments suppressed due to low confidence (1)
prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java:969
- nativeResetDurationSeconds is derived via unit.toSeconds(duration), which truncates sub-second values to 0. That means calls like nativeResetDuration(500, MILLISECONDS) silently disable resets despite passing the duration > 0 validation. Consider either rejecting durations that round to 0 seconds, documenting second-level granularity, or rounding up to at least 1 second.
public Builder nativeResetDuration(long duration, TimeUnit unit) {
if (duration <= 0) {
throw new IllegalArgumentException(duration + ": value > 0 expected");
}
nativeResetDurationSeconds = unit.toSeconds(duration);
return this;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // While exemplars are sampled during observe() calls (see lines 175, 188), | ||
| // 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. |
There was a problem hiding this comment.
The comment references specific source line numbers ("see lines 175, 188"), which will become stale as the file changes. Prefer referencing the relevant methods/fields (e.g., observe()/observeWithExemplar() and exemplarSampler) instead of absolute line numbers.
| // While exemplars are sampled during observe() calls (see lines 175, 188), | |
| // 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. | |
| // While exemplars are sampled during observe() and observeWithExemplar() via | |
| // exemplarSampler, 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. |
| /** | ||
| * The interval between the start of two export attempts. Default is 60000. | ||
| * | ||
| * <p>Like OpenTelemetry's <a | ||
| * href="https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#periodic-metric-reader">OTEL_METRIC_EXPORT_INTERVAL</a>, | ||
| * but in seconds rather than milliseconds. |
There was a problem hiding this comment.
The Javadoc says "Default is 60000" for an API that accepts seconds; that reads as 60000 seconds (~16.7h). If the intended default is OpenTelemetry's 60000ms (i.e., 60 seconds), update the wording/units to avoid misleading users.
| HistogramSnapshot snapshot = histogram.collect(); | ||
| assertThat(snapshot.getDataPoints()).hasSize(1); | ||
| assertThat(snapshot.getDataPoints().get(0).hasNativeHistogramData()).isTrue(); | ||
| } |
There was a problem hiding this comment.
This test doesn't assert anything specific about the configured reset duration; it would still pass if nativeResetDuration(...) were a no-op (nativeOnly() already yields native histogram data). Consider asserting the configured duration is propagated (e.g., via reflection on the built Histogram / MetricsProperties) or exercising the reset scheduling/behavior in a deterministic way.
No description provided.