-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add utilization to the THREAD_POOL node stats #132156
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 2 commits
be2d2ea
45f2c4d
08c91e8
9ad2294
2636bbf
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 |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| package org.elasticsearch.threadpool; | ||
|
|
||
| import org.elasticsearch.TransportVersions; | ||
| import org.elasticsearch.common.collect.Iterators; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
@@ -25,6 +26,7 @@ | |
| import java.util.Iterator; | ||
| import java.util.List; | ||
|
|
||
| import static java.lang.Float.NaN; | ||
| import static java.util.Collections.emptyIterator; | ||
| import static org.elasticsearch.common.collect.Iterators.single; | ||
|
|
||
|
|
@@ -41,14 +43,27 @@ public static ThreadPoolStats merge(ThreadPoolStats first, ThreadPoolStats secon | |
| return new ThreadPoolStats(mergedThreadPools.values()); | ||
| } | ||
|
|
||
| public record Stats(String name, int threads, int queue, int active, long rejected, int largest, long completed) | ||
| public record Stats(String name, int threads, int queue, int active, long rejected, int largest, long completed, float utilization) | ||
| implements | ||
| Writeable, | ||
| ChunkedToXContent, | ||
| Comparable<Stats> { | ||
|
|
||
| public Stats(StreamInput in) throws IOException { | ||
| this(in.readString(), in.readInt(), in.readInt(), in.readInt(), in.readLong(), in.readInt(), in.readLong()); | ||
| public static Stats readFrom(StreamInput in) throws IOException { | ||
| final String name = in.readString(); | ||
| final int threads = in.readInt(); | ||
| final int queue = in.readInt(); | ||
| final int active = in.readInt(); | ||
| final long rejected = in.readLong(); | ||
| final int largest = in.readInt(); | ||
| final long completed = in.readLong(); | ||
| final float utilization; | ||
| if (in.getTransportVersion().onOrAfter(TransportVersions.UTILIZATION_IN_THREAD_POOL_NODE_STATS)) { | ||
| utilization = in.readFloat(); | ||
| } else { | ||
| utilization = NaN; | ||
| } | ||
| return new Stats(name, threads, queue, active, rejected, largest, completed, utilization); | ||
| } | ||
|
|
||
| static Stats merge(Stats firstStats, Stats secondStats) { | ||
|
|
@@ -59,7 +74,8 @@ static Stats merge(Stats firstStats, Stats secondStats) { | |
| sumStat(firstStats.active, secondStats.active), | ||
| sumStat(firstStats.rejected, secondStats.rejected), | ||
| sumStat(firstStats.largest, secondStats.largest), | ||
| sumStat(firstStats.completed, secondStats.completed) | ||
| sumStat(firstStats.completed, secondStats.completed), | ||
| NaN // Don't sum utilization, it makes no sense | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is used by the get cluster info REST api to merge the stats for each thread pool received from each node. I don't think it makes sense to merge utilization here, and in any case we don't render it in toXContent. Perhaps if/when we start rendering it we can implement a sensible merge? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the indication that we don't really want to do this here. Although this might seem simpler than #131480 in terms of lines-of-code added, IMO that's because this is a bit of a short-cut. Fields in the transport messages which aren't represented in the REST API are tech debt. I'd rather we moved towards having the allocator use dedicated messages for all these things rather than relying on stats APIs - the stats APIs we use today return a bunch of stuff about which we don't care. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a few like this now for features that aren't settled. Not because it was too hard, but rather to allow the freedom to change them before we commit to something that would be considered a breaking change. |
||
| ); | ||
| } | ||
|
|
||
|
|
@@ -96,6 +112,9 @@ public void writeTo(StreamOutput out) throws IOException { | |
| out.writeLong(rejected); | ||
| out.writeInt(largest); | ||
| out.writeLong(completed); | ||
| if (out.getTransportVersion().onOrAfter(TransportVersions.UTILIZATION_IN_THREAD_POOL_NODE_STATS)) { | ||
| out.writeFloat(utilization); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -125,6 +144,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP | |
| rejected != -1 ? single((builder, params) -> builder.field(Fields.REJECTED, rejected)) : emptyIterator(), | ||
| largest != -1 ? single((builder, params) -> builder.field(Fields.LARGEST, largest)) : emptyIterator(), | ||
| completed != -1 ? single((builder, params) -> builder.field(Fields.COMPLETED, completed)) : emptyIterator(), | ||
| // TODO: Leaving out thread pool utilization until we've settled on its final form | ||
| ChunkedToXContentHelper.endObject() | ||
| ); | ||
| } | ||
|
|
@@ -137,7 +157,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP | |
| } | ||
|
|
||
| public ThreadPoolStats(StreamInput in) throws IOException { | ||
| this(in.readCollectionAsList(Stats::new)); | ||
| this(in.readCollectionAsList(Stats::readFrom)); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
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.
Just a stop-gap until we have the singular utilisation value
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.
:'-)