fix: harden metric scrape edge cases #2297
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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."); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Member
There was a problem hiding this comment. The 1 s deadline is only reachable via this package-private constructor (tests). Should we expose a knob for this?
Member
Author
There was a problem hiding this comment. The deadline constructor remains package-private and test-only; there is no public configuration knob. The production constructor continues to use the default one-second deadline. |
||
| 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 = 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) { | ||
|
Member
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment. Addressed in ec7f83d. The observation generation now has an explicit DEFAULT_MAX_BUFFER_SIZE of 1,000,000, independent of the timeout. Appends wait for space while the generation is active and stop when it is deactivated; I chose bounded blocking rather than drop-on-overflow so recording does not silently lose observations. |
||
| appendLock.lock(); | ||
| try { | ||
|
|
@@ -74,14 +86,15 @@ void reset() { | |
| reset = true; | ||
| } | ||
|
|
||
| @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 = timedOut ? null : createResult.get(); | ||
|
Member
There was a problem hiding this comment. The spin-wait timeout covers this first loop, but the second wait loop below (
Member
Author
There was a problem hiding this comment. Fixed in ec7f83d. The buffer handoff is now coordinated through generation/phase transitions, so the second unbounded bufferFilled.await() is gone. Appenders that straddle activation/deactivation are accounted for, and there is regression coverage for a stalled appender. |
||
|
|
||
| // 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."); | ||
|
Member
There was a problem hiding this comment. Throwing here is the intended behavior on the scrape path ( On the internal reset/scale-down path, should we log and defer (skip this cycle's scale-down) instead of throwing, and then only surface the timeout on the scrape/
Member
Author
There was a problem hiding this comment. Fixed in ec7f83d. |
||
| } | ||
| return result; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escaping control chars (log-injection / CWE-117 defense) and the 100-char cap are good, but the raw value stays in the message. The client-facing leak via
HttpExchangeAdapteris closed by the #2283 fix; however the servlet adapters (servlet-jakarta/javax) re-throw to the container, so up to 100 chars of the offending config value can still land in a container error page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ec7f83d. Invalid configuration messages now include only the property key and expected format; the offending raw value is no longer included. This is also applied to the PushGateway credential-related paths, so servlet/container error propagation cannot expose the configured value.