[DRAFT] Allow different Micrometer backends to be configured #509

jgallimore · 2022-06-23T10:27:29Z

Issue #508 This is a draft PR to demonstrate a potential way to configure and use the other Micrometer backends that are available.

jgallimore · 2022-06-23T13:54:24Z

Looks like I have some tests to fix with this at the very least, but I'm interested in what people think about the approach.

donbourne · 2022-06-24T01:57:49Z

This looks like a pretty cool way to automatically register and configure micrometer backends. I think conceptually you're saying that when the specified list of classes in the RequiresClass annotation are present on the classpath at runtime then the corresponding Producer should be active. How does RequiresClass get used (apologies if that's obvious and I'm missing it!)?

jgallimore · 2022-06-24T09:34:20Z

That's correct - the corresponding Producer is only available to CDI if the classes referenced with the @RequiredClass annotation are available. The @RequiresClass annotation isn't actually read anywhere - just having a reference on the producer to a specific class is enough to stop CDI using the producer if those classes aren't available, as there will be a NoClassDefFoundError when trying to load the class for the producer.

Spring Boot uses a similar mechanism to load the various Micrometer backends. I quite like that this should enable the specific backend config to be done using MicroProfile config.

It looks like the unit tests don't use CDI at present, hence why there are some failures - I'll see if I can sort that out.

I'm also thinking to add an "enabled" option to each one, and default to disabled for all of them, except Prometheus. That way, vendors could just include all the micrometer backend jars (if they wanted to), allowing consumers to simply enable the ones they wish to use.

jgallimore · 2022-06-24T16:14:38Z

@donbourne In amongst my excitement, I forgot to merge this code: https://github.com/smallrye/smallrye-metrics/pull/509/files#diff-c58a21a7cc71c5ba90e694d6a23a5b459dd2812994f408acae420edb366bb697R114-R122 from another branch I had in progress where I was playing with this. This uses the @RequiresClass annotation to determine which producers to add during the BeforeBeanDiscovery phase. Sorry for any confusion. I'm adding some tests using Weld and Arquillian to ensure that this logic works correctly.

donbourne · 2022-06-24T17:52:36Z

just having a reference on the producer to a specific class is enough to stop CDI using the producer if those classes aren't available, as there will be a NoClassDefFoundError when trying to load the class for the producer.

omg, that is nefarious and awesome.

I'm also thinking to add an "enabled" option to each one, and default to disabled for all of them, except Prometheus. That way, vendors could just include all the micrometer backend jars (if they wanted to), allowing consumers to simply enable the ones they wish to use.

I like that idea.

Channyboy · 2022-06-28T03:07:08Z

implementation/src/main/java/io/smallrye/metrics/MetricRegistries.java

