Add ServerMetrics to collect metrics related server #5627

seongbeenkim · 2024-04-22T07:38:38Z

Motivation:

Add ServerMetrics to collect metrics related server. #4992

Modifications:

Add ServerMetrics.increasePendingHttp1Requests()
in Http1RequestDecoder#channelRead before fireChannelRead()
Add ServerMetrics.increasePendingHttp2Requests()
in Http2RequestDecoder#onHeadersRead before fireChannelRead()
Add ServerMetrics.decreasePendingHttp1Requests(), ServerMetrics.decreasePendingHttp2Requests(), ServerMetrics.increaseActiveHttp1WebSocketRequests(), ServerMetrics.increaseActiveHttp1Requests() and ServerMetrics.increaseActiveHttp2Requests()
in HttpServerHandler#serve0 before service.serve(reqCtx, req)
Add ServerMetrics.decreaseActiveHttp1WebSocketRequests(), ServerMetrics.decreaseActiveHttp1Requests() and ServerMetrics.decreaseActiveHttp2Requests()
in AbstractHttpResponseHandler#tryComplete
Add ServerMetrics.increaseActiveConnectionsAndGet() and ServerMetrics.decreaseActiveConnections()
in ConnectionLimitingHandler#channelRead

Result:

Closes Expose the server-side metrics programmatically #4992. (If this resolves the issue.)
users can implement features based on ServerMertrics like graceful shutdowns such as stopping the server when there are no pending requests.

CLAassistant · 2024-04-22T07:38:46Z

All committers have signed the CLA.

minwoox

Looks great, @seongbeenkim! Left some suggestions. 😉

minwoox · 2024-04-22T08:07:47Z

core/src/test/java/com/linecorp/armeria/server/ServerMetricsTest.java

+            sb.service(PATH, new HttpService() {
+                @Override
+                public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception {
+                    assertThat(server.server().config().serverMetrics().activeRequests()).isOne();


Let's also add test cases that fail. e.g. throwing an exception in this serve() method and request timeout.

i've added the test cases!!

minwoox · 2024-04-22T08:08:30Z

core/src/main/java/com/linecorp/armeria/server/Server.java

@@ -574,14 +575,19 @@ private ChannelFuture doStart(ServerPort port) {
        }

        private void setupServerMetrics() {
-            final MeterRegistry meterRegistry = config().meterRegistry();
+            final MeterRegistry meterRegistry = config.meterRegistry();
+            final ServerMetrics serverMetrics = config.serverMetrics();
            final GracefulShutdownSupport gracefulShutdownSupport = this.gracefulShutdownSupport;
            assert gracefulShutdownSupport != null;

            meterRegistry.gauge("armeria.server.pending.responses", gracefulShutdownSupport,
                                GracefulShutdownSupport::pendingResponses);


@seongbeenkim is going to add the metric to the ServerMetrics in the follow-up PR.

minwoox · 2024-04-22T08:26:22Z

core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java

+    /**
+     * Returns the number of active requests.
+     */
+    public long activeRequests() {


What are your thoughts on breaking down activeRequests into activeHttp1Requests, activeHttp1WebSocketRequests, and activeHttp2Requests, and then summing them to form activeRequests?

i think that is a great idea!
so i added activeHttp1Requests, activeHttp1WebSocketRequests, and activeHttp2Request for activeRequests!
also pendingHttp1Requests and pendingHttp2Requests for pendingRequests

github-actions · 2024-04-22T08:52:42Z

🔍 Build Scan® (commit: `3bfbdc7`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	❌ (failure)	https://ge.armeria.dev/s/djbijatvahg32
build-self-hosted-unsafe-jdk-8	❌ (failure)	https://ge.armeria.dev/s/rqihq67anqvwc
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/yy3mencazxxvm
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/ugqbydlninxgc
build-self-hosted-unsafe-jdk-17-min-java-11	❌ (failure)	https://ge.armeria.dev/s/d4ohba3wzsg4o
build-self-hosted-unsafe-jdk-17-leak	❌ (null)	https://ge.armeria.dev/s/ehia7ekznae2y
build-self-hosted-unsafe-jdk-11	❌ (failure)	https://ge.armeria.dev/s/e5sddfrlsagmu
build-macos-12-jdk-21	❌ (failure)	https://ge.armeria.dev/s/ynutpbsrd7vmm

…Requests

…eature/line#4992/metric

minwoox · 2024-04-27T02:34:49Z

core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

+                if (isHttp1WebSocket) {
+                    serverMetrics.increaseActiveHttp1WebSocketRequests();
+                    serverMetrics.decreasePendingHttp1Requests();
+                } else if (reqCtx.sessionProtocol().isExplicitHttp1()) {
+                    serverMetrics.increaseActiveHttp1Requests();
+                    serverMetrics.decreasePendingHttp1Requests();
+                } else if (reqCtx.sessionProtocol().isExplicitHttp2()) {
+                    serverMetrics.increaseActiveHttp2Requests();
+                    serverMetrics.decreasePendingHttp2Requests();
+                }


Sorry, I think the current approach looks a bit unappealing. I've changed my mind. 😓 How do you feel about incorporating this logic into the decoded request instead?

class AggregatingDecodedHttpRequest { AggregatingDecodedHttpRequest(...) { ... if (routingCtx.sessionProtocol().isMultiplex()) { metrics.increasePendingHttp2Requests(); } else { metrics.increasePendingHttp1Requests(); } } ... private void completeAggregationFuture() { if (aggregationFuture.complete(null)) { if (routingCtx.sessionProtocol().isMultiplex()) { metrics.decreasePendingHttp2Requests(); metrics.increaseActiveHttp2Requests(); } else { metrics.decreasePendingHttp1Requests(); metrics.decreaseActiveHttp1Requests(); } } } } class StreamingDecodedHttpRequest { StreamingDecodedHttpRequest(...) { ... if (http1WebSocket) { metrics.increaseActiveHttp1WebSocketRequests(); } else if (routingCtx.sessionProtocol().isMultiplex()) { metrics.increaseActiveHttp2Requests(); } else { metrics.increaseActiveHttp1Requests(); } } } class EmptyContentDecodedHttpRequest {...}

I'm so sorry to take it so long to apply the review!
i've been trying to write more test cases related to AggregatingDecodedHttpRequest, StreamingDecodedHttpRequest and StreamingDecodedHttpRequest.

but it's a little bit hard and taking some time..
i will do my best to apply the review asap!

jrhee17

Looks good overall! Left some minor comments

jrhee17 · 2024-05-07T08:01:23Z

core/src/test/java/com/linecorp/armeria/server/ConnectionLimitingHandlerTest.java

@@ -44,13 +50,18 @@ void testExceedMaxNumConnections() {

    @Test
    void testMaxNumConnectionsRange() {
-        final ConnectionLimitingHandler handler = new ConnectionLimitingHandler(Integer.MAX_VALUE);
+        final Server server = Server.builder()


Question) Couldn't we just create a ServerMetrics instance and pass it instead of creating a Server?
Ditto for the other test case above.

e.g.

final ServerMetrics serverMetrics = new ServerMetrics(); val handler = new ConnectionLimitingHandler(Integer.MAX_VALUE, serverMetrics); ...

Yes, we can create ServerMetrics!!
i think creating ServerMetrics is a better way for the test code!!
i will apply the review thank you!! :)

jrhee17 · 2024-05-07T08:17:47Z

core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java

+ */
+public final class ServerMetrics {
+
+    private final LongAdder pendingRequests = new LongAdder();


This doesn't seem to be used

Suggested change

private final LongAdder pendingRequests = new LongAdder();

oh right.. i forgot to delete it after some modifications..
i will remove the field!! thank you!!

jrhee17 · 2024-05-07T08:24:23Z

core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java

@@ -823,6 +831,8 @@ static String toString(
        buf.append(absoluteUriTransformer);
        buf.append(", unhandledExceptionsReportIntervalMillis: ");
        buf.append(unhandledExceptionsReportIntervalMillis);
+        buf.append(", serverMetrics: ");
+        buf.append(serverMetrics);


We should also implement toString() for serverMetrics if we want to do this.

oh i will implement toString() method!! thank you!!

jrhee17 · 2024-05-07T08:31:24Z

core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java

@@ -82,6 +85,14 @@ void disconnectWhenFinished() {
    }

    final boolean tryComplete(@Nullable Throwable cause) {
+        if (req.isHttp1WebSocket()) {


I'm concerned decreaseActive*Requests will never have a chance to be called.
e.g. If a request is received but a connection is immediately closed, will decreaseActive*Requests always be called?

I'm wondering if the future where the request/response is completed is a more fail-safe location. What do you think @minwoox ?

armeria/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

Line 768 in cf45ab6

}

That's a good suggestion. I prefer the location. 👍

minwoox · 2024-05-21T13:20:25Z

Let me resolve the conflict. 😉

Add ServerMetrics to collect metrics related server

1f70133

minwoox added the new feature label Apr 22, 2024

minwoox added this to the 1.29.0 milestone Apr 22, 2024

minwoox reviewed Apr 22, 2024

View reviewed changes

seongbeenkim added 7 commits April 26, 2024 15:58

Add test cases when request timeout and throwing an exception in serve()

20615d9

Add activeHttp1WebSocketRequests, activeHttp1Requests and activeHttp2…

6b18e7f

…Requests

Add pendingHttp1Requests and pendingHttp2Requests

0a77e61

Modify pendingRequests()

5caf915

Apply checkstyleMain

49767b0

Modify test case

bbef4e1

Merge branch 'main' of https://github.com/seongbeenkim/armeria into f…

cfc9d80

…eature/line#4992/metric

minwoox reviewed Apr 27, 2024

View reviewed changes

jrhee17 reviewed May 7, 2024

View reviewed changes

minwoox mentioned this pull request May 8, 2024

Add pendingResponses to ServerMetrics #5666

Open

Merge branch 'main' into pr-5627-seongbeenkim-feature/line#4992/metric

3bfbdc7

minwoox requested review from ikhoon and trustin as code owners May 21, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ServerMetrics to collect metrics related server #5627

Add ServerMetrics to collect metrics related server #5627

seongbeenkim commented Apr 22, 2024 •

edited

CLAassistant commented Apr 22, 2024 •

edited

minwoox left a comment

minwoox Apr 22, 2024

seongbeenkim Apr 26, 2024

minwoox Apr 22, 2024

minwoox Apr 22, 2024

seongbeenkim Apr 26, 2024

github-actions bot commented Apr 22, 2024 •

edited

minwoox Apr 27, 2024

seongbeenkim May 21, 2024

jrhee17 left a comment

jrhee17 May 7, 2024

seongbeenkim May 21, 2024

jrhee17 May 7, 2024

seongbeenkim May 21, 2024

jrhee17 May 7, 2024

seongbeenkim May 21, 2024

jrhee17 May 7, 2024

minwoox May 7, 2024

minwoox commented May 21, 2024

Add ServerMetrics to collect metrics related server #5627

Are you sure you want to change the base?

Add ServerMetrics to collect metrics related server #5627

Conversation

seongbeenkim commented Apr 22, 2024 • edited

CLAassistant commented Apr 22, 2024 • edited

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024 • edited

🔍 Build Scan® (commit: 3bfbdc7)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented May 21, 2024

seongbeenkim commented Apr 22, 2024 •

edited

CLAassistant commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

🔍 Build Scan® (commit: `3bfbdc7`)