[WFLY-14108] Introduce a base "metrics" subsystem and move microprofile-metrics to the microprofile feature-pack #13756

jmesnil · 2020-12-01T15:09:14Z

[WFLY-14108] Introduce a base "metrics" subsystem and move microprofile-metrics to the microprofile feature-pack

This PR is composed of a bunch of small commits during the development.
I will squash them in a single commit before merging but I kept them for the time being to help the review.

@bstansberry If you are reviewing this PR, could you please pay special attention to the 1st commit (that moves the microprofile-metrics extension from ee to full feature-pack (though the microprofile feature-pack). The rest of the commits are mostly code refactoring (moving some functionalities to the newly created metrics subsystem and use it from the microprofile-metrics-smallrye subsystem). The key change of all this PR is that the observability layer is now using the metrics layer instead of the microprofile-metrics layer.

I saw that some of my files are still missing copyright headers (I'll add them but I want to get a CI job running now).
I'll also add some diffs that shows that the metrics exposed by the metrics and microprofile-metrics-subsystem are identical (for WildFly management model and JVM metrics)

Finally, this PR will need a Core upgrade as the enums for the host-excludes is missing values for WildFly 20 & 21 (I've added & commented the relevant XML elements)

JIRA: https://issues.redhat.com/browse/WFLY-14108

ee-9/feature-pack/src/main/resources/feature_groups/standalone-ha.xml

jmesnil · 2020-12-01T15:26:21Z

...main/java/org/wildfly/extension/microprofile/metrics/_private/MicroProfileMetricsLogger.java

@@ -39,7 +39,7 @@
 *
 * @author <a href="http://jmesnil.net/">Jeff Mesnil</a> (c) 2018 Red Hat inc.
 */
-@MessageLogger(projectCode = "WFLYMETRICS", length = 4)
+@MessageLogger(projectCode = "WFLYMPMETRICS", length = 4)


I changed the project code but we are fine as the new metrics subsystem reuse the WFLYMETRICS code and keep the same message id for the same errors and logs

...ile/metrics-smallrye/src/main/resources/schema/wildfly-microprofile-metrics-smallrye_2_1.xsd

jmesnil · 2020-12-01T15:28:36Z

testsuite/domain/src/test/java/org/jboss/as/test/integration/domain/HostExcludesTestCase.java

-                // This module was added in EAP70, but we move it to the EAP62 extension list to allow the test passing
-                // without adding it to the host-exclude section. We don't want to expose it in the host-exclude.
-                "org.wildfly.extension.mod_cluster"
+        WILDFLY_19_0("WildFly19.0", WILDFLY_18_0,Arrays.asList(


I'm not sure why this was not spotted before but once I added the org.wildfly.extension.metrics to more recent WIldFly version, this was spotted in the CI

bstansberry · 2020-12-01T20:56:25Z

...eature-pack/galleon-common/src/main/resources/layers/standalone/observability/layer-spec.xml

@@ -2,7 +2,6 @@
 <layer-spec xmlns="urn:jboss:galleon:layer-spec:1.0" name="observability">
    <dependencies>
        <layer name="microprofile-config" optional="true"/>
-        <layer name="microprofile-metrics" optional="true"/>


We may have to redefine / override this in the 'wildfly' f-p as all of these layers are optional people might be excluding them using the 'microprofile-*' name.

If they are using 'wildfly-ee' then they have no choice; they have to change their exclude to 'metrics'. But if they use 'wildfly' their existing exclude could work.

@jfdenise @yersan WDYT? Overriding a layer isn't nice either, and having a fixed definition for 'observability' is also a good thing.

@bstansberry +1 on redefining the observability layer in wildfly FP to add back the optional dependency on the microprofile-metrics.

I don't see the new "metrics" base layer in observability. User can't replace their exclusion to "metrics". I missed something there?

@jfdenise

wildfly/ee-feature-pack/galleon-common/src/main/resources/layers/standalone/observability/layer-spec.xml

Line 4 in 9c2367e

<layer name="metrics" optional="true"/>

or am I misunderstanding what you mean?

@jmesnil , thank-you I should have looked at a commit, not the actual full PR. The 'metrics' is there, that answers my question.

WDYT? Overriding a layer isn't nice either, and having a fixed definition for 'observability' is also a good thing.

microprofile-config and microprofile-health subsystems will be also moved out from wildfly-ee. I'm not sure what observability will contain in wildfly-ee. It seems it will tend to be a complete different layer on wildfly-ee vs wildfly fp. Re-define it sounds although it seems users will see a different server configuration when using wildfly-ee and wildfly, so the same scripts won't work for the same layer. Am I wrong?

In wildfly-ee observability will contain 'health' and 'metrics', the two most important aspects of observing a server. It won't have any form of tracing. Having MP Config was always more an implementation detail not a feature of 'observability'.

So, I do think the observability concept makes sense for wildfly-ee.

Yes, redefining the layer means what your configuration looks like will be different depending on what top-level FP you use. That does make it more unlikely someone can use the same script for both. Or the script becomes more complex.

I think if people were customizing they are going to have to adapt no matter what we do. For example the 'security-enabled' attribute on the mp-metrics-smallrye subsystem is no longer functional. And if you're using wildfly-ee the mp subsystems are just gone.

My concern was oriented toward those cases thinking that since observability is going to have notable differences between the fp consumed we should have different names for such layer. I wanted to be sure it's assumable.

Then, if we re-define observability, it means an user using the ee-wildfly fp that wants to incorporate MP specs moving to wildfly fp, would need to redefine / adapt any CLI scripts if he is using observability Galleon layer (or any layer that depends upon it, e.g cloud-profile and cloud-server)

docs/src/main/asciidoc/_admin-guide/subsystem-configuration/Metrics.adoc

metrics/src/main/java/org/wildfly/extension/metrics/MetricsCollectorService.java

metrics/src/main/java/org/wildfly/extension/metrics/WildFlyMetric.java

metrics/src/main/java/org/wildfly/extension/metrics/deployment/DeploymentMetricService.java

...main/java/org/wildfly/extension/microprofile/metrics/_private/MicroProfileMetricsLogger.java

bstansberry · 2020-12-02T00:16:20Z

...c/main/java/org/wildfly/extension/microprofile/metrics/MicroProfileVendorMetricRegistry.java

+                @Override
+                public long getCount() {
+                    OptionalDouble value = metric.getValue();
+                    return  Double.valueOf(value.orElse(0)).longValue();


It would be nice if SmallRye had something like a

public interface OptionalMetric extends Metric { Optional<Metric> getOptional(); }

Then check for that when exporting and if that's the Metric type use the Optional, skipping export !optional.isPresent().

Responding with zero exposes the metric names which in theory could be privileged information.

Yes, that's a big difference between the base metrics and the microprofile-metrics-smallryesubsystem.

In the base metrics, if the metric value is not defined, I skip exposing any info about that (including its name and description):

wildfly/metrics/src/main/java/org/wildfly/extension/metrics/PrometheusExporter.java

Line 48 in 9c2367e

if (!metricValue.isPresent()) {

With MicroProfile Metrics, any registered metric is considered valid and I have no point where I can "validated" the metric value before it is printed out.

@jmesnil If you think the OptionalMetric idea is useful I can file an RFE for such an SPI in smallrye. It's kind of hacky because it extends 'Metric' just to be an acceptable type for the registry, but the normal Metric impl really isn't meant to be used.

jmesnil · 2020-12-02T21:01:31Z

I've checked that this PR does not introduce (too many) changes in the metrics output compared to the existing branch.
I've added a gist at https://gist.github.com/jmesnil/6253ea2150f22f379ce5a9b7d569d795 to show 4 outputs:

master-standalone-full.txt is the output from the master branch with the standalone-full.xml config
master-standalone-microprofile.txt is the output from the master branch with the standalone-microprofile.xml config
current-standalone-full.txt is the output from this PR with the standalone-full.xml config
current-standalone-microprofile.txt is the output from the master branch with the standalone-microprofilel.xml config

There is no difference between master-standalone-microprofile.txt & current-standalone-microprofile.txt (as they both use smallrye-metrics to output the metrics)/

There are some differences between master-standalone-full.txt and current-standalone-microprofile.txt:

the order of the tags may have changed (that's ok)
the microprofile_scope="vendor" tag is no longer present in the current base output (that's ok)
the vendor_memoryPool memory pool names have changed. The master branch is silly and makes consuming these metrics very difficult. This PR has moved the name of the memory pool in a tag (which is more consistent with all the other metrics output).
- Before, it was vendor_memoryPool_Code_Cache_usage_bytes & vendor_memoryPool_Compressed_Class_Space_usage_bytes(with different descriptions)
- Now it is vendor_memoryPool_usage_bytes{name="Code Cache"} & vendor_memoryPool_usage_bytes{name="Compressed Class Space"} (with a single description)
I'm pretty sure that the actual output for counter metrics with units is wrong. smallrye-metrics output the metric with the name wildfly_undertow_total_request_time_total_seconds while I think it should be wildfly_undertow_total_request_time_seconds_total. I'm using the same convention in the base metrics` subsystem so I'm bug-compatible but I'll open an issue agains smallrye-metrics (or microprofile-metrics) to clarify this later.

Note that I have only checked the output using standalone-full.xml and standalone-microprofile.xml with a deployed JAX-RS app (so I had some Undertow deployment metrics). It does not cover all metrics that can be exposed by WildFly but it gives a pretty good idea about the compatibility.
As far as I am concerned, we're good (we could object about the vendor_memoryPool metrics but (a) they are not required by MicroProfile Metrics, (b) the current convention is inconsistent)

metrics/src/main/java/org/wildfly/extension/metrics/_private/MetricsLogger.java

bstansberry · 2020-12-02T23:50:24Z

@jmesnil Thanks for the updates. I recommend calling out the vendor_memoryPool thing on your requirements doc, just because it says there will be no name changes. I agree the use of tags is better and the other way could be regarded as a bug.

jmesnil · 2020-12-03T08:05:58Z

@jmesnil Thanks for the updates. I recommend calling out the vendor_memoryPool thing on your requirements doc, just because it says there will be no name changes. I agree the use of tags is better and the other way could be regarded as a bug.

done

bstansberry

LGTM.

bstansberry

I found some issues; please see jmesnil#4

docs/src/main/asciidoc/_admin-guide/subsystem-configuration/Metrics.adoc

mchoma · 2020-12-08T09:28:46Z

docs/src/main/asciidoc/_admin-guide/subsystem-configuration/MicroProfile_Metrics.adoc

@@ -35,9 +35,9 @@ standalone configurations explicitly sets it to `false` to accept unauthenticate
 The Metric HTTP endpoint is accessible on WildFly HTTP management interface http://localhost:9990/metrics[http://localhost:9990/metrics].

 Secured access to the HTTP endpoint is controlled by the `security-enabled` attribute of the `/subsystem=microprofile-metrics-smallrye` resource.
-If it is set to `true`, the HTTP client must be authenticated.
+The value of this attribute will override the `security-enabled` attribute of the `/subsystem=metrics` resource (documented in <<metrics-http-endpoint,Metrics subsystem configuration guide>>).


What about exposed-subsystems and prefix does same overriding logic apply as well in case of these properties.

If metrics subsystem is responsible for registring /metrics http endpoint and will be always present, do we really need this overriding logic?

exposed-subsystems and prefix are specific to the microprofile-metrics-smallrye and do not provide this overriding logic.

This special case to handle the security-enabled is to keep it backwards compatible. Users that change this value expects to be able to use it. Making it a no-op would have break their configuration.

jmesnil · 2020-12-08T13:16:37Z

I've squashed all the commits (incuding the ones that Brian created in jmesnil#4)

…pack by providing it through the wildfly-mp-feature-pack-galleon-common Galleon feature-pack. JIRA: https://issues.redhat.com/browse/WFLY-14108 Signed-off-by: Jeff Mesnil <jmesnil@redhat.com>

…pack Add a `metrics` subsystem that provides base observability features for the ee-feature-pack. This subsystem will is responsible for the /metrics HTTP endpoint on the management interface. When the `microprofile-metrics-smallrye` subsystem is installed, it will "take over" this HTTP endpoint to provide the MicroProfile Metrics feature on it. This base `metrics` subsystems exposes metrics for the WildFly management model (subsystem and deployment subtrees) as well as metrics from the JVM (using JMX to read them). JIRA: https://issues.redhat.com/browse/WFLY-14108

JIRA: https://issues.redhat.com/browse/WFLY-14134 Signed-off-by: Jeff Mesnil <jmesnil@redhat.com>

jmesnil · 2020-12-14T08:15:25Z

@bstansberry Pre-check is "Done" for EAP7-1566. I've removed the missing-reqs label and this PR can be merged.

jmesnil added 22.x Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements labels Dec 1, 2020

jmesnil commented Dec 1, 2020

View reviewed changes

ee-9/feature-pack/src/main/resources/feature_groups/standalone-ha.xml Outdated Show resolved Hide resolved

wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Dec 1, 2020

jmesnil commented Dec 1, 2020

View reviewed changes

...ile/metrics-smallrye/src/main/resources/schema/wildfly-microprofile-metrics-smallrye_2_1.xsd Outdated Show resolved Hide resolved

jmesnil commented Dec 1, 2020

View reviewed changes