-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add allocation write load stats to write thread pool #130373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
4037ccf
b8d8e56
fd8f602
47debcd
37b20e8
be50439
45f6836
bd2e0f1
629ca35
b0fdaf2
46a659b
3ecac48
4986fa5
7ded14f
f942332
f9488fe
5057324
fb05d84
326f9e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import java.util.concurrent.RejectedExecutionHandler; | ||
| import java.util.concurrent.ThreadFactory; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.concurrent.atomic.LongAdder; | ||
| import java.util.function.Function; | ||
|
|
||
|
|
@@ -50,6 +51,11 @@ public final class TaskExecutionTimeTrackingEsThreadPoolExecutor extends EsThrea | |
| private volatile long lastPollTime = System.nanoTime(); | ||
| private volatile long lastTotalExecutionTime = 0; | ||
| private final ExponentialBucketHistogram queueLatencyMillisHistogram = new ExponentialBucketHistogram(QUEUE_LATENCY_HISTOGRAM_BUCKETS); | ||
| private final boolean trackQueueLatencyEWMA; | ||
| private final boolean trackUtilizationEWMA; | ||
| private final ExponentiallyWeightedMovingAverage queueLatencyMillisEWMA; | ||
| private final ExponentiallyWeightedMovingAverage percentPoolUtilizationEWMA; | ||
| private final AtomicReference<Double> lastUtilizationValue = new AtomicReference<>(0.0); | ||
|
|
||
| TaskExecutionTimeTrackingEsThreadPoolExecutor( | ||
| String name, | ||
|
|
@@ -65,9 +71,14 @@ public final class TaskExecutionTimeTrackingEsThreadPoolExecutor extends EsThrea | |
| TaskTrackingConfig trackingConfig | ||
| ) { | ||
| super(name, corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory, handler, contextHolder); | ||
|
|
||
| this.runnableWrapper = runnableWrapper; | ||
| this.executionEWMA = new ExponentiallyWeightedMovingAverage(trackingConfig.getEwmaAlpha(), 0); | ||
| this.executionEWMA = new ExponentiallyWeightedMovingAverage(trackingConfig.getExecutionTimeEwmaAlpha(), 0); | ||
| this.trackOngoingTasks = trackingConfig.trackOngoingTasks(); | ||
| this.trackQueueLatencyEWMA = trackingConfig.trackQueueLatencyEWMA(); | ||
| this.queueLatencyMillisEWMA = new ExponentiallyWeightedMovingAverage(trackingConfig.getQueueLatencyEwmaAlpha(), 0); | ||
| this.trackUtilizationEWMA = trackingConfig.trackPoolUtilizationEWMA(); | ||
| this.percentPoolUtilizationEWMA = new ExponentiallyWeightedMovingAverage(trackingConfig.getPoolUtilizationEwmaAlpha(), 0); | ||
| } | ||
|
|
||
| public List<Instrument> setupMetrics(MeterRegistry meterRegistry, String threadPoolName) { | ||
|
|
@@ -136,6 +147,20 @@ public int getCurrentQueueSize() { | |
| return getQueue().size(); | ||
| } | ||
|
|
||
| public double getPercentPoolUtilizationEWMA() { | ||
| if (trackUtilizationEWMA == false) { | ||
| return 0; | ||
| } | ||
| return this.percentPoolUtilizationEWMA.getAverage(); | ||
| } | ||
|
|
||
| public double getQueuedTaskLatencyMillisEWMA() { | ||
| if (trackQueueLatencyEWMA == false) { | ||
| return 0; | ||
| } | ||
| return queueLatencyMillisEWMA.getAverage(); | ||
| } | ||
DiannaHohensee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Returns the fraction of the maximum possible thread time that was actually used since the last time | ||
| * this method was called. | ||
|
|
@@ -153,20 +178,41 @@ public double pollUtilization() { | |
|
|
||
| lastTotalExecutionTime = currentTotalExecutionTimeNanos; | ||
| lastPollTime = currentPollTimeNanos; | ||
|
|
||
| if (trackUtilizationEWMA) { | ||
| percentPoolUtilizationEWMA.addValue(utilizationSinceLastPoll); | ||
| // Test only tracking. | ||
| assert setUtilizationSinceLastPoll(utilizationSinceLastPoll); | ||
| } | ||
|
||
|
|
||
| return utilizationSinceLastPoll; | ||
| } | ||
|
|
||
| // Test only | ||
| private boolean setUtilizationSinceLastPoll(double utilizationSinceLastPoll) { | ||
| lastUtilizationValue.set(utilizationSinceLastPoll); | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| protected void beforeExecute(Thread t, Runnable r) { | ||
| if (trackOngoingTasks) { | ||
| ongoingTasks.put(r, System.nanoTime()); | ||
| } | ||
|
|
||
| assert super.unwrap(r) instanceof TimedRunnable : "expected only TimedRunnables in queue"; | ||
| final TimedRunnable timedRunnable = (TimedRunnable) super.unwrap(r); | ||
| timedRunnable.beforeExecute(); | ||
| final long taskQueueLatency = timedRunnable.getQueueTimeNanos(); | ||
| assert taskQueueLatency >= 0; | ||
| queueLatencyMillisHistogram.addObservation(TimeUnit.NANOSECONDS.toMillis(taskQueueLatency)); | ||
| var queueLatencyMillis = TimeUnit.NANOSECONDS.toMillis(taskQueueLatency); | ||
| queueLatencyMillisHistogram.addObservation(queueLatencyMillis); | ||
|
|
||
| if (trackQueueLatencyEWMA) { | ||
| if (queueLatencyMillis > 0) { | ||
| queueLatencyMillisEWMA.addValue(queueLatencyMillis); | ||
| } | ||
DiannaHohensee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -208,6 +254,12 @@ protected void appendThreadPoolExecutorDetails(StringBuilder sb) { | |
| .append("total task execution time = ") | ||
| .append(TimeValue.timeValueNanos(getTotalTaskExecutionTime())) | ||
| .append(", "); | ||
| if (trackQueueLatencyEWMA) { | ||
| sb.append("task queue EWMA = ").append(TimeValue.timeValueMillis((long) getQueuedTaskLatencyMillisEWMA())).append(", "); | ||
| } | ||
| if (trackUtilizationEWMA) { | ||
| sb.append("thread pool utilization percentage EWMA = ").append(getPercentPoolUtilizationEWMA()).append(", "); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -222,7 +274,27 @@ public Map<Runnable, Long> getOngoingTasks() { | |
| } | ||
|
|
||
| // Used for testing | ||
| public double getEwmaAlpha() { | ||
| public double getExecutionEwmaAlpha() { | ||
| return executionEWMA.getAlpha(); | ||
| } | ||
|
|
||
| // Used for testing | ||
| public double getQueueLatencyEwmaAlpha() { | ||
| return queueLatencyMillisEWMA.getAlpha(); | ||
| } | ||
|
|
||
| // Used for testing | ||
| public double getPoolUtilizationEwmaAlpha() { | ||
| return percentPoolUtilizationEWMA.getAlpha(); | ||
| } | ||
|
|
||
| // Used for testing | ||
| public boolean trackingQueueLatencyEwma() { | ||
| return trackQueueLatencyEWMA; | ||
| } | ||
nicktindall marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Used for testing | ||
| public boolean trackUtilizationEwma() { | ||
| return trackUtilizationEWMA; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this 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.
Nit: I think when you get to three consecutive booleans, it's worth making a builder for readability, e.g.
A side benefit would be you could default all to false and just specify the ones that were true.
Happy to be challenged on that.
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.
If we decide this is a good idea, also happy for it to be a separate PR because it'll add a fair bit of noise I imagine
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.
Sure, I'll put up a separate PR for this
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.
Alternatively I wonder if the max latency tracking is so cheap that we don't need to have a config for it. The ongoing task tracking is quite expensive, so it makes sense to be able to opt out of it. It would also reduce the surface area of this change.
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.
It seems a waste to track information unnecessarily, and potentially confusing wondering why it's not used. I'd prefer not to. Also never know how code will grow over time.
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.
To close this out, I've merged a TaskTrackingConfig Builder subclass and updated the callers with it.