Merged
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Contributor
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.
prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java
Outdated
Show resolved
Hide resolved
...trics-config/src/main/java/io/prometheus/metrics/config/ExporterOpenTelemetryProperties.java
Outdated
Show resolved
Hide resolved
prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/HistogramTest.java
Show resolved
Hide resolved
jaydeluca
reviewed
Feb 2, 2026
Comment on lines
23
to
29
| * observations. Potential optimizations include: | ||
| * | ||
| * <ul> | ||
| * <li>Using {@link java.util.concurrent.locks.ReadWriteLock} to allow concurrent observations | ||
| * <li>Using lock-free data structures with {@link java.util.concurrent.atomic atomic} operations | ||
| * <li>Implementing a lock-free ring buffer with striped buckets | ||
| * </ul> |
Collaborator
There was a problem hiding this comment.
maybe this one could be converted into an issue if we want to improve it, or we just remove the TODO comment entirely? I'm not sure the addition of this new comment is all that useful
Member
Author
There was a problem hiding this comment.
not mention that it's synchronized - or just leave out the optimizations?
Collaborator
There was a problem hiding this comment.
the "Potential optimizations include:" list. it looks like this geared towards people who might refactor the code, as opposed to someone using it?
jaydeluca
reviewed
Feb 5, 2026
| } | ||
|
|
||
| @Test | ||
| public void testNativeResetDuration() { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public void testNativeResetDuration() { | |
| void testNativeResetDuration() { |
| } | ||
|
|
||
| @Test | ||
| public void testNativeResetDurationNegativeValue() { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public void testNativeResetDurationNegativeValue() { | |
| void testNativeResetDurationNegativeValue() { |
| } | ||
|
|
||
| @Test | ||
| public void testNativeResetDurationZeroValue() { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public void testNativeResetDurationZeroValue() { | |
| void testNativeResetDurationZeroValue() { |
| } | ||
|
|
||
| @Test | ||
| public void testNativeResetDurationSubSecond() { |
Collaborator
There was a problem hiding this comment.
Suggested change
| public void testNativeResetDurationSubSecond() { | |
| void testNativeResetDurationSubSecond() { |
jaydeluca
approved these changes
Feb 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.