From a27efa5ad8d03d72991a8f3dca87e7e78debec10 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Thu, 12 Aug 2021 09:02:34 -0500 Subject: [PATCH] Metrics and routings (#3260) * Handle the case where no downstream handler marks the explicit start of processing of a request * xxxSupport#postConfigureEndpoint accepts both default and endpoint routing rules; metrics impl uses them appropriately now; add mp/metrics test with metrics on its own socket Signed-off-by: tim.quinn@oracle.com --- .../micrometer/MicrometerSupport.java | 6 +- .../io/helidon/metrics/MetricsSupport.java | 22 ++-- .../metrics/TestMetricsOnOwnSocket.java | 112 ++++++++++++++++++ .../restcdi/HelidonRestCdiExtension.java | 20 +--- .../rest/HelidonRestServiceSupport.java | 33 ++++-- ...KeyPerformanceIndicatorContextFactory.java | 19 ++- 6 files changed, 175 insertions(+), 37 deletions(-) create mode 100644 microprofile/metrics/src/test/java/io/helidon/microprofile/metrics/TestMetricsOnOwnSocket.java diff --git a/integrations/micrometer/micrometer/src/main/java/io/helidon/integrations/micrometer/MicrometerSupport.java b/integrations/micrometer/micrometer/src/main/java/io/helidon/integrations/micrometer/MicrometerSupport.java index a7029215302..5f25e0bc804 100644 --- a/integrations/micrometer/micrometer/src/main/java/io/helidon/integrations/micrometer/MicrometerSupport.java +++ b/integrations/micrometer/micrometer/src/main/java/io/helidon/integrations/micrometer/MicrometerSupport.java @@ -95,12 +95,12 @@ public MeterRegistry registry() { @Override public void update(Routing.Rules rules) { - configureEndpoint(rules); + configureEndpoint(rules, rules); } @Override - protected void postConfigureEndpoint(Routing.Rules rules) { - rules + protected void postConfigureEndpoint(Routing.Rules defaultRules, Routing.Rules serviceEndpointRoutingRules) { + defaultRules .any(new MetricsContextHandler(meterRegistryFactory.meterRegistry())) .get(context(), this::getOrOptions) .options(context(), this::getOrOptions); diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/MetricsSupport.java b/metrics/metrics/src/main/java/io/helidon/metrics/MetricsSupport.java index 3b72f0b8f1d..6f09198a918 100644 --- a/metrics/metrics/src/main/java/io/helidon/metrics/MetricsSupport.java +++ b/metrics/metrics/src/main/java/io/helidon/metrics/MetricsSupport.java @@ -379,22 +379,26 @@ public void configureVendorMetrics(String routingName, * {@link #update(io.helidon.webserver.Routing.Rules)} (e.g. you should not * use both, as otherwise you would register the endpoint twice) * - * @param rules routing rules (also accepts - * {@link io.helidon.webserver.Routing.Builder} + * @param defaultRules routing rules for default routing (also accepts {@link io.helidon.webserver.Routing.Builder}) + * @param serviceEndpointRoutingRules possibly different rules for the metrics endpoint routing */ @Override - protected void postConfigureEndpoint(Routing.Rules rules) { + protected void postConfigureEndpoint(Routing.Rules defaultRules, Routing.Rules serviceEndpointRoutingRules) { Registry base = rf.getARegistry(MetricRegistry.Type.BASE); Registry vendor = rf.getARegistry(MetricRegistry.Type.VENDOR); Registry app = rf.getARegistry(MetricRegistry.Type.APPLICATION); // register the metric registry and factory to be available to all - rules.any(new MetricsContextHandler(app, rf)); + MetricsContextHandler metricsContextHandler = new MetricsContextHandler(app, rf); + defaultRules.any(metricsContextHandler); + if (defaultRules != serviceEndpointRoutingRules) { + serviceEndpointRoutingRules.any(metricsContextHandler); + } - configureVendorMetrics(null, rules); + configureVendorMetrics(null, defaultRules); // routing to root of metrics - rules.get(context(), (req, res) -> getMultiple(req, res, base, app, vendor)) + serviceEndpointRoutingRules.get(context(), (req, res) -> getMultiple(req, res, base, app, vendor)) .options(context(), (req, res) -> optionsMultiple(req, res, base, app, vendor)); // routing to each scope @@ -402,7 +406,7 @@ protected void postConfigureEndpoint(Routing.Rules rules) { .forEach(registry -> { String type = registry.type(); - rules.get(context() + "/" + type, (req, res) -> getAll(req, res, registry)) + serviceEndpointRoutingRules.get(context() + "/" + type, (req, res) -> getAll(req, res, registry)) .get(context() + "/" + type + "/{metric}", (req, res) -> getByName(req, res, registry)) .options(context() + "/" + type, (req, res) -> optionsAll(req, res, registry)) .options(context() + "/" + type + "/{metric}", (req, res) -> optionsOne(req, res, registry)); @@ -415,7 +419,7 @@ protected void postConfigureEndpoint(Routing.Rules rules) { * {@link io.helidon.webserver.Routing.Builder#register(io.helidon.webserver.Service...)} * rather than calling this method directly. If multiple sockets (and * routings) should be supported, you can use the - * {@link #configureEndpoint(io.helidon.webserver.Routing.Rules)}, and + * {@link #configureEndpoint(io.helidon.webserver.Routing.Rules, io.helidon.webserver.Routing.Rules)}, and * {@link #configureVendorMetrics(String, io.helidon.webserver.Routing.Rules)} * methods. * @@ -423,7 +427,7 @@ protected void postConfigureEndpoint(Routing.Rules rules) { */ @Override public void update(Routing.Rules rules) { - configureEndpoint(rules); + configureEndpoint(rules, rules); } private static KeyPerformanceIndicatorSupport.Context kpiContext(ServerRequest request) { diff --git a/microprofile/metrics/src/test/java/io/helidon/microprofile/metrics/TestMetricsOnOwnSocket.java b/microprofile/metrics/src/test/java/io/helidon/microprofile/metrics/TestMetricsOnOwnSocket.java new file mode 100644 index 00000000000..17e81f57038 --- /dev/null +++ b/microprofile/metrics/src/test/java/io/helidon/microprofile/metrics/TestMetricsOnOwnSocket.java @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2021 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.helidon.microprofile.metrics; + +import javax.inject.Inject; +import javax.json.JsonObject; +import javax.ws.rs.client.ClientBuilder; +import javax.ws.rs.client.Invocation; +import javax.ws.rs.client.WebTarget; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; + +import io.helidon.common.http.Http; +import io.helidon.microprofile.server.ServerCdiExtension; +import io.helidon.microprofile.tests.junit5.AddConfig; +import io.helidon.microprofile.tests.junit5.HelidonTest; + +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Order; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.is; + +@HelidonTest() +// Set up the metrics endpoint on its own socket +@AddConfig(key = "server.sockets.0.name", value = "metrics") +// No port setting, so use any available one +@AddConfig(key = "server.sockets.0.bind-address", value = "0.0.0.0") +@AddConfig(key = "metrics.routing", value = "metrics") +@AddConfig(key = "metrics.key-performance-indicators.extended", value = "true") +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +public class TestMetricsOnOwnSocket { + + private Invocation metricsInvocation= null; + + private static int loadCountBeforePingingMetrics = -1; + + @Inject + private ServerCdiExtension serverCdiExtension; + + @Inject + private WebTarget webTarget; + + Invocation metricsInvocation() { + if (metricsInvocation == null) { + int metricsPort = serverCdiExtension.port("metrics"); + metricsInvocation = ClientBuilder.newClient() + .target(String.format("http://localhost:%d/metrics/vendor", metricsPort)) + .request(MediaType.APPLICATION_JSON_TYPE) + .buildGet(); + } + return metricsInvocation; + } + + @Order(0) + @Test + void checkMetricsBeforeRequests() { + // Just record the load count value. Other tests might have run earlier so we cannot assume the count is exactly 0. + loadCountBeforePingingMetrics = getRequestsLoadCount("Pre-test"); + assertThat("Pre-test load count", loadCountBeforePingingMetrics, is(greaterThanOrEqualTo(0))); + + } + + @Order(1) + @Test + void checkMessageFromDefaultRouting() { + try (Response r = webTarget + .path("helloworld") + .request(MediaType.TEXT_PLAIN_TYPE) + .get()) { + + assertThat("Response code getting greeting", r.getStatus(), is(Http.Status.OK_200.code())); + } + } + + @Order(2) + @Test + void checkMetricsAfterGet() { + int load = getRequestsLoadCount("Post-test"); + assertThat("Change in load count", load - loadCountBeforePingingMetrics, is(1)); + + } + + private int getRequestsLoadCount(String descr) { + try (Response r = metricsInvocation().invoke()) { + assertThat(descr + " metrics sampling response", r.getStatus(), is(Http.Status.OK_200.code())); + + JsonObject metrics = r.readEntity(JsonObject.class); + assertThat("Check for requests.load", metrics.containsKey("requests.load"), is(true)); + JsonObject load = metrics.getJsonObject("requests.load"); + assertThat("JSON requests.load contains count", load.containsKey("count"), is(true)); + + return load.getInt("count"); + } + } +} diff --git a/service-common/rest-cdi/src/main/java/io/helidon/servicecommon/restcdi/HelidonRestCdiExtension.java b/service-common/rest-cdi/src/main/java/io/helidon/servicecommon/restcdi/HelidonRestCdiExtension.java index c89f6707611..a01c2aa1a77 100644 --- a/service-common/rest-cdi/src/main/java/io/helidon/servicecommon/restcdi/HelidonRestCdiExtension.java +++ b/service-common/rest-cdi/src/main/java/io/helidon/servicecommon/restcdi/HelidonRestCdiExtension.java @@ -46,7 +46,7 @@ import javax.interceptor.Interceptor; import io.helidon.config.Config; -import io.helidon.config.ConfigValue; +import io.helidon.microprofile.server.RoutingBuilders; import io.helidon.microprofile.server.ServerCdiExtension; import io.helidon.servicecommon.rest.HelidonRestServiceSupport; import io.helidon.webserver.Routing; @@ -239,23 +239,11 @@ protected Routing.Builder registerService( Config config = ((Config) ConfigProvider.getConfig()).get(configPrefix); serviceSupport = serviceSupportFactory.apply(config); - ConfigValue routingNameConfig = config.get("routing") - .asString(); - Routing.Builder defaultRouting = server.serverRoutingBuilder(); + RoutingBuilders routingBuilders = RoutingBuilders.create(config); - Routing.Builder endpointRouting = defaultRouting; + serviceSupport.configureEndpoint(routingBuilders.defaultRoutingBuilder(), routingBuilders.routingBuilder()); - if (routingNameConfig.isPresent()) { - String routingName = routingNameConfig.get(); - // support for overriding this back to default routing using config - if (!"@default".equals(routingName)) { - endpointRouting = server.serverNamedRoutingBuilder(routingName); - } - } - - serviceSupport.configureEndpoint(endpointRouting); - - return defaultRouting; + return routingBuilders.defaultRoutingBuilder(); } protected T serviceSupport() { diff --git a/service-common/rest/src/main/java/io/helidon/servicecommon/rest/HelidonRestServiceSupport.java b/service-common/rest/src/main/java/io/helidon/servicecommon/rest/HelidonRestServiceSupport.java index 9d9c8c775f6..73184219c2b 100644 --- a/service-common/rest/src/main/java/io/helidon/servicecommon/rest/HelidonRestServiceSupport.java +++ b/service-common/rest/src/main/java/io/helidon/servicecommon/rest/HelidonRestServiceSupport.java @@ -35,7 +35,8 @@ * * *

- * Concrete implementations must implement {@link #postConfigureEndpoint(Routing.Rules)} to do any service-specific routing. + * Concrete implementations must implement {@link #postConfigureEndpoint(Routing.Rules, Routing.Rules)} to do any + * service-specific routing. * See also the {@link Builder} information for possible additional overrides. *

* @@ -59,6 +60,18 @@ protected HelidonRestServiceSupport(Logger logger, Builder builder, String corsEnabledServiceHelper = CorsEnabledServiceHelper.create(serviceName, builder.crossOriginConfig); } + /** + * Avoid using this obsolete method. Use {@link #configureEndpoint(Routing.Rules, Routing.Rules)} instead. (Neither method + * should typically invoked directly from user code.) + * + * @param rules routing rules (also accepts + * {@link io.helidon.webserver.Routing.Builder} + */ + @Deprecated + public final void configureEndpoint(Routing.Rules rules) { + configureEndpoint(rules, rules); + } + /** * Configures service endpoint on the provided routing rules. This method * just adds the endpoint path (as defaulted or configured). @@ -66,21 +79,25 @@ protected HelidonRestServiceSupport(Logger logger, Builder builder, String * {@link #update(io.helidon.webserver.Routing.Rules)} (e.g. you should not * use both, as otherwise you would register the endpoint twice) * - * @param rules routing rules (also accepts - * {@link io.helidon.webserver.Routing.Builder} + * @param defaultRules default routing rules (also accepts {@link io.helidon.webserver.Routing.Builder} + * @param serviceEndpointRoutingRules actual rules (if different from default) for the service endpoint */ - public final void configureEndpoint(Routing.Rules rules) { + public final void configureEndpoint(Routing.Rules defaultRules, Routing.Rules serviceEndpointRoutingRules) { // CORS first - rules.any(context, corsEnabledServiceHelper.processor()); - postConfigureEndpoint(rules); + defaultRules.any(context, corsEnabledServiceHelper.processor()); + if (defaultRules != serviceEndpointRoutingRules) { + serviceEndpointRoutingRules.any(context, corsEnabledServiceHelper.processor()); + } + postConfigureEndpoint(defaultRules, serviceEndpointRoutingRules); } /** * Concrete implementations override this method to perform any service-specific routing set-up. * - * @param rules {@code Routing.Rules} to be updated + * @param defaultRules default {@code Routing.Rules} to be updated + * @param serviceEndpointRoutingRules actual rules (if different from the default ones) to be updated for the service endpoint */ - protected abstract void postConfigureEndpoint(Routing.Rules rules); + protected abstract void postConfigureEndpoint(Routing.Rules defaultRules, Routing.Rules serviceEndpointRoutingRules); protected String context() { return context; diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/KeyPerformanceIndicatorContextFactory.java b/webserver/webserver/src/main/java/io/helidon/webserver/KeyPerformanceIndicatorContextFactory.java index 6e8b16dd197..a4189587427 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/KeyPerformanceIndicatorContextFactory.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/KeyPerformanceIndicatorContextFactory.java @@ -69,15 +69,32 @@ protected Metrics kpiMetrics() { private static class DeferrableRequestContext extends ImmediateRequestContext implements KeyPerformanceIndicatorSupport.DeferrableRequestContext { + private boolean isStartRecorded = false; + @Override public void requestHandlingStarted(Metrics kpiMetrics) { kpiMetrics(kpiMetrics); + recordStartTime(); // In case no handler in the chain manages the start-of-processing moment. kpiMetrics.onRequestReceived(); } @Override public void requestProcessingStarted() { - recordStartTime(); + recordStartTime(); // Overwrite the previously-recorded, provisional start time, now that we have a real one. + recordProcessingStarted(); + } + + @Override + public void requestProcessingCompleted(boolean isSuccessful) { + // No handler explicitly dealt with start-of-processing, so approximate it based on request receipt time. + if (!isStartRecorded) { + recordProcessingStarted(); + } + super.requestProcessingCompleted(isSuccessful); + } + + private void recordProcessingStarted() { + isStartRecorded = true; Metrics kpiMetrics = kpiMetrics(); if (kpiMetrics != null) { kpiMetrics().onRequestStarted();