Add pendingResponse to ServerMetrics #5895

EunJungYoo · 2024-09-07T13:18:45Z

Motivation:

Related Issue: Add pendingResponses to ServerMetrics #5666
Currently, GracefulShutdownSupport manages the pendingResponse metric. With the introduction of the ServerMetrics class, which handles server-related metrics, it’s necessary to consolidate the management of pendingResponse within ServerMetrics for better organization and consistency.

Modifications:

Moved the pendingResponse metric from GracefulShutdownSupport to ServerMetrics.
Renamed the pendingResponses method to activeNonTransientResponses to better reflect its purpose and functionality.

Result:

Closes Add pendingResponses to ServerMetrics #5666 . (If this resolves the issue.)
Server-related metrics are now managed within the ServerMetrics class, ensuring a centralized and consistent approach.

CLAassistant · 2024-09-07T13:18:54Z

All committers have signed the CLA.

github-actions · 2024-09-07T14:02:20Z

🔍 Build Scan® (commit: `ae2d484`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/3mwdwq6e5fhzw
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/hj7dkq47b2qqy
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/ziczjbhx2w5p6
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/lctpsq5ijhebo
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/v7qxeovlu3enq
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/csgizwcy6irxw
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/m3pydak4xcvfy

codecov · 2024-09-07T16:48:02Z

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.66%. Comparing base (9a29b39) to head (736963a).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
...ava/com/linecorp/armeria/server/ServerMetrics.java	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5895      +/-   ##
============================================
- Coverage     74.66%   74.66%   -0.01%     
- Complexity    21681    21704      +23     
============================================
  Files          1896     1897       +1     
  Lines         80320    80391      +71     
  Branches      10548    10553       +5     
============================================
+ Hits          59974    60023      +49     
- Misses        15318    15339      +21     
- Partials       5028     5029       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jrhee17

Only left nit comments, basically looks good to me 👍

jrhee17 · 2024-09-09T02:59:08Z

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

+    private final ServerMetrics serverMetrics;
+
+    /**
+     * Creates a new instance.
+     */
+    GracefulShutdownSupport(ServerMetrics serverMetrics) {
+        this.serverMetrics = serverMetrics;
    }


nit; member fields/constructors can go under the static declarations

https://armeria.dev/community/developer-guide#organize

I have made the corrections! I will be more careful with the order next time

jrhee17 · 2024-09-09T02:59:25Z

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

+    /**
+     * Creates a new instance.
+     */
+    GracefulShutdownSupport(ServerMetrics serverMetrics) {


Can be private, ditto for the other ctors

Suggested change

GracefulShutdownSupport(ServerMetrics serverMetrics) {

private GracefulShutdownSupport(ServerMetrics serverMetrics) {

I have changed it to private

jrhee17 · 2024-09-09T03:02:50Z

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

@@ -174,6 +190,10 @@ public void bindTo(MeterRegistry meterRegistry) {
        meterRegistry.gauge(allRequestsMeterName,
                            ImmutableList.of(Tag.of("protocol", "http1.websocket"), Tag.of("state", "active")),
                            activeHttp1WebSocketRequests);
+        // pending non-transient responses
+        meterRegistry.gauge(allRequestsMeterName,


Question) Can we keep the old format (armeria.server.pending.responses) so that metric name related breaking changes aren't made?

I have changed it to the old format

jrhee17 · 2024-09-09T03:05:12Z

Can you also sign the CLA? #5895 (comment)

EunJungYoo · 2024-09-15T09:35:08Z

Can you also sign the CLA? #5895 (comment)
I have finished signing the CLA

jrhee17

Added a minor commit, let me know if anything doesn't make sense. Thanks @EunJungYoo 🙇 👍

ikhoon · 2024-09-19T06:21:30Z

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

+    void increasePendingResponses() {
+        pendingResponses.increment();
+    }
+
+    void decreasePendingResponses() {
+        pendingResponses.decrement();
+    }


The number of pendingResponses seems the same as activeRequests. If so, we don't need to record the same value twice in different places.

Should we remove inc() and dec() in GracefulShutdownSupport and use activeRequests() for "armeria.server.pending.responses"

#5627 (comment)

There was a previous mention of the difference between pendingResponses and activeRequests. If we were to merge them, additional modifications might be necessary. Would it be better to merge them?

The current changes are a clean-up but do not provide additional benefits to users.

I prefer to add metrics for transient requests in ServerMetrics

Expose a meter with state=transient tag for TransientService

Exclude transient requests from activeRequests

Switch state=pending to state=active or state=transient

Add useful methods
public long transientHttp1Requests() { return transientHttp1Requests.longValue(); } public long transientHttp2Requests() { return transientHttp2Requests.longValue(); } public long transientRequests() { return transientHttp1Requests() + transientHttp2Requests(); } public long allRequests() { return pendingRequests() + activeRequests() + transientRequests(); }

After those changes, pendingResponses will be the same as activeRequests.

@ikhoon That's a good suggestion. 👍

EunJungYoo requested review from ikhoon, jrhee17, minwoox and trustin as code owners September 7, 2024 13:18

EunJungYoo force-pushed the issue-5666 branch from 4d5d2e8 to d376d4b Compare September 7, 2024 16:10

EunJungYoo force-pushed the issue-5666 branch from d376d4b to 736963a Compare September 7, 2024 17:07

jrhee17 reviewed Sep 9, 2024

View reviewed changes

jrhee17 added the improvement label Sep 9, 2024

jrhee17 added this to the 1.31.0 milestone Sep 9, 2024

EunJungYoo force-pushed the issue-5666 branch from 736963a to e2dc69c Compare September 12, 2024 10:23

Add pendingResponse to ServerMetrics

5964079

EunJungYoo force-pushed the issue-5666 branch from e2dc69c to 5964079 Compare September 15, 2024 09:30

minor nits

7417ccd

jrhee17 approved these changes Sep 19, 2024

View reviewed changes

lint

ae2d484

ikhoon reviewed Sep 19, 2024

View reviewed changes

github-actions bot added the Stale label Oct 25, 2024

jrhee17 modified the milestones: 1.31.0, 1.32.0 Nov 5, 2024

github-actions bot removed the Stale label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pendingResponse to ServerMetrics #5895

Add pendingResponse to ServerMetrics #5895

EunJungYoo commented Sep 7, 2024

CLAassistant commented Sep 7, 2024 •

edited

Loading

github-actions bot commented Sep 7, 2024 •

edited

Loading

codecov bot commented Sep 7, 2024 •

edited

Loading

jrhee17 left a comment

jrhee17 Sep 9, 2024

EunJungYoo Sep 15, 2024

jrhee17 Sep 9, 2024

EunJungYoo Sep 15, 2024

jrhee17 Sep 9, 2024

EunJungYoo Sep 15, 2024

jrhee17 commented Sep 9, 2024

EunJungYoo commented Sep 15, 2024

jrhee17 left a comment

ikhoon Sep 19, 2024

EunJungYoo Sep 19, 2024

ikhoon Sep 20, 2024

minwoox Sep 23, 2024

	GracefulShutdownSupport(ServerMetrics serverMetrics) {
	private GracefulShutdownSupport(ServerMetrics serverMetrics) {

Add pendingResponse to ServerMetrics #5895

Are you sure you want to change the base?

Add pendingResponse to ServerMetrics #5895

Conversation

EunJungYoo commented Sep 7, 2024

CLAassistant commented Sep 7, 2024 • edited Loading

github-actions bot commented Sep 7, 2024 • edited Loading

🔍 Build Scan® (commit: ae2d484)

codecov bot commented Sep 7, 2024 • edited Loading

Codecov Report

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

jrhee17 commented Sep 9, 2024

EunJungYoo commented Sep 15, 2024

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

CLAassistant commented Sep 7, 2024 •

edited

Loading

github-actions bot commented Sep 7, 2024 •

edited

Loading

🔍 Build Scan® (commit: `ae2d484`)

codecov bot commented Sep 7, 2024 •

edited

Loading