Add inbound connections metrics #49103
Conversation
andrershov
added
>feature
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed (:Distributed/Network) |
andrershov
changed the title
| void inboundMessage(TcpChannel channel, BytesReference message) throws Exception { | ||
| channel.getChannelStats().markAccessed(threadPool.relativeTimeInMillis()); | ||
| TransportLogger.logInboundMessage(channel, message); | ||
| readBytesMetric.inc(message.length() + TcpHeader.MARKER_BYTES_SIZE + TcpHeader.MESSAGE_LENGTH_SIZE); | ||
| readBytesMetric.inc(getMessageSizeWithHeader(message)); |
There was a problem hiding this comment.
Just store this in a variable so you can reuse it in the other two places below :)
| static final String RESPONSES_SENT_SIZE_IN_BYTES = "resp_sent_size_in_bytes"; | ||
| static final String KEEP_ALIVE_PINGS_RECEIVED_COUNT = "keep_alive_pings_rcv_count"; | ||
| static final String KEEP_ALIVE_PINGS_RECEIVED_SIZE = "keep_alive_pings_rcv_size"; | ||
| static final String KEEP_ALIVE_PINGS_RECEIVED_SIZE_IN_BYTES = "keep_alive_pings_rcv_size_in_bytes"; |
There was a problem hiding this comment.
Exposing (and generally separately tracking) the size of pings and pongs seems somewhat redundant since we can compute them from counts?
In particular, this would resolve the issue of getting inconsistent data in these stats (since the number of bytes and the count can change between calls to count and sum).
| MeanMetric outboundResponses = outboundHandler.getOutboundResponses(); | ||
| MeanMetric inboundKeepAlivePings = inboundHandler.getInboundKeepAlivePings(); | ||
| MeanMetric outboundKeepAlivePongs = outboundHandler.getOutboundKeepAlivePongs(); | ||
| TransportStats.InboundConnectionsStats inboundConnectionsStats = new TransportStats.InboundConnectionsStats( |
There was a problem hiding this comment.
Maybe create a Builder for TransportStats.InboundConnectionsStats. This constructor already pretty much impossible to read without an IDE showing parameter names and it's just going to get worse as we add more to this object?
| } else { | ||
| bytesMetric = outboundPingsMetric; | ||
| } | ||
| SendContext sendContext = new SendContext(channel, bytesMetric, () -> serializedPing, listener); |
There was a problem hiding this comment.
This can just be deduplicated by adding a metric parameter to sendBytes and reusing it here. Though (see my other comment on this) we probably don't need to even track the bytes in pings and pongs separate from other bytes.
| inboundOpenedChannels.inc(); | ||
| channel.addCloseListener(ActionListener.wrap(() -> { | ||
| acceptedChannels.remove(channel); | ||
| inboundClosedChannels.inc(); |
There was a problem hiding this comment.
One of the two metrics here is redundant since openedChannels - closedChannels = acceptedChannels.size(). I think it's more useful to expose the number of currently open channels and either the open or close count instead of keeping track of a redundant counter here, both implementation wise and in terms of usefulness for the consumer of these numbers. That's also what is suggested in the meta #36127 I think
| private final long keepAlivePongsSentCount; | ||
| private final long keepAlivePongsSentBytes; | ||
|
|
||
| static final String INBOUND_CONNECTIONS = "inbound_connections"; |
There was a problem hiding this comment.
As suggested in the other comment and #36127, probably better to make this a gauge and only keep the closed counter.
rjernst
added
the
Team:Distributed (Obsolete)
This commit adds the following metrics for inbound connections:
There is no separate reporting for each interface so far (anyway currently only single publish address is supported).