+            for (Bean<?> bean : beans) {
+                final Object reference = bm.getReference(bean, MeterRegistry.class, bm.createCreationalContext(bean));
+                if (MeterRegistry.class.isInstance(reference)) {
+                    meterRegistries.add(MeterRegistry.class.cast(reference));


Is it possible to keep the MPPrometheusMeterRegistry as is and just add the "new" registries to the Global Registry directly? I suppose that means this doesn't even have to be in MetricRegistries .

Seems like all the back-end registries are being included to the BASE, APPLICATION, VENDOR MPCompositeMeterRegistry and those 3 composite registries will then be added to the global meter registry anyways. Specifically, wouldn't this mean that there is a single Prometheus Meter Registry (From MicrometerBackEnd) that will have all app, base, vendor and future custom scope metrics. This would be difficult to filter out with the existing scrape calls that the Prometheus client provides for the finer granularity calls /metric?scope=___ requests.

The CounterAdapter, GaugeAdapter, etc are already registering with the Global Meter Registry directly. And since we have created and have control of the MPPrometheusRegistrys with the MeterFilters requiring the specific ThreadLocal we can register to the right scope. However, it will still register to anything else in the global registry (that isn't in our control, i.e. the other Micrometer Back ends that have no MeterFilters requiring a boolean thread local ).

Also, as part of the custom scope change, I was intending to change this to which is a little more dynamic than from before: https://github.com/Channyboy/smallrye-metrics/blob/micrometer-customScopes/implementation/src/main/java/io/smallrye/metrics/MetricRegistries.java#L111

I'm clearly biased 😂 , but adding the extra/new registries to global registry should be the most straightforward way if every "new" registration is going to the global registry. We have our own little kingdom of MPPrometheusMeterRegistrys that we can micromanage with the ThreadLocal Meter Filters to make fine granularity exporting easier and to keep the 1-to-1 Metric Registry -> Prometheus Meter Registry simple. Especially if we need to add custom scopes.

@Channyboy Thanks for the review! That sounds reasonable, let me take a swing at adding these to the global registry.

I've made the change - however - what I now see is that the application metrics from the various annotated methods do not show up in my other configured backend systems, whereas they did when using the MPCompositeRegistry previously. Having those metrics show up in Elastic / Statsd / whatever is definitely what I was shooting for. Any thoughts?

@jgallimore Sorry for the delay, these should help #511

… backend-config

…me>]

…ready enforces Numbers

… backend-config

Channyboy · 2022-09-13T00:40:18Z

implementation/src/main/java/io/smallrye/metrics/legacyapi/LegacyMetricsExtension.java

-import javax.enterprise.inject.spi.ProcessAnnotatedType;
-import javax.enterprise.inject.spi.ProcessManagedBean;
-import javax.enterprise.inject.spi.WithAnnotations;
+import javax.enterprise.inject.spi.*;


should we keep it explicit and not use a wildcard?

Agreed - I've pushed a change for this.

Channyboy · 2022-09-13T00:40:20Z

implementation/src/main/java/io/smallrye/metrics/legacyapi/LegacyMetricsExtension.java

-import java.util.Map;
-import java.util.Optional;
-import java.util.Properties;
+import java.util.*;


should we keep it explicit and not use a wildcard?

Agreed - I've pushed a change for this.

jgallimore · 2022-09-14T20:49:03Z

@Channyboy I've merged in the latest changes from the micrometer branch (javax->jakarta change) and fixed up the imports. Hopefully this is good to go.

Channyboy · 2022-09-15T04:19:01Z

Not able to test other backends yet, but it doesn't blow up for me ;)

jgallimore changed the title ~~Allow different Micrometer backends to be configured~~ [DRAFT] Allow different Micrometer backends to be configured Jun 23, 2022

jgallimore added enhancement New feature or request micrometer labels Jun 23, 2022

jgallimore force-pushed the backend-config branch 2 times, most recently from 5618fc8 to 8622394 Compare June 27, 2022 15:07

Channyboy reviewed Jun 28, 2022

View reviewed changes

jgallimore force-pushed the backend-config branch from 9015705 to 201484e Compare June 29, 2022 10:34

jgallimore force-pushed the backend-config branch 3 times, most recently from 86ce67e to 67d0984 Compare July 12, 2022 15:35

Allow different Micrometer backends to be configured

66997dc

jgallimore force-pushed the backend-config branch from 67d0984 to 66997dc Compare July 13, 2022 12:33

jgallimore and others added 5 commits August 5, 2022 16:05

Merge remote-tracking branch 'ChannyBoy/micrometer-customScopes' into…

da798f7

… backend-config

Update impl to prase query params -> /metrics[?scope=<scope>&name=<na…

2e2d548

…me>]

Tiny update regarding Gauges accepting only Numbers. Existing impl al…

b1e1e4c

…ready enforces Numbers

Merge remote-tracking branch 'ChannyBoy/micrometer-gauge-number' into…

287ba82

… backend-config

Merge branch 'micrometer' into backend-config

da6e370

Channyboy reviewed Sep 13, 2022

View reviewed changes

jgallimore added 2 commits September 14, 2022 11:54

Single class imports

bad9733

Single class imports

98b2f02

Channyboy approved these changes Sep 14, 2022

View reviewed changes

Channyboy mentioned this pull request Sep 14, 2022

Javax to Jakarta #544

Merged

jgallimore added 2 commits September 14, 2022 21:30

Merge remote-tracking branch 'upstream/micrometer' into backend-config

f982e81

Fix imports

f1ba76b

jgallimore requested a review from Channyboy September 14, 2022 20:48

Channyboy approved these changes Sep 15, 2022

View reviewed changes

jgallimore merged commit 828da06 into smallrye:micrometer Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Allow different Micrometer backends to be configured #509

[DRAFT] Allow different Micrometer backends to be configured #509

jgallimore commented Jun 23, 2022

jgallimore commented Jun 23, 2022

donbourne commented Jun 24, 2022

jgallimore commented Jun 24, 2022

jgallimore commented Jun 24, 2022

donbourne commented Jun 24, 2022 •

edited

Loading

Channyboy Jun 28, 2022 •

edited

Loading

jgallimore Jun 28, 2022

jgallimore Jun 28, 2022

Channyboy Jun 28, 2022

Channyboy Sep 13, 2022

jgallimore Sep 14, 2022

Channyboy Sep 13, 2022

jgallimore Sep 14, 2022

jgallimore commented Sep 14, 2022

Channyboy commented Sep 15, 2022

[DRAFT] Allow different Micrometer backends to be configured #509

[DRAFT] Allow different Micrometer backends to be configured #509

Conversation

jgallimore commented Jun 23, 2022

jgallimore commented Jun 23, 2022

donbourne commented Jun 24, 2022

jgallimore commented Jun 24, 2022

jgallimore commented Jun 24, 2022

donbourne commented Jun 24, 2022 • edited Loading

Channyboy Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

jgallimore Jun 28, 2022

Choose a reason for hiding this comment

jgallimore Jun 28, 2022

Choose a reason for hiding this comment

Channyboy Jun 28, 2022

Choose a reason for hiding this comment

Channyboy Sep 13, 2022

Choose a reason for hiding this comment

jgallimore Sep 14, 2022

Choose a reason for hiding this comment

Channyboy Sep 13, 2022

Choose a reason for hiding this comment

jgallimore Sep 14, 2022

Choose a reason for hiding this comment

jgallimore commented Sep 14, 2022

Channyboy commented Sep 15, 2022

donbourne commented Jun 24, 2022 •

edited

Loading

Channyboy Jun 28, 2022 •

edited

Loading