-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Move to a single shared utilization tracker #131796
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
base: main
Are you sure you want to change the base?
Conversation
| public final class TaskExecutionTimeTrackingEsThreadPoolExecutor extends EsThreadPoolExecutor { | ||
| public static final int QUEUE_LATENCY_HISTOGRAM_BUCKETS = 18; | ||
| private static final int[] LATENCY_PERCENTILES_TO_REPORT = { 50, 90, 99 }; | ||
| private static final long UTILISATION_REFRESH_INTERVAL_NANOS = TimeValue.timeValueSeconds(45).nanos(); |
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.
This should be a setting perhaps so we can experiment?
| * Get the most recent utilization value calculated | ||
| */ | ||
| public double getUtilization() { | ||
| return lastUtilization; |
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.
perhaps we should also call recalculateUtilizationIfDue in here in case there is zero activity in the pool? probably not an issue for the write pool.
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.
I think we want that to avoid it being infinitely stale, but then you get into a task tracking issue?
Also, it seems like the value can now be 30s old instead of current?
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.
I added a recalculate here, in case we end up using this. You're right, because we are recalculating independently of the polling it's possible the utilisation is up to TaskTrackingConfig#utilizationRefreshInterval old. Given that it's an average over the same interval, and our current goal of acting only on persistent hot-spots I think that's probably OK, but hopefully we can get a better utilisation measure.
| private boolean trackOngoingTasks = false; | ||
| private boolean trackMaxQueueLatency = false; | ||
| private double ewmaAlpha = DEFAULT_EXECUTION_TIME_EWMA_ALPHA_FOR_TEST; | ||
| private TimeValue utilizationRefreshInterval = TimeValue.timeValueSeconds(30); |
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.
I suspect that 30s might be too short given we saw > 1.0 utilization in the APM metrics which were calculated every 60s. Perhaps we should use 60 or split the difference and do 45?
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
I though of a different approach, when we talked outside. I'll try to summarize. Happy to send PR if you like it. We can trade liveliness to accuracy when measuring thread-pool utilization. That means reporting Couple of terms I use below: Approach is simple - pollUtilization returns execution time of previous frame, since we know exactly We need to consider following cases:
pseudocode: Class variables:
currentFrame
currentFrameExecutionTime // for finished tasks
previousFrame
previousFrameExecutionTime // for finished tasks
currentTasks
afterExecute(task):
endFrame = task.endTime / interval
maybeResetFrame(endFrame)
startFrame = task.startTime / interval
if (startFrame == currentFrame):
currentFrameExecutionTime += task.endTime - task.startTime # task started and finished in same frame
else:
currentFrameExecutionTime += task.endTime - currentFrame * interval
if(startFrame == previousFrame):
previousFrameExecutionTime += currentFrame * interval - task.startTime # task started in previous frame
else:
previousFrameExecutionTime += interval # task started before previous frame
# first time seeing frame or havent updated frame for a long time
maybeResetFrame(nowFrame):
if (nowFrame == currentFrame):
pass
else if (nowFrame - currentFrame == 1):
previousFrameExecutionTime = currentFrameExecutionTime
currentFrameExecutionTime = 0
previousFrame = currentFrame
currentFrame = nowFrame
else:
previousFrameExecutionTime = 0
currentFrameExecutionTime = 0
currentFrame = nowFrame
previousFrame = nowFrame -1
# returns previous frame tasks, completed and still running
pollUtilization():
nowFrame = timeNow / interval
maybeResetFrame(nowFrame)
totalTime = previousFrameExecutionTime
for (task in currentTasks):
startFrame = task.startTime / interval
if (startFrame == previousFrame):
totalTime += currentFrame * interval - task.startTime # task started in previous frame, still running
else if (startFrame < previousFrame):
totalTime += interval # task started before previous frame, still running
return totalTime / threadPoolSize * interval # can be cached by frame key
|
I like it but I think it's better to avoid the dependency on current tasks. Not all thread pools track running tasks and to me it seems quite expensive to do (i.e you wouldn't want to turn it on across the board). I'd be inclined to keep it simple and revisit if the accuracy is a problem? |
|
as discussed in slack I will work on my proposal addressing drawbacks of tracking ongoing tasks |
Initial work after discussion around thread-pool utilization tracking.