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. 😉

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

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.

Question) Could "armeria.server.pending.responses" be deprecated?
I guess ServerMetric. activeRequests() == GracefulShutdownSupport.pendingResponses().

pendingResponses does not include the transient service requests, meanwhile activeRequests include them.
If we make activeRequests do not contain the transient service requests, we can deprecate pendingResponses. But I wasn't sure if it was okay. 🤔

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

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

🔍 Build Scan® (commit: `41ee0f8`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/4ifqqebrqqg2m
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/yxoruju6lzbt6
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/zz5iyd4v2doc4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/3f53m5nj42fzo
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/uiwawqqk27mww
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/sgaxi46w2n2kq
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ntndzfpvxjte2
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/tojkworoyinvc

…Requests

…eature/line#4992/metric

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

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. 😉

jrhee17 · 2024-05-29T08:13:12Z

Gentle ping 🙏

minwoox · 2024-05-30T13:13:28Z

I've pushed commits because we are about to release the next version.
There are also some failing tests that will be resolved after #5680 gets merged.

minwoox

@seongbeenkim Thanks for the all your work! 😄

ikhoon · 2024-06-03T05:26:19Z

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

+     * Returns the number of all pending requests.
+     */
+    public long pendingRequests() {
+        return pendingHttp1Requests.longValue() + pendingHttp2Requests.longValue();


nit:

Suggested change

return pendingHttp1Requests.longValue() + pendingHttp2Requests.longValue();

return pendingHttp1Requests() + pendingHttp2Requests();

ikhoon · 2024-06-03T05:27:42Z

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

+        return activeHttp1WebSocketRequests.longValue() +
+               activeHttp1Requests.longValue() +
+               activeHttp2Requests.longValue();


ditto.

Suggested change

return activeHttp1WebSocketRequests.longValue() +

activeHttp1Requests.longValue() +

activeHttp2Requests.longValue();

return activeHttp1WebSocketRequests() +

activeHttp1Requests() +

activeHttp2Requests();

ikhoon · 2024-06-03T05:43:08Z

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

+                                ServerMetrics::activeConnections);
+            meterRegistry.gauge("armeria.server.pending.requests", serverMetrics,
+                                ServerMetrics::pendingRequests);
+            meterRegistry.gauge("armeria.server.active.requests", serverMetrics,


Should we implement MeterBinder to provide a way to easily export the values as metrics? For example:

armeria/core/src/main/java/com/linecorp/armeria/common/metric/CertificateMetrics.java

Line 34 in 00d421f

final class CertificateMetrics implements MeterBinder {

That's a good suggestion. 👍

ikhoon · 2024-06-03T07:20:52Z

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

+            meterRegistry.gauge("armeria.server.pending.requests", serverMetrics,
+                                ServerMetrics::pendingRequests);
+            meterRegistry.gauge("armeria.server.active.requests", serverMetrics,
+                                ServerMetrics::activeRequests);


Should we tag SessionProtocol to the metric?

All addressed. PTAL. 😉

ikhoon

It looks like a useful feature. ServerMetric may be used to implement a graceful shutdown logic.
Thanks, @seongbeenkim and @minwoox.

ikhoon · 2024-06-05T03:40:22Z

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);


Question) Could "armeria.server.pending.responses" be deprecated?
I guess ServerMetric. activeRequests() == GracefulShutdownSupport.pendingResponses().

jrhee17

Looks good overall 👍 Pushed a small commit which cleans up dead code and tests.
Also left a comment which I'm not how to handle at the moment 🤔

jrhee17 · 2024-06-05T09:51:32Z

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

@@ -767,6 +766,14 @@ private void handleRequestOrResponseComplete() {
                    requestOrResponseComplete = true;
                    return;
                }
+                if (req.isHttp1WebSocket()) {


Sorry, I've realized that this won't be reached if the request hits the following lines:

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

Line 332 in e084508

return;

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

Line 360 in e084508

return;

I'm not sure what the best way to handle this is at the moment 😅 Any ideas are welcome

Good point. I think we need to call req.abort() for the early returned requests.
Otherwise, ByteBuf in the requests could be leaked.

We may decrease the increased metric in .abort() when req.init() isn't invoked.

Thanks for identifying these locations. 👍
I think we can proceed as follows:

Increase the pending requests in HttpRequestDecoders regardless of whether requests are aggregating or streaming.

Decrease the pending requests in the locations identified by @jrhee17.

Decrease the pending requests and increase the active requests right before invoking service.serve().

What do you think?

Sounds good to me 👍

trustin · 2024-06-05T17:31:55Z

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

+        if (routingContext.sessionProtocol().isMultiplex()) {
+            serverMetrics.increaseActiveHttp2Requests();
+        } else {
+            serverMetrics.increaseActiveHttp1Requests();


Could you elaborate on why we need to update the metrics in this class? Wouldn't it be better calling these in Http[12]RequestDecoder?

I wanted to avoid making this change in the serve0 method:
0a77e61#diff-8c8fd516b9814fa10a52ccad8660a3383cc0862c1e0cc6f91b0a456eb11a3641R458-R476

However, I think we don't have any other choice but to make the change there due to the comment:
#5627 (comment)

I see. Let's wait for @seongbeenkim to address the comments, then. 😉

I pushed the commit. PTAL. 😉
41ee0f8

Increasing pending requests in Http[12]RequestDecoder, decreasing pending requests and increasing active requests in HttpServerHandler look good to me.

This isn't just because of the comment #5627 (comment)
but also because it simplifies understanding and managing the flow of metrics.

From my review, all classes implement DecodedHttpRequest are only created in Http[12]RequestDecoder.
if we manage the metrics within Http[12]RequestDecoder and HttpServerHandler,
we could clearly observe that pending requests increase according to protocol in Http[12]RequestDecoder just before handling requests, and pending requests decreas and active requests increase in HttpServerHandler, regardless of the specific DecodedHttpRequest implementation.

However, if we manage the metrics within each implemented DecodedHttpRequest,
we couldn't see clearly when to decrease or increase pending and active requests because each class handles the metrics differently. This complexity makes it challenging to address issues when metrics need to be managed amidst the processing of requests, as noted in the comment.

Since all class implement DecodedHttpRequest are created through Http[12]RequestDecoder and are simply treated as DecodedHttpRequest in lots of classes includingHttpServerHandler,
so i think handling the metrics directly through DecodedHttpRequest seems more straightforward than basing it on whether the requests are aggregated or streaming.

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

trustin

Thank very much, @seongbeenkim and @minwoox! 🙇

trustin · 2024-06-08T16:55:00Z

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

+                                            System.nanoTime(), SystemInfo.currentTimeMicros(), true, false
+                                    );


Perhaps we can revert this cosmetic change?

Suggested change

System.nanoTime(), SystemInfo.currentTimeMicros(), true, false

);

System.nanoTime(), SystemInfo.currentTimeMicros(), true, false);

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

core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java Outdated Show resolved Hide resolved

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

minwoox added 3 commits May 30, 2024 20:50

Address comments from @jrhee17

847761b

Fix

5ac3325

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

74bbc27

minwoox added 2 commits May 31, 2024 15:57

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

32f99b8

Fix

2f1e84b

minwoox approved these changes May 31, 2024

View reviewed changes

ikhoon reviewed Jun 3, 2024

View reviewed changes

Address comment from @ikhoon

7831d5e

Merge branch 'main' into feature/line#4992/metric

51c3613

ikhoon approved these changes Jun 5, 2024

View reviewed changes

minor cleanups

dd1194f

jrhee17 reviewed Jun 5, 2024

View reviewed changes

lint

6fadd56

trustin reviewed Jun 5, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java Outdated Show resolved Hide resolved

Address the comment from @jrhee17

41ee0f8

trustin approved these changes Jun 8, 2024

View reviewed changes

	return pendingHttp1Requests.longValue() + pendingHttp2Requests.longValue();
	return pendingHttp1Requests() + pendingHttp2Requests();

		System.nanoTime(), SystemInfo.currentTimeMicros(), true, false
		);

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

minwoox Jun 5, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024 • edited

🔍 Build Scan® (commit: 41ee0f8)

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

jrhee17 commented May 29, 2024

minwoox commented May 30, 2024

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

Choose a reason for hiding this comment

ikhoon left a comment

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

minwoox Jun 7, 2024 • edited

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

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongbeenkim commented Apr 22, 2024 •

edited

CLAassistant commented Apr 22, 2024 •

edited

minwoox Jun 5, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

🔍 Build Scan® (commit: `41ee0f8`)

minwoox Jun 7, 2024 •

edited