fix: harden metric scrape edge cases #2297
There are no files selected for viewing
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
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
| 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."); | ||
| } | ||
| } |
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
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
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
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
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
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
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
| 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) { | ||
|
Collaborator
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? |
||
| 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) { | ||
|
Collaborator
There was a problem hiding this comment.
|
||
| 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(); | ||
|
Collaborator
There was a problem hiding this comment. The spin-wait timeout covers this first loop, but the second wait loop below ( |
||
|
|
||
| // 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."); | ||
|
Collaborator
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/ |
||
| } | ||
| return result; | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
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.