fix: harden scrape edge cases

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

Author: zeitlinger

docs/apidiffs/current_vs_latest/prometheus-metrics-exporter-httpserver.txt

@@ -157,7 +157,11 @@ void testErrorHandling() throws IOException {
     start("error");
     Response response = scrape("GET", "");
     assertThat(response.status).isEqualTo(500);
-    assertThat(response.stringBody()).contains("Simulating an error.");
+    assertErrorResponseBody(response.stringBody());
+  }
+
+  protected void assertErrorResponseBody(String body) {
+    assertThat(body).contains("Simulating an error.");
   }
 
   @Test

integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/ExporterIT.java

@@ -1,10 +1,19 @@
 package io.prometheus.metrics.it.exporter.test;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import java.io.IOException;
 import java.net.URISyntaxException;
 
 class HttpServerIT extends ExporterIT {
   public HttpServerIT() throws IOException, URISyntaxException {
     super("exporter-httpserver-sample");
   }
+
+  @Override
+  protected void assertErrorResponseBody(String body) {
+    assertThat(body)
+        .isEqualTo("An internal error occurred while scraping metrics.\n")
+        .doesNotContain("Simulating an error.");
+  }
 }

integration-tests/it-exporter/it-exporter-test/src/test/java/io/prometheus/metrics/it/exporter/test/HttpServerIT.java

@@ -94,9 +94,8 @@ static ExporterPushgatewayProperties load(PropertySource propertySource)
     if (scheme != null) {
       if (!scheme.equals("http") && !scheme.equals("https")) {
         throw new PrometheusPropertiesException(
-            String.format(
-                "%s.%s: Illegal value. Expecting 'http' or 'https'. Found: %s",
-                PREFIX, SCHEME, scheme));
+            Util.invalidValueMessage(
+                PREFIX + "." + SCHEME, "Illegal value. Expecting 'http' or 'https'.", scheme));
       }
     }
 
@@ -119,10 +118,10 @@ static ExporterPushgatewayProperties load(PropertySource propertySource)
         return EscapingScheme.DOTS_ESCAPING;
       default:
         throw new PrometheusPropertiesException(
-            String.format(
-                "%s.%s: Illegal value. Expecting 'allow-utf-8', 'values', 'underscores', "
-                    + "or 'dots'. Found: %s",
-                PREFIX, ESCAPING_SCHEME, scheme));
+            Util.invalidValueMessage(
+                PREFIX + "." + ESCAPING_SCHEME,
+                "Illegal value. Expecting 'allow-utf-8', 'values', 'underscores', or 'dots'.",
+                scheme));
     }
   }
 

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

@@ -24,7 +24,7 @@ static Boolean loadBoolean(String prefix, String propertyName, PropertySource pr
       String fullKey = prefix.isEmpty() ? propertyName : prefix + "." + propertyName;
       if (!"true".equalsIgnoreCase(property) && !"false".equalsIgnoreCase(property)) {
         throw new PrometheusPropertiesException(
-            String.format("%s: Expecting 'true' or 'false'. Found: %s", fullKey, property));
+            invalidValueMessage(fullKey, "Expecting 'true' or 'false'.", property));
       }
       return Boolean.parseBoolean(property);
     }
@@ -88,7 +88,8 @@ static List<Double> loadDoubleList(
           }
         } catch (NumberFormatException e) {
           throw new PrometheusPropertiesException(
-              fullKey + "=" + property + ": Expecting comma separated list of double values");
+              invalidValueMessage(
+                  fullKey, "Expecting comma separated list of double values", property));
         }
       }
       return Arrays.asList(result);
@@ -130,7 +131,7 @@ static Integer loadInteger(String prefix, String propertyName, PropertySource pr
         return Integer.parseInt(property);
       } catch (NumberFormatException e) {
         throw new PrometheusPropertiesException(
-            fullKey + "=" + property + ": Expecting integer value");
+            invalidValueMessage(fullKey, "Expecting integer value", property));
       }
     }
     return null;
@@ -146,7 +147,7 @@ static Double loadDouble(String prefix, String propertyName, PropertySource prop
         return Double.parseDouble(property);
       } catch (NumberFormatException e) {
         throw new PrometheusPropertiesException(
-            fullKey + "=" + property + ": Expecting double value");
+            invalidValueMessage(fullKey, "Expecting double value", property));
       }
     }
     return null;
@@ -162,7 +163,7 @@ static Long loadLong(String prefix, String propertyName, PropertySource property
         return Long.parseLong(property);
       } catch (NumberFormatException e) {
         throw new PrometheusPropertiesException(
-            fullKey + "=" + property + ": Expecting long value");
+            invalidValueMessage(fullKey, "Expecting long value", property));
       }
     }
     return null;
@@ -197,4 +198,52 @@ static <T extends Number> void assertValue(
       throw new PrometheusPropertiesException(fullMessage);
     }
   }
