Add port tag to server metrics #6116
Conversation
Motivation: A server can be configured with multiple ports, and operators may want to collect connection and request metrics per port for better observability and debugging. Modifications: - Added a `port` tag to server metrics. - If a `DomainSocketAddress` is used, the path of the address is used as the tag value. Result: - Enabled per-port metrics collection for better monitoring and troubleshooting.
| private final Map<Integer, LongAdder> pendingHttp1Requests; | ||
| private final Map<Integer, LongAdder> pendingHttp2Requests; | ||
| private final Map<Integer, LongAdder> activeHttp1WebSocketRequests; | ||
| private final Map<Integer, LongAdder> activeHttp1Requests; | ||
| private final Map<Integer, LongAdder> activeHttp2Requests; | ||
| private final Map<String, LongAdder> domainSocketPendingHttp1Requests; | ||
| private final Map<String, LongAdder> domainSocketPendingHttp2Requests; | ||
| private final Map<String, LongAdder> domainSocketActiveHttp1WebSocketRequests; | ||
| private final Map<String, LongAdder> domainSocketActiveHttp1Requests; | ||
| private final Map<String, LongAdder> domainSocketActiveHttp2Requests; |
There was a problem hiding this comment.
Optional) The implementation that holds many Maps in this class looks complex. How about introducing ServerMetricsGroup to encapsulate the complexity?
My suggestion:
public interface ServerMetrics extends MeterBinder {
}
class ServerPortMetrics implements ServerMetrics {
...
}
class ServerMetricsGroup implements ServerMetrics {
private Map<Object, ServerPortMetrics> delegates;
...
}There was a problem hiding this comment.
I don't fully understand your intention.
So the ServerMetricsGroup has all the delegates?
There was a problem hiding this comment.
Right. I imagined the following implementation:
class ServerMetricsGroup implements ServerMetrics {
private Map<Object, ServerPortMetrics> delegates;
void increasePendingHttp1Requests(SocketAddress socketAddress) {
Object key = getPathOrPort(socketAddress);
delegates.get(key).increasePendingHttp1Requests();
}
public long pendingHttp1Requests() {
// Sum all pendingHttp1Requests in delegates and return
}
}There was a problem hiding this comment.
That looks much simpler. 👍
Let me use that approach.
There was a problem hiding this comment.
On my second thought, I've realized that I didn't want to use the Object as a key because
InetSocketAddressis used mostly for applications- So the integer port needs to box and unbox.
Let me develop your idea and avoid those unboxing.
There was a problem hiding this comment.
Split to InetSocketServerMetrics and DomainSocketServerMetrics. PTAL. 🙇♂️
|
@yzfeng2020 Please let me know if the current implementation if fine. 😉 |
| } finally { | ||
| lock.unlock(); | ||
| } | ||
|
|
||
| config().serverMetrics().addActivePort(actualPort); |
There was a problem hiding this comment.
Note) It slightly bothers me that the active port is registered after binding to the port (since technically a connection could be attempted immediately and may not be able to find a LongAdder).
In practice, I assume this won't happen much though.
Alternatively, another way that could've been possible seems to be using channel attributes.
PerPortMetrics perPortMetrics;
...
// use the metrics from the attribute to record metrics
CompletionStage<Void> doStart(@Nullable Void arg) {
Bootstrap b;
b.childAttr(AttributeKey.valueOf("SERVER_METRICS"), serverPortMetrics);
}
...
void operationComplete(ChannelFuture f) {
if (f.isSuccess()) {
// bind each port separately
serverPortMetrics.bind(meterRegistry, Tags.of(""));
...
}I'm fine with the current implementation though
There was a problem hiding this comment.
That's a brilliant idea. 👍
Changed to use the attribute and reverted the ServerMetrics to a concrete class since it's unlikely that a user would need to implement her own ServerMetrics.
Motivation:
A server can be configured with multiple ports, and operators may want to collect connection and request metrics per port for better observability and debugging.
Modifications:
porttag to server metrics.DomainSocketAddressis used, the path of the address is used as the tag value.Result: