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

It is hard to notice from the metric name that the difference between the two metrics is the existence of transient services.
We may add an API and a new tag to explicitly expose the number of transient service requests in the f/u work.

Yeah, that's a good suggestion. 👍

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

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

🔍 Build Scan® (commit: `9371f15`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/f6dangr2eafsq
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/jgktuyts66nym
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/trchnvzvvczmq
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/nnvlgviplsawm
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/y6fsk52tlpdm4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/tg552oevoqtns
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ppsia33yhdrvi
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/3ir4tnr5aqgho

…Requests

…eature/line#4992/metric

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

jrhee17

Looks good overall! Left some minor comments

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

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

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

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

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! 😄

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

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

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 🤔

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

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

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

Thanks addressed. 😉

jrhee17

👍 👍 👍

minwoox · 2024-06-10T07:57:22Z

Thanks a lot, @seongbeenkim! 😉

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

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

lint

6fadd56

trustin reviewed Jun 5, 2024

View reviewed changes

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

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

Address the comment from @trustin

9371f15

jrhee17 approved these changes Jun 10, 2024

View reviewed changes

minwoox merged commit 8dd819c into line:main Jun 10, 2024
14 of 15 checks passed

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

ikhoon Jun 5, 2024

minwoox Jun 5, 2024 •

edited

ikhoon Jun 10, 2024

minwoox Jun 10, 2024

github-actions bot commented Apr 22, 2024 •

edited

jrhee17 left a comment

minwoox commented May 21, 2024

jrhee17 commented May 29, 2024

minwoox commented May 30, 2024

minwoox left a comment

ikhoon left a comment

ikhoon Jun 5, 2024

jrhee17 left a comment

trustin left a comment

trustin Jun 8, 2024

minwoox Jun 10, 2024

jrhee17 left a comment

minwoox commented Jun 10, 2024

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

Add ServerMetrics to collect metrics related server #5627

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

minwoox Apr 22, 2024

Choose a reason for hiding this comment

ikhoon Jun 5, 2024

Choose a reason for hiding this comment

minwoox Jun 5, 2024 • edited

Choose a reason for hiding this comment

ikhoon Jun 10, 2024

Choose a reason for hiding this comment

minwoox Jun 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024 • edited

🔍 Build Scan® (commit: 9371f15)

jrhee17 left a 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

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Jun 5, 2024

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin Jun 8, 2024

Choose a reason for hiding this comment

minwoox Jun 10, 2024

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox commented Jun 10, 2024

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: `9371f15`)