+
+  static String invalidValueMessage(String fullKey, String message, String found) {
+    String separator = message.endsWith(".") ? " " : ". ";
+    return fullKey + ": " + message + separator + "Found: " + escape(found);
+  }
+
+  static String escape(String value) {
+    StringBuilder result = new StringBuilder(value.length() + 2);
+    result.append('"');
+    int maxLength = Math.min(value.length(), 100);
+    for (int i = 0; i < maxLength; i++) {
+      char c = value.charAt(i);
+      switch (c) {
+        case '\b':
+          result.append("\\b");
+          break;
+        case '\t':
+          result.append("\\t");
+          break;
+        case '\n':
+          result.append("\\n");
+          break;
+        case '\f':
+          result.append("\\f");
+          break;
+        case '\r':
+          result.append("\\r");
+          break;
+        case '"':
+          result.append("\\\"");
+          break;
+        case '\\':
+          result.append("\\\\");
+          break;
+        default:
+          if (c < 0x20 || c == 0x7f) {
+            result.append(String.format("\\u%04x", (int) c));
+          } else {
+            result.append(c);
+          }
+      }
+    }
+    if (value.length() > maxLength) {
+      result.append("...");
+    }
+    result.append('"');
+    return result.toString();
+  }
 }

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

@@ -28,7 +28,7 @@ void load() {
                         Map.of("io.prometheus.exporter.include_created_timestamps", "invalid"))))
         .withMessage(
             "io.prometheus.exporter.include_created_timestamps: Expecting 'true' or 'false'. Found:"
-                + " invalid");
+                + " \"invalid\"");
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(
             () ->
@@ -37,7 +37,7 @@ void load() {
                         Map.of("io.prometheus.exporter.exemplars_on_all_metric_types", "invalid"))))
         .withMessage(
             "io.prometheus.exporter.exemplars_on_all_metric_types: Expecting 'true' or 'false'."
-                + " Found: invalid");
+                + " Found: \"invalid\"");
   }
 
   private static ExporterProperties load(Map<String, String> map) {
@@ -85,6 +85,6 @@ void prometheusTimestampsInMs() {
                         Map.of("io.prometheus.exporter.prometheus_timestamps_in_ms", "invalid"))))
         .withMessage(
             "io.prometheus.exporter.prometheus_timestamps_in_ms: Expecting 'true' or 'false'."
-                + " Found: invalid");
+                + " Found: \"invalid\"");
   }
 }

prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/ExporterPropertiesTest.java

@@ -27,7 +27,7 @@ void load() {
         .isThrownBy(() -> load(Map.of("io.prometheus.exporter.pushgateway.scheme", "foo")))
         .withMessage(
             "io.prometheus.exporter.pushgateway.scheme: Illegal value. Expecting 'http' or 'https'."
-                + " Found: foo");
+                + " Found: \"foo\"");
   }
 
   @Test
@@ -60,7 +60,7 @@ void loadWithInvalidEscapingScheme() {
             () -> load(Map.of("io.prometheus.exporter.pushgateway.escaping_scheme", "invalid")))
         .withMessage(
             "io.prometheus.exporter.pushgateway.escaping_scheme: Illegal value. Expecting"
-                + " 'allow-utf-8', 'values', 'underscores', or 'dots'. Found: invalid");
+                + " 'allow-utf-8', 'values', 'underscores', or 'dots'. Found: \"invalid\"");
   }
 
   @Test

prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/ExporterPushgatewayPropertiesTest.java

@@ -38,7 +38,7 @@ void loadInvalidValue() {
         .isThrownBy(
             () -> load(new HashMap<>(Map.of("io.prometheus.openmetrics2.enabled", "invalid"))))
         .withMessage(
-            "io.prometheus.openmetrics2.enabled: Expecting 'true' or 'false'. Found: invalid");
+            "io.prometheus.openmetrics2.enabled: Expecting 'true' or 'false'. Found: \"invalid\"");
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(
             () ->
@@ -47,7 +47,7 @@ void loadInvalidValue() {
                         Map.of("io.prometheus.openmetrics2.content_negotiation", "invalid"))))
         .withMessage(
             "io.prometheus.openmetrics2.content_negotiation: Expecting 'true' or 'false'. Found:"
-                + " invalid");
+                + " \"invalid\"");
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(
             () ->
@@ -56,7 +56,7 @@ void loadInvalidValue() {
                         Map.of("io.prometheus.openmetrics2.composite_values", "invalid"))))
         .withMessage(
             "io.prometheus.openmetrics2.composite_values: Expecting 'true' or 'false'. Found:"
-                + " invalid");
+                + " \"invalid\"");
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(
             () ->
@@ -65,7 +65,7 @@ void loadInvalidValue() {
                         Map.of("io.prometheus.openmetrics2.exemplar_compliance", "invalid"))))
         .withMessage(
             "io.prometheus.openmetrics2.exemplar_compliance: Expecting 'true' or 'false'. Found:"
-                + " invalid");
+                + " \"invalid\"");
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(
             () ->
@@ -74,7 +74,7 @@ void loadInvalidValue() {
                         Map.of("io.prometheus.openmetrics2.native_histograms", "invalid"))))
         .withMessage(
             "io.prometheus.openmetrics2.native_histograms: Expecting 'true' or 'false'. Found:"
-                + " invalid");
+                + " \"invalid\"");
   }
 
   private static OpenMetrics2Properties load(Map<String, String> map) {

prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/OpenMetrics2PropertiesTest.java

@@ -51,6 +51,16 @@ void loadOptionalDuration_invalidNumber_throws() {
 
     assertThatExceptionOfType(PrometheusPropertiesException.class)
         .isThrownBy(() -> Util.loadOptionalDuration("", "foo", propertySource))
-        .withMessage("foo=abc: Expecting long value");
+        .withMessage("foo: Expecting long value. Found: \"abc\"");
+  }
+
+  @Test
+  void invalidValueMessageEscapesRawValue() {
+    Map<Object, Object> regularProperties = new HashMap<>(Map.of("foo", "bad\n\"value"));
+    PropertySource propertySource = new PropertySource(regularProperties);
+
+    assertThatExceptionOfType(PrometheusPropertiesException.class)
+        .isThrownBy(() -> Util.loadBoolean("", "foo", propertySource))
+        .withMessage("foo: Expecting 'true' or 'false'. Found: \"bad\\n\\\"value\"");
   }
 }

prometheus-metrics-config/src/test/java/io/prometheus/metrics/config/UtilTest.java

@@ -2,6 +2,7 @@
 
 import io.prometheus.metrics.model.snapshots.DataPointSnapshot;
 import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.ReentrantLock;
@@ -18,6 +19,7 @@
 class Buffer {
 
   private static final long bufferActiveBit = 1L << 63;
+  private static final long DEFAULT_MAX_SPIN_WAIT_NANOS = TimeUnit.SECONDS.toNanos(1);
   // Tracking observation counts requires an AtomicLong for coordination between recording and
   // collecting. AtomicLong does much worse under contention than the LongAdder instances used
   // elsewhere to hold aggregated state. To improve, we stripe the AtomicLong into N instances,
@@ -34,16 +36,22 @@ class Buffer {
   ReentrantLock appendLock = new ReentrantLock();
   ReentrantLock runLock = new ReentrantLock();
   Condition bufferFilled = appendLock.newCondition();
+  private final long maxSpinWaitNanos;
 
   Buffer() {
+    this(DEFAULT_MAX_SPIN_WAIT_NANOS);
+  }
+
+  Buffer(long maxSpinWaitNanos) {
+    this.maxSpinWaitNanos = maxSpinWaitNanos;
     stripedObservationCounts = new AtomicLong[Runtime.getRuntime().availableProcessors()];
     for (int i = 0; i < stripedObservationCounts.length; i++) {
       stripedObservationCounts[i] = new AtomicLong(0);
     }
   }
 
   boolean append(double value) {
-    int index = Math.abs((int) Thread.currentThread().getId()) % stripedObservationCounts.length;
+    int index = stripeIndex(Thread.currentThread().getId(), stripedObservationCounts.length);
     AtomicLong observationCountForThread = stripedObservationCounts[index];
     long count = observationCountForThread.incrementAndGet();
     if ((count & bufferActiveBit) == 0) {
@@ -54,6 +62,10 @@ boolean append(double value) {
     }
   }
 
+  static int stripeIndex(long threadId, int stripeCount) {
+    return (int) Math.floorMod(threadId, stripeCount);
+  }
+
   private void doAppend(double amount) {
     appendLock.lock();
     try {
@@ -74,14 +86,15 @@ void reset() {
     reset = true;
   }
 
-  @SuppressWarnings("ThreadPriorityCheck")
+  @SuppressWarnings({"NullAway", "ThreadPriorityCheck"})
   <T extends DataPointSnapshot> T run(
       Function<Long, Boolean> complete,
       Supplier<T> createResult,
       Consumer<Double> observeFunction) {
     double[] buffer;
     int bufferSize;
     T result;
+    boolean timedOut = false;
 
     runLock.lock();
     try {
@@ -91,14 +104,19 @@ <T extends DataPointSnapshot> T run(
         expectedCount += observationCount.getAndAdd(bufferActiveBit);
       }
 
+      long deadline = System.nanoTime() + maxSpinWaitNanos;
       while (!complete.apply(expectedCount)) {
         // Wait until all in-flight threads have added their observations to the histogram /
         // summary.
         // we can't use a condition here, because the other thread doesn't have a lock as it's on
         // the fast path.
+        if (System.nanoTime() - deadline >= 0) {
+          timedOut = true;
+          break;
+        }
         Thread.yield();
       }
-      result = createResult.get();
+      result = timedOut ? null : createResult.get();
 
       // Signal that the buffer is inactive.
       long expectedBufferSize = 0;
@@ -137,6 +155,9 @@ <T extends DataPointSnapshot> T run(
     for (int i = 0; i < bufferSize; i++) {
       observeFunction.accept(buffer[i]);
     }
+    if (timedOut) {
+      throw new IllegalStateException("Timed out while waiting for in-flight observations.");
+    }
     return result;
   }
 }