Expose network memory pool stats #42912
Conversation
Tim-Brooks
added
>non-issue
WIP
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed |
|
I'm opening this up as a POC for some feedback. A few notes:
I think that what I would like to do is expose this on maybe an experimental API? And not back port the actual REST API to 7.x. As we continue to hash out improved network metrics, we can decide if this will be rolled into the current transport stats API. And once the API is determined we can back port to 7.x. But I am open to discussion on how to do this. |
There was a problem hiding this comment.
This looks good to me in terms of the approach (there's not much of any overhead to it and APIs look just fine to me). This should save inspecting heap dumps in a few Netty related scenarios already imo.
=> I'd be all for adding a REST API to this and some tests and make progress on it :)
I left two comments (the Netty one might be irrelevant at this stage and can be ignored if so)
| protected MemoryUsage memoryStats() { | ||
| MemoryUsage baseMemoryUsage = super.memoryStats(); | ||
|
|
||
| ByteBufAllocator allocator = ByteBufAllocator.DEFAULT; |
There was a problem hiding this comment.
This is more of a detail, but don't we also have to grab the stats off of io.netty.buffer.UnpooledByteBufAllocator#DEFAULT if we have the pooled allocator running by default? Otherwise we miss all the heap usage our REST layer creates in the transport stats since it uses Unpooled.allocate right?
There was a problem hiding this comment.
We never release the bytes used at the REST level so those metrics would increase forever. In fact those metrics would throw off the network level metrics in this PR currently, so I will probably need to separate the allocators (if we are using unpooled). I will look into that.
There was a problem hiding this comment.
Anyway I guess what I'm saying here is that I view application level bytes (in the rest request and transport deserialized messages) as beyond the scope of this PR currently as it would involve adding a lifecycle to these messages.
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class TransportStatsV2 implements Writeable, ToXContentFragment { |
There was a problem hiding this comment.
This class is probable just here by accident? It doesn't seem used or functional yet.
| @@ -45,6 +45,12 @@ public DequeRecycler(C<T> c, Deque<T> queue, int maxSize) { | |||
| return new DV(v, true); | |||
| } | |||
|
|
|||
| @Override | |||
| public int recycledCount() { | |||
| // TODO: Does not reflect that the currently allocated number of pages might be less. | |||
There was a problem hiding this comment.
why not add tracking for this in DequeRecycler? Is this difficult to do?
There was a problem hiding this comment.
It is just work and I wanted to avoid making many changes before getting feedback (also I think it needs to be synchronized for the non-concurrent Deques).
| protected MemoryUsage memoryStats() { | ||
| MemoryUsage baseMemoryUsage = super.memoryStats(); | ||
|
|
||
| ByteBufAllocator allocator = ByteBufAllocator.DEFAULT; |
|
I'll make some more updates to this PR later this week based on feedback. |
|
Closing this as it was a POC and is well out of date. We can always bring these concepts back into an updated PR if necessary. |
This is related to #36127. This PR exposes stats about the memory pools
that are used by the networking layer.