From e923b0a90de74248bb7af8e37fc0d08dcc00fd4a Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Fri, 17 Nov 2023 11:22:59 -0600 Subject: [PATCH] Use correct config nodes for metrics settings; when using global meter registry set metrics config correctly (#8008) Signed-off-by: Tim Quinn --- .../metrics/api/MetricsFactoryManager.java | 11 +++- .../metrics/MetricsCdiExtension.java | 8 +-- .../microprofile/metrics/RegistryFactory.java | 5 +- webserver/observe/metrics/pom.xml | 5 ++ .../observe/metrics/MetricsFeature.java | 2 +- .../observe/metrics/MetricsObserver.java | 2 +- .../metrics/TestMetricsConfigPropagation.java | 61 +++++++++++++++++++ .../src/test/resources/application.yaml | 27 ++++++++ 8 files changed, 110 insertions(+), 11 deletions(-) create mode 100644 webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestMetricsConfigPropagation.java create mode 100644 webserver/observe/metrics/src/test/resources/application.yaml diff --git a/metrics/api/src/main/java/io/helidon/metrics/api/MetricsFactoryManager.java b/metrics/api/src/main/java/io/helidon/metrics/api/MetricsFactoryManager.java index d539d8c25b7..982520d59d0 100644 --- a/metrics/api/src/main/java/io/helidon/metrics/api/MetricsFactoryManager.java +++ b/metrics/api/src/main/java/io/helidon/metrics/api/MetricsFactoryManager.java @@ -122,7 +122,8 @@ static MetricsFactory getMetricsFactory(Config metricsConfigNode) { */ static MetricsFactory getMetricsFactory() { return access(() -> { - metricsConfigNode = Objects.requireNonNullElseGet(metricsConfigNode, GlobalConfig::config); + metricsConfigNode = Objects.requireNonNullElseGet(metricsConfigNode, + MetricsFactoryManager::externalMetricsConfig); metricsFactory = Objects.requireNonNullElseGet(metricsFactory, () -> getMetricsFactory(metricsConfigNode)); return metricsFactory; @@ -149,6 +150,14 @@ static void closeAll() { metricsFactory = null; } + private static Config externalMetricsConfig() { + Config serverFeaturesMetricsConfig = GlobalConfig.config().get("server.features.observe.observers.metrics"); + if (!serverFeaturesMetricsConfig.exists()) { + serverFeaturesMetricsConfig = GlobalConfig.config().get("metrics"); + } + return serverFeaturesMetricsConfig; + } + private static MetricsFactory completeGetInstance(MetricsConfig metricsConfig, Config metricsConfigNode) { metricsConfig = applyOverrides(metricsConfig); diff --git a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricsCdiExtension.java b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricsCdiExtension.java index b1a8a362b10..60150aa312a 100644 --- a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricsCdiExtension.java +++ b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricsCdiExtension.java @@ -551,14 +551,10 @@ private MetricsObserver configure() { MetricsFactory metricsFactory = MetricsFactory.getInstance(config); Contexts.globalContext().register(metricsFactory); - MetricsConfig.Builder metricsConfigBuilder = MetricsConfig.builder() - .config(config); - MetricsConfig metricsConfig = metricsConfigBuilder.build(); + MetricsConfig metricsConfig = metricsFactory.metricsConfig(); MeterRegistry meterRegistry = metricsFactory.globalRegistry(metricsConfig); - RegistryFactory.getInstance(meterRegistry); // initialize before first use - return builder.metricsConfig(metricsConfigBuilder) + return builder.metricsConfig(metricsConfig) .meterRegistry(meterRegistry) - .metricsConfig(MetricsConfig.builder(metricsConfig)) .build(); } diff --git a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/RegistryFactory.java b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/RegistryFactory.java index 884201f8a3b..a260d1cedaa 100644 --- a/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/RegistryFactory.java +++ b/microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/RegistryFactory.java @@ -76,8 +76,9 @@ static RegistryFactory create(MeterRegistry meterRegistry) { } static RegistryFactory getInstance(MeterRegistry meterRegistry) { - REGISTRY_FACTORY.set(create(meterRegistry)); - return REGISTRY_FACTORY.get(); + return REGISTRY_FACTORY.updateAndGet(rf -> rf != null && rf.meterRegistry == meterRegistry + ? rf + : create(meterRegistry)); } /** diff --git a/webserver/observe/metrics/pom.xml b/webserver/observe/metrics/pom.xml index 848bde9160b..d9418a5d8cb 100644 --- a/webserver/observe/metrics/pom.xml +++ b/webserver/observe/metrics/pom.xml @@ -93,6 +93,11 @@ helidon-config-yaml test + + io.helidon.webserver.testing.junit5 + helidon-webserver-testing-junit5 + test + diff --git a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java index 2ecd42b926c..6ea0a52b410 100644 --- a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java +++ b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java @@ -64,8 +64,8 @@ class MetricsFeature { private KeyPerformanceIndicatorSupport.Metrics kpiMetrics; MetricsFeature(MetricsObserverConfig config) { - this.meterRegistry = config.meterRegistry().orElseGet(MetricsFactory.getInstance()::globalRegistry); this.metricsConfig = config.metricsConfig(); + this.meterRegistry = config.meterRegistry().orElseGet(() -> MetricsFactory.getInstance().globalRegistry(metricsConfig)); } /** diff --git a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsObserver.java b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsObserver.java index d85401e5ddf..e71c07c0e79 100644 --- a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsObserver.java +++ b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsObserver.java @@ -122,7 +122,7 @@ public MetricsObserverConfig prototype() { @Override public String type() { - return "log"; + return "metrics"; } @Override diff --git a/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestMetricsConfigPropagation.java b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestMetricsConfigPropagation.java new file mode 100644 index 00000000000..0d19112d0d3 --- /dev/null +++ b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestMetricsConfigPropagation.java @@ -0,0 +1,61 @@ +/* + * Copyright (c) 2023 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.webserver.observe.metrics; + +import io.helidon.common.media.type.MediaTypes; +import io.helidon.webclient.http1.Http1Client; +import io.helidon.webclient.http1.Http1ClientResponse; +import io.helidon.webserver.WebServer; +import io.helidon.webserver.testing.junit5.ServerTest; + +import jakarta.json.JsonObject; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +@ServerTest +class TestMetricsConfigPropagation { + + private final Http1Client client; + + TestMetricsConfigPropagation(WebServer server, Http1Client client) { + this.client = client; + } + + @Test + void checkExtendedKpiMetrics() { + try (Http1ClientResponse response = client.get("/observe/metrics") + .accept(MediaTypes.APPLICATION_JSON) + .request()) { + assertThat("Metrics endpoint", response.status().code(), is(200)); + JsonObject metricsResponse = response.as(JsonObject.class); + JsonObject vendorMeters = metricsResponse.getJsonObject("vendor"); + assertThat("Vendor meters", vendorMeters, notNullValue()); + + // Make sure that the extended KPI metrics were turned on as directed by the configuration. + assertThat("Metrics KPI load", + vendorMeters.getJsonNumber("requests.load").intValue(), + greaterThan(0)); + + // Make sure that requests.count is absent because of the filtering in the config. + assertThat("Metrics KPI requests.count", vendorMeters.get("requests.count"), nullValue()); + } + } +} diff --git a/webserver/observe/metrics/src/test/resources/application.yaml b/webserver/observe/metrics/src/test/resources/application.yaml new file mode 100644 index 00000000000..204c874771f --- /dev/null +++ b/webserver/observe/metrics/src/test/resources/application.yaml @@ -0,0 +1,27 @@ +# +# Copyright (c) 2023 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. +# +server: + features: + observe: + observers: + metrics: + scoping: + scopes: + - name: vendor + filter: + include: requests.l.* + key-performance-indicators: + extended: true