address todos (#1832)

Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>

Author: zeitlinger

prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/ExporterOpenTelemetryProperties.java

@@ -4,7 +4,33 @@
 import java.util.Map;
 import javax.annotation.Nullable;
 
-// TODO: JavaDoc is currently only in OpenTelemetryExporter.Builder. Look there for reference.
+/**
+ * Properties for configuring the OpenTelemetry exporter.
+ *
+ * <p>These properties can be configured via {@code prometheus.properties}, system properties, or
+ * programmatically.
+ *
+ * <p>All properties are prefixed with {@code io.prometheus.exporter.opentelemetry}.
+ *
+ * <p>Available properties:
+ *
+ * <ul>
+ *   <li>{@code protocol} - OTLP protocol: {@code "grpc"} or {@code "http/protobuf"}
+ *   <li>{@code endpoint} - OTLP endpoint URL
+ *   <li>{@code headers} - HTTP headers for outgoing requests
+ *   <li>{@code intervalSeconds} - Export interval in seconds
+ *   <li>{@code timeoutSeconds} - Request timeout in seconds
+ *   <li>{@code serviceName} - Service name resource attribute
+ *   <li>{@code serviceNamespace} - Service namespace resource attribute
+ *   <li>{@code serviceInstanceId} - Service instance ID resource attribute
+ *   <li>{@code serviceVersion} - Service version resource attribute
+ *   <li>{@code resourceAttributes} - Additional resource attributes
+ * </ul>
+ *
+ * @see <a
+ *     href="https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/">OpenTelemetry
+ *     SDK Environment Variables</a>
+ */
 public class ExporterOpenTelemetryProperties {
 
   // See
@@ -153,6 +179,14 @@ public static class Builder {
 
     private Builder() {}
 
+    /**
+     * The OTLP protocol to use.
+     *
+     * <p>Supported values: {@code "grpc"} or {@code "http/protobuf"}.
+     *
+     * <p>See OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_protocol">OTEL_EXPORTER_OTLP_PROTOCOL</a>.
+     */
     public Builder protocol(String protocol) {
       if (!protocol.equals("grpc") && !protocol.equals("http/protobuf")) {
         throw new IllegalArgumentException(
@@ -162,17 +196,43 @@ public Builder protocol(String protocol) {
       return this;
     }
 
+    /**
+     * The OTLP endpoint to send metric data to.
+     *
+     * <p>The default depends on the protocol:
+     *
+     * <ul>
+     *   <li>{@code "grpc"}: {@code "http://localhost:4317"}
+     *   <li>{@code "http/protobuf"}: {@code "http://localhost:4318/v1/metrics"}
+     * </ul>
+     *
+     * <p>See OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_metrics_endpoint">OTEL_EXPORTER_OTLP_METRICS_ENDPOINT</a>.
+     */
     public Builder endpoint(String endpoint) {
       this.endpoint = endpoint;
       return this;
     }
 
-    /** Add a request header. Call multiple times to add multiple headers. */
+    /**
+     * Add an HTTP header to be applied to outgoing requests. Call multiple times to add multiple
+     * headers.
+     *
+     * <p>See OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_headers">OTEL_EXPORTER_OTLP_HEADERS</a>.
+     */
     public Builder header(String name, String value) {
       this.headers.put(name, value);
       return this;
     }
 
+    /**
+     * The interval between the start of two export attempts. Default is 60 seconds.
+     *
+     * <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>
+     * (which defaults to 60000 milliseconds), but specified in seconds rather than milliseconds.
+     */
     public Builder intervalSeconds(int intervalSeconds) {
       if (intervalSeconds <= 0) {
         throw new IllegalArgumentException(intervalSeconds + ": Expecting intervalSeconds > 0");
@@ -181,6 +241,13 @@ public Builder intervalSeconds(int intervalSeconds) {
       return this;
     }
 
+    /**
+     * The timeout for outgoing requests. Default is 10.
+     *
+     * <p>Like OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_metrics_timeout">OTEL_EXPORTER_OTLP_METRICS_TIMEOUT</a>,
+     * but in seconds rather than milliseconds.
+     */
     public Builder timeoutSeconds(int timeoutSeconds) {
       if (timeoutSeconds <= 0) {
         throw new IllegalArgumentException(timeoutSeconds + ": Expecting timeoutSeconds > 0");
@@ -189,26 +256,63 @@ public Builder timeoutSeconds(int timeoutSeconds) {
       return this;
     }
 
+    /**
+     * The {@code service.name} resource attribute.
+     *
+     * <p>If not explicitly specified, {@code client_java} will try to initialize it with a
+     * reasonable default, like the JAR file name.
+     *
+     * <p>See {@code service.name} in OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/specs/otel/resource/semantic_conventions/#service">Resource
+     * Semantic Conventions</a>.
+     */
     public Builder serviceName(String serviceName) {
       this.serviceName = serviceName;
       return this;
     }
 
+    /**
+     * The {@code service.namespace} resource attribute.
+     *
+     * <p>See {@code service.namespace} in OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/specs/otel/resource/semantic_conventions/#service-experimental">Resource
+     * Semantic Conventions</a>.
+     */
     public Builder serviceNamespace(String serviceNamespace) {
       this.serviceNamespace = serviceNamespace;
       return this;
     }
 
+    /**
+     * The {@code service.instance.id} resource attribute.
+     *
+     * <p>See {@code service.instance.id} in OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/specs/otel/resource/semantic_conventions/#service-experimental">Resource
+     * Semantic Conventions</a>.
+     */
     public Builder serviceInstanceId(String serviceInstanceId) {
       this.serviceInstanceId = serviceInstanceId;
       return this;
     }
 
+    /**
+     * The {@code service.version} resource attribute.
+     *
+     * <p>See {@code service.version} in OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/specs/otel/resource/semantic_conventions/#service-experimental">Resource
+     * Semantic Conventions</a>.
+     */
     public Builder serviceVersion(String serviceVersion) {
       this.serviceVersion = serviceVersion;
       return this;
     }
 
+    /**
+     * Add a resource attribute. Call multiple times to add multiple resource attributes.
+     *
+     * <p>See OpenTelemetry's <a
+     * href="https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration">OTEL_RESOURCE_ATTRIBUTES</a>.
+     */
     public Builder resourceAttribute(String name, String value) {
       this.resourceAttributes.put(name, value);
       return this;

prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java

@@ -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;
     }
 

prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/SlidingWindow.java

@@ -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> {
 

prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java

@@ -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(),

prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/HistogramTest.java

@@ -1423,7 +1423,7 @@ void testObserve() {
 
   @Test
   // See https://github.com/prometheus/client_java/issues/646
-  public void testNegativeAmount() {
+  void testNegativeAmount() {
     Histogram histogram =
         Histogram.builder()
             .name("histogram")
@@ -1565,6 +1565,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()

prometheus-metrics-exporter-opentelemetry/src/main/java/io/prometheus/metrics/exporter/opentelemetry/PrometheusMetricProducer.java

@@ -41,8 +41,14 @@ public PrometheusMetricProducer(
 
   @Override
   public Collection<MetricData> collectAllMetrics() {
-    // TODO: We could add a filter configuration for the OpenTelemetry exporter and call
-    // registry.scrape(filter) if a filter is configured, like in the Servlet exporter.
+    // Note: Currently all metrics from the registry are exported. To add metric filtering
+    // similar to the Servlet exporter, one could:
+    // 1. Add filter properties to ExporterOpenTelemetryProperties (allowedNames, excludedNames,
+    // etc.)
+    // 2. Convert these properties to a Predicate<String> using MetricNameFilter.builder()
+    // 3. Call registry.scrape(filter) instead of registry.scrape()
+    // OpenTelemetry also provides its own Views API for filtering and aggregation, which may be
+    // preferred for OpenTelemetry-specific deployments.
     MetricSnapshots snapshots = registry.scrape();
     Resource resourceWithTargetInfo = resource.merge(resourceFromTargetInfo(snapshots));
     InstrumentationScopeInfo scopeFromInfo = instrumentationScopeFromOtelScopeInfo(snapshots);