From a38bcf96af4a74f5697662606cc67aca32a835db Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Wed, 10 Jul 2024 12:46:17 +0900 Subject: [PATCH] Change conflicting meter names in `ServerMetrics` (#5804) Motivation: `MetricCollectingService` uses `.active.requests` to track the number of the current requests. If `armeria.server` is used as the prefix of the meter name, the name conflicts with "armeria.server.active.requests" in `ServerMetrics`. `ServerMetrics.activeRequests()` counts all requests including `TransientService` which is excluded by `MetricCollectingService` with no `TransientServiceOption`. So, semantically, it would be better to use `.all` to the meter name to indicate that it counts all requests. Modifications: - Rename "armeria.server.active.requests" in `ServerMetrics` to "armeria.server.all.requests{state=active}". - Rename "armeria.server.pending.requests" in `ServerMetrics` to "armeria.server.all.requests{state=pending}". Result: The meter names in `ServerMetrics` no longer conflict with `MetricCollectingService`. --- .../armeria/server/ServerMetrics.java | 21 +++++---- .../armeria/server/ServerMetricsTest.java | 47 ++++++++++++++++++- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java b/core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java index 9fd684439be..9a57bf65888 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServerMetrics.java @@ -157,17 +157,22 @@ void decreaseActiveConnections() { public void bindTo(MeterRegistry meterRegistry) { meterRegistry.gauge("armeria.server.connections", activeConnections); // pending requests - meterRegistry.gauge("armeria.server.pending.requests", - ImmutableList.of(Tag.of("protocol", "http1")), pendingHttp1Requests); - meterRegistry.gauge("armeria.server.pending.requests", - ImmutableList.of(Tag.of("protocol", "http2")), pendingHttp2Requests); + final String allRequestsMeterName = "armeria.server.all.requests"; + meterRegistry.gauge(allRequestsMeterName, + ImmutableList.of(Tag.of("protocol", "http1"), Tag.of("state", "pending")), + pendingHttp1Requests); + meterRegistry.gauge(allRequestsMeterName, + ImmutableList.of(Tag.of("protocol", "http2"), Tag.of("state", "pending")), + pendingHttp2Requests); // Active requests - meterRegistry.gauge("armeria.server.active.requests", ImmutableList.of(Tag.of("protocol", "http1")), + meterRegistry.gauge(allRequestsMeterName, + ImmutableList.of(Tag.of("protocol", "http1"), Tag.of("state", "active")), activeHttp1Requests); - meterRegistry.gauge("armeria.server.active.requests", ImmutableList.of(Tag.of("protocol", "http2")), + meterRegistry.gauge(allRequestsMeterName, + ImmutableList.of(Tag.of("protocol", "http2"), Tag.of("state", "active")), activeHttp2Requests); - meterRegistry.gauge("armeria.server.active.requests", - ImmutableList.of(Tag.of("protocol", "http1.websocket")), + meterRegistry.gauge(allRequestsMeterName, + ImmutableList.of(Tag.of("protocol", "http1.websocket"), Tag.of("state", "active")), activeHttp1WebSocketRequests); } diff --git a/core/src/test/java/com/linecorp/armeria/server/ServerMetricsTest.java b/core/src/test/java/com/linecorp/armeria/server/ServerMetricsTest.java index 5b8aa3f9444..11b21bcac4a 100644 --- a/core/src/test/java/com/linecorp/armeria/server/ServerMetricsTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/ServerMetricsTest.java @@ -19,13 +19,16 @@ import static org.awaitility.Awaitility.await; import java.time.Duration; +import java.util.Map; import java.util.concurrent.CompletableFuture; +import org.assertj.core.api.Condition; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import com.linecorp.armeria.client.BlockingWebClient; import com.linecorp.armeria.client.ClientFactory; import com.linecorp.armeria.client.ClientOptions; import com.linecorp.armeria.client.WebClient; @@ -37,14 +40,31 @@ import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.SessionProtocol; +import com.linecorp.armeria.common.metric.MeterIdPrefixFunction; +import com.linecorp.armeria.common.metric.MoreMeters; +import com.linecorp.armeria.common.prometheus.PrometheusMeterRegistries; +import com.linecorp.armeria.server.metric.MetricCollectingService; import com.linecorp.armeria.testing.junit5.server.ServerExtension; +import io.micrometer.prometheusmetrics.PrometheusMeterRegistry; + class ServerMetricsTest { @RegisterExtension - private static final ServerExtension server = new ServerExtension() { + final ServerExtension server = new ServerExtension() { + @Override + protected boolean runForEachTest() { + return true; + } + @Override protected void configure(ServerBuilder sb) throws Exception { + final PrometheusMeterRegistry prometheusMeterRegistry = PrometheusMeterRegistries.newRegistry(); + sb.meterRegistry(prometheusMeterRegistry); + // Use 'armeria.server' to make sure that the metric names are not conflicted with `ServerMetrics`. + sb.decorator(MetricCollectingService.newDecorator( + MeterIdPrefixFunction.ofDefault("armeria.server"))); + sb.requestTimeoutMillis(0) .requestAutoAbortDelayMillis(0) .service("/ok/http", new HttpService() { @@ -257,4 +277,29 @@ void checkWhenRequestTimeout(SessionProtocol sessionProtocol, long expectedPendi await().until(() -> serverMetrics.activeConnections() == 0); } } + + @CsvSource({ "H1C", "H2C" }) + @ParameterizedTest + void meterNames(SessionProtocol protocol) { + final BlockingWebClient client = BlockingWebClient.of(server.uri(protocol)); + assertThat(client.get("/ok/http").status()).isEqualTo(HttpStatus.OK); + + await().untilAsserted(() -> { + final Map meters = MoreMeters.measureAll(server.server().meterRegistry()); + // armeria.server.active.requests#value is measured by MetricCollectingService + assertThat(meters).hasKeySatisfying(new Condition("armeria.server.active.requests#value") { + @Override + public boolean matches(String key) { + return key.startsWith("armeria.server.active.requests#value{hostname.pattern="); + } + }); + + final String protocolName = protocol == SessionProtocol.H1C ? "http1" : "http2"; + // armeria.server.active.requests.all#value is measured by ServerMetrics + assertThat(meters).containsKey("armeria.server.all.requests#value{protocol=" + protocolName + + ",state=active}"); + assertThat(meters).containsKey("armeria.server.all.requests#value{protocol=" + protocolName + + ",state=pending}"); + }); + } }