Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add pendingResponse to ServerMetrics #5895

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add pendingResponse to ServerMetrics #5895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5964079
Add pendingResponse to ServerMetrics
EunJungYoo Sep 7, 2024
7417ccd
minor nits
jrhee17 Sep 19, 2024
ae2d484
lint
jrhee17 Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions core/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.time.Duration;
import java.util.concurrent.Executor;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.atomic.LongAdder;

import com.google.common.base.Ticker;

Expand All @@ -29,39 +28,48 @@
*/
abstract class GracefulShutdownSupport {

static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor) {
return create(quietPeriod, blockingTaskExecutor, Ticker.systemTicker());
private final ServerMetrics serverMetrics;

static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor,
ServerMetrics serverMetrics) {
return create(quietPeriod, blockingTaskExecutor, Ticker.systemTicker(), serverMetrics);
}

static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor, Ticker ticker) {
return new DefaultGracefulShutdownSupport(quietPeriod, blockingTaskExecutor, ticker);
static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor, Ticker ticker,
ServerMetrics serverMetrics) {
return new DefaultGracefulShutdownSupport(quietPeriod, blockingTaskExecutor, ticker, serverMetrics);
}

static GracefulShutdownSupport createDisabled() {
return new DisabledGracefulShutdownSupport();
static GracefulShutdownSupport createDisabled(ServerMetrics serverMetrics) {
return new DisabledGracefulShutdownSupport(serverMetrics);
}

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

/**
* Increases the number of pending responses.
*/
final void inc() {
pendingResponses.increment();
serverMetrics.increasePendingResponses();
}

/**
* Decreases the number of pending responses.
*/
void dec() {
pendingResponses.decrement();
serverMetrics.decreasePendingResponses();
}

/**
* Returns the number of pending responses.
*/
final long pendingResponses() {
return pendingResponses.sum();
return serverMetrics.pendingResponses();
}

/**
Expand All @@ -78,6 +86,14 @@ private static final class DisabledGracefulShutdownSupport extends GracefulShutd

private volatile boolean shuttingDown;

/**
* Creates a new instance.
*
*/
private DisabledGracefulShutdownSupport(ServerMetrics serverMetrics) {
super(serverMetrics);
}

@Override
boolean isShuttingDown() {
return shuttingDown;
Expand All @@ -102,7 +118,9 @@ private static final class DefaultGracefulShutdownSupport extends GracefulShutdo
private long lastResTimeNanos;
private volatile long shutdownStartTimeNanos;

DefaultGracefulShutdownSupport(Duration quietPeriod, Executor blockingTaskExecutor, Ticker ticker) {
private DefaultGracefulShutdownSupport(Duration quietPeriod, Executor blockingTaskExecutor,
Ticker ticker, ServerMetrics serverMetrics) {
super(serverMetrics);
quietPeriodNanos = quietPeriod.toNanos();
this.blockingTaskExecutor = blockingTaskExecutor;
this.ticker = ticker;
Expand Down
6 changes: 2 additions & 4 deletions core/src/main/java/com/linecorp/armeria/server/Server.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,11 +502,11 @@ private final class ServerStartStopSupport extends StartStopSupport<Void, Void,
@Override
protected CompletionStage<Void> doStart(@Nullable Void arg) {
if (config().gracefulShutdownQuietPeriod().isZero()) {
gracefulShutdownSupport = GracefulShutdownSupport.createDisabled();
gracefulShutdownSupport = GracefulShutdownSupport.createDisabled(config.serverMetrics());
} else {
gracefulShutdownSupport =
GracefulShutdownSupport.create(config().gracefulShutdownQuietPeriod(),
config().blockingTaskExecutor());
config().blockingTaskExecutor(), config.serverMetrics());
}

// Initialize the server sockets asynchronously.
Expand Down Expand Up @@ -585,8 +585,6 @@ private void setupServerMetrics() {
final GracefulShutdownSupport gracefulShutdownSupport = this.gracefulShutdownSupport;
assert gracefulShutdownSupport != null;

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

Expand Down
19 changes: 19 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public final class ServerMetrics implements MeterBinder {
private final LongAdder activeHttp1WebSocketRequests = new LongAdder();
private final LongAdder activeHttp1Requests = new LongAdder();
private final LongAdder activeHttp2Requests = new LongAdder();
private final LongAdder pendingResponses = new LongAdder();

/**
* AtomicInteger is used to read the number of active connections frequently.
Expand Down Expand Up @@ -98,6 +99,13 @@ public long activeHttp2Requests() {
return activeHttp2Requests.longValue();
}

/**
* Returns the number of pending responses.
*/
public long pendingResponses() {
return pendingResponses.longValue();
}

/**
* Returns the number of open connections.
*/
Expand Down Expand Up @@ -153,6 +161,14 @@ void decreaseActiveConnections() {
activeConnections.decrementAndGet();
}

void increasePendingResponses() {
pendingResponses.increment();
}

void decreasePendingResponses() {
pendingResponses.decrement();
}
Comment on lines +164 to +170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikhoon That's a good suggestion. 👍


@Override
public void bindTo(MeterRegistry meterRegistry) {
meterRegistry.gauge("armeria.server.connections", activeConnections);
Expand All @@ -174,6 +190,8 @@ public void bindTo(MeterRegistry meterRegistry) {
meterRegistry.gauge(allRequestsMeterName,
ImmutableList.of(Tag.of("protocol", "http1.websocket"), Tag.of("state", "active")),
activeHttp1WebSocketRequests);
// pending responses
meterRegistry.gauge("armeria.server.pending.responses", pendingResponses);
}

@Override
Expand All @@ -185,6 +203,7 @@ public String toString() {
.add("pendingHttp2Requests", pendingHttp2Requests)
.add("activeHttp2Requests", activeHttp2Requests)
.add("activeConnections", activeConnections)
.add("pendingResponses", pendingResponses)
.toString();
}
}
8 changes: 6 additions & 2 deletions core/src/test/java/com/linecorp/armeria/server/GracefulShutdownSupportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ class GracefulShutdownSupportTest {
@Mock
private Ticker ticker;

private ServerMetrics serverMetrics;

private GracefulShutdownSupport support;
private ThreadPoolExecutor executor;

Expand All @@ -54,7 +56,9 @@ void setUp() {
0, 1, 1, TimeUnit.SECONDS, new LinkedTransferQueue<>(),
ThreadFactories.newThreadFactory("graceful-shutdown-test", true));

support = GracefulShutdownSupport.create(Duration.ofNanos(QUIET_PERIOD_NANOS), executor, ticker);
serverMetrics = new ServerMetrics();
support = GracefulShutdownSupport.create(Duration.ofNanos(QUIET_PERIOD_NANOS), executor, ticker,
serverMetrics);
}

@AfterEach
Expand All @@ -64,7 +68,7 @@ void tearDown() {

@Test
void testDisabled() {
final GracefulShutdownSupport support = GracefulShutdownSupport.createDisabled();
final GracefulShutdownSupport support = GracefulShutdownSupport.createDisabled(serverMetrics);
assertThat(support.isShuttingDown()).isFalse();
assertThat(support.completedQuietPeriod()).isTrue();
assertThat(support.isShuttingDown()).isTrue();
Expand Down
Loading