Quarkus: Offer a build time to optionally enlist metrics

[emmanuelbernard]

Like health, extensions should optionally enlist metrics.
We could use a build item pushed by the metrics extension to detect presence.
Or we could push the metrics code as a build item and the metrics extension would enable / disable them.

Needs thinking.

CC @kenfinnigan for metrics input, @gunnarmorling for usage input, @loicmathieu for health extension design input

[kenfinnigan]

I think it's reasonable to offer a way for extensions to add "vendor" scoped metrics that might be of interest.

cc @jmartisk

[gunnarmorling]

From a Kafka Streams perspective, what I'd like to do for its extension is to provide the metrics we discussed in the blog post here automatically OOTB, if we somehow detect that the metrics extension capabilities are available.

[geoand]

Could we not just have each extension register some CDI bean that adds whatever is necessary?
That would obviously require the metrics extension to take the beans into account (if it doesn't already).

What @gunnarmorling showed in the link could be generated by the KafkStreams aextension couldn't it? If we think it makes sense, then I'd definitely be willing to help out.

[kenfinnigan]

Each extension could certainly use the MetricRegistry to define its metrics directly instead of using a build item. However, using a build item means we can process them via a Recorder so they can be done at native image generation time instead of on startup.

[jmartisk]

If an extension needs to register a static set of metrics that is always the same, it is possible to use metric annotations directly in the extension, that's the easiest way, and the would get registered at build. But 1) this is probably not flexible enough for most cases 2) it is not possible atm to declare that these metrics should be in the vendor scope - we would need a custom mechanism for that, but that should be relatively easy if needed.

For dynamically created metrics, I can imagine a MetricBuildItem that contains required metadata so it can be registered in STATIC_INIT. Not sure at the moment how much of a problem could be if the extension wanted to get hold of a reference to the metric instance during build and save it somewhere for runtime use (for example HistogramImpl doesn't have a default constructor). But looking it up in the vendor registry at runtime could probably be enough, that's just a lookup in a map.

It is also possible for an extension to simply look up the MetricRegistry and register the metrics by itself. The Jaeger extension currently does this (at runtime, but it could be done at build time as well) https://github.com/quarkusio/quarkus/blob/0.23.2/extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/QuarkusJaegerMetricsFactory.java#L20

[geoand]

The CDI container is also booted at static init (see this) so any metrics related beans would also be processed at native image generation time.

[jmartisk]

I came up with a crude implementation that I'd like to get feedback on (the Health changes are obviously not part of the proposal, just an example how it can be used): https://github.com/jmartisk/quarkus/commit/2be8e99a08ad5220804fe839cc3201653b8ec0bf

It introduces AdditionalMetricBuildItem that any extension can produce, SmallRye Metrics extension will then pick it up and register the metric.

The example for Health extension adds four metrics:
Three counters that count how many health/readiness/liveness checks were performed
And one gauge that is implemented by a Callable<Number> in runtime code

This introduces a hard dependency on metrics from the other extension. Not sure if it can somehow be made optional (and skip the metrics stuff if metrics are not available).

Just want to know if this model looks suitable for the affected extensions

[geoand]

This introduces a hard dependency on metrics from the other extension. Not sure if it can somehow be made optional (and skip the metrics stuff if metrics are not available).

The AdditionalMetricBuildItem could be moved to a new spi module. Then each extension that needs to produce it would just depend on that SPI module.
This is how similar problems have been dealt with in other parts of the code (for example smallrye-health also contains an spi module which is used by the agroal extension).

[jmartisk]

Good to know, thanks @geoand ! Will do that if this model gets generally accepted

[loicmathieu]

The build item must be in an SPI and the extension must define the metric
extention as *optional dependency *so when a user don't include it the
dependencies are not added to the artifact.
You really should check how we did it for extension health check:
https://quarkus.io/guides/extension-authors-guide#extension-health-check
An we need a way to disabled it, by extension, as collecting metrics have
performance impact.

I also think that the current metrics generated by Jaeger should be
retrofitted to using this new mechanism ...

Le mar. 15 oct. 2019 à 09:57, Jan Martiška [e-mail] a
écrit :

Good to know, thanks @geoand https://github.com/geoand ! Will do that
if this model gets generally accepted

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/quarkusio/quarkus/issues/4375?email_source=notifications&email_token=AAN4DAKQIHX4ZHE6FLK4YEDQOVZWFA5CNFSM4I5PBIFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBHZ24Y#issuecomment-542088563,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAN4DAM3YRWJW6CWKCXRV7DQOVZWFANCNFSM4I5PBIFA
.

[geoand]

The build item must be in an SPI and the extension must define the metric extention as *optional dependency *so when a user don't include it the dependencies are not added to the artifact.

Which dependency needs to be optional?

I don't think that in this case anything needs to be optional since the "client" extension by depending on the SPI only will not be bringing in any dependencies at all.
If the extension that handles the build items exist all is good, if it doesn't those build items are just dropped on the floor.

[loicmathieu]

@geoand as in the heath check in extension implementation, the SPI will not depends on metrics, but as the extension (jaeger for example) will provides metrics, it must have an optional dependency to the metrics extensions.

For ex:

• quarkus-jaeger-deployement => dependency to quarkus-metrics-spi
• quarkus-jaeger-runtime => optional dependency to quarkus-metrics (the runtime module)

So if someone uses jaeger but not metrics, no metrics dependencies will be loaded. But if someone use both jaeger and metrics the metrics dependencies will be loaded and the metrics will be registered. I remeber we discuss this design a lot during the extention health check implementation, maybe we should have written it somewhere :)

[geoand]

OK yeah, I see what you are saying. For that use case it makes sense!
However not all cases need a dependency on the runtime artifact.
In any case, thanks for writing it down.

[loicmathieu]

@jmartisk I comes up with a different design that yours, mainly inspired by what I did for extension health check, that allows to have an optional dependency to the metric extension so that if a user didn't include the extension in it's pom.xml it will not be included.

You can have a look at this branch, I implement a single metric for the agroal extension and if I only include agroal in my pom I didn't have any metrics published, if I add both agroal and metrics extension the agroal metrics are published.
https://github.com/quarkusio/quarkus/compare/master...loicmathieu:feat/extention-metrics

[jmartisk]

@loicmathieu I currently have this, now including the change with optional dependency:
https://github.com/jmartisk/quarkus/commits/master-extension-metrics-with-example

The main difference from your design I see is that in your case, the extension has to provide a class that contains annotated metrics, whereas in my design, it provides just the metric metadata object (except for gauges where you have to provide a Callable that implements the metric), therefore it is easier to dynamically create any number of metrics depending on what the application contains (for example, it is possible to scan the application classes for, say, REST endpoints, and register some metrics for each endpoint). I would guess this will be quite a typical use case, but I'd like to hear some feedback from extension maintainers, like @gunnarmorling if they think it's adequate

[loicmathieu]

@jmartisk my approach is easier to use (and for the maintainer that already provides health check it is similar to again ... easier) but provides less functionalities.

I agree that providing metrics dynamically is a valid use case and it is a lack in my implementation.

What I'm not OK with your implementation is that your SPI depends on microprofile-metrics-api, so this class will be embedded in all applications whereas it is not needed if the metrics extension is not included. So you will have one more un-needed dependency ...
So I prefere my implementation because it really provides a way to only include the metrics dependencies if the user add them to it's pom.xml.

[geoand]

@jmartisk my approach is easier to use (and for the maintainer that already provides health check it is similar to again ... easier) but provides less functionalities.

I agree that providing metrics dynamically is a valid use case and it is a lack in my implementation.

What I'm not OK with your implementation is that your SPI depends on microprofile-metrics-api, so this class will be embedded in all applications whereas it is not needed if the metrics extension is not included. So you will have one more un-needed dependency ...

I haven't really followed the discussion so what I am about to say might not make sense:

Why would having an spi depend on another spi be a problem? They are just build time dependencies to glue various Quarkus, are they not?

[jmartisk]

What I'm not OK with your implementation is that your SPI depends on microprofile-metrics-api, so this class will be embedded in all applications whereas it is not needed if the metrics extension is not included. So you will have one more un-needed dependency ...

It doesn't seem so, if the extension does not register any metric than no classes from microprofile-metrics-api are included in the resulting application, because everything from the API is dead code. I checked the resulting runner JAR and there is nothing related to metrics in it.
We could certainly get rid of this dependency too, but the usage would get quite ugly because the extension would not be able to use Metadata and Tag classes, so all the required metadata would have to be passed using some custom wrappers that we create for it. Which would be possible to do, but I'm not sure if we really need that.

[loicmathieu]

@jmartisk @geoand SPI can have dependencies, they will not be available at runtime. My bad :)

@jmartisk when you talk about dead code you talk about native image right ? Keep in mind that Quarkus is more used with Hotspot today than in SVM so we need to make sure that no extra dependencies are added when building a regular JAR application.

Still I don't like the complexity to use it for an extension author, what I really dislike is the Class.forName => https://github.com/jmartisk/quarkus/commit/8c6e892df58b6c50bc938a22e8a8f3f15e35e55f#diff-72da87fd60f85af7006064310bede085R44

Maybe a mixed aproach when some can provides metrics easily like in my implementation or use a more complex aproach when needed dynamic metrics creation is what wee need ...

[jmartisk]

Still I don't like the complexity to use it for an extension author, what I really dislike is the Class.forName

It's just an example, even though I agree it's not nice - surely the extension can have a config property to enable/disable metrics, like in your case.

Maybe a mixed aproach when some can provides metrics easily like in my implementation or use a more complex aproach when needed dynamic metrics creation is what wee need ...

Agreed, it is also useful for an extension to be able to introduce a CDI bean with metric annotations. But I think we don't quite need a special BuildItem for that, isn't that just a matter of adding such bean to the application through an AdditionalBeanBuildItem if the extension's configuration says that metrics should be enabled? This doesn't need to go through the Metrics' processor at all

when you talk about dead code you talk about native image right ? Keep in mind that Quarkus is more used with Hotspot today than in SVM so we need to make sure that no extra dependencies are added when building a regular JAR application.

I mean both the regular jar and the native image, it seems they don't contain anything from the Metrics API if the application's pom doesn't depend on quarkus-smallrye-metrics.

[loicmathieu]

Agreed, it is also useful for an extension to be able to introduce a CDI bean with metric annotations. But I think we don't quite need a special BuildItem for that, isn't that just a matter of adding such bean to the application through an AdditionalBeanBuildItem if the extension's configuration says that metrics should be enabled? This doesn't need to go through the Metrics' processor at all

This is the design of what we did for Health, the rational is to spare each extension writer an if block ... and maybe later implements a smarter way to disable metrics.
It also enables a global flag quarkus.metrics.extension.enabled to disable all extension metrics.
The idea was to be able, later to configure extension metrics gloabby like this:
quarkus.metrics.extension=datasource,fault-tolerence to enable it for datasource and fault-tolerence but not the other ones.

You can follow the discussion we had there: https://github.com/quarkusio/quarkus/issues/3107

I really think that extention metrics should be as close as extension health to facilitate it's implementation by all extension authors. Keep in mind that an extension author may not be someone from the Quarkus team (and platform is coming ... ) so may not have knowledge of how Quarkus works nor how metrics works. Developer joy should also be for extension developers :)

[jmartisk]

I just realized one more significant problem with the annotation based approach. This will create the metrics in the application scope, but we want metrics from extensions to be in the vendor scope, because they are not declared directly by the application. MP Metrics API doesn't allow turning annotations into metrics in vendor scope so we would have to extend it somehow, like introduce a new annotation that determines the scope.

Ability to disable extension metrics globally could indeed be useful, we would just have to make sure that it will really turn off all of them - all extensions would have to adhere to it and use the same mechanism. If one extension maintainer finds out that the mechanism is not flexible enough and will come up with their own mechanism to register metrics, the quarkus.metrics.extension.enabled flag might not work for it. And I'm a bit afraid whether for example Jaeger extension would be able to do that, because it uses a wholly different mechanism that is based on io.jaegertracing.spi.MetricsFactory that is embedded in its upstream dependency, so it might not be able to register metrics at build time using Quarkus-specific mechanisms (maybe it would be doable somehow, I didn't check it thoroughly).

[emmanuelbernard]

I have not looked at the code, just the conversations. Let me give you some context:

• most extensions will expose metrics
• but if SR metrics extension is not in the CP, then these metrics code must not be in the CP (in practice if there is no Metrics Build Item to listen to, the extensions will not publish their metrics.
• a global switch off flag must be available like we do for health
• we should keep the door open for per extension switch too like we have for health
• all extensions will standardize to use this unified mechanism

[emmanuelbernard]

Oh and it is best if an extension is not in control of the off switch, rather be mutualized by the metrics extension. Like we have tried in health.

[loicmathieu]

@jmartisk I noticed that using annotation currently implies having metrics in the application scope, I think that if it can be handled somhow it would be good.

I know that Jaeger is a different story, but let's try to have something that works for 80% of use cases :). Jaeger can still use the global switch directly from ConfigProvider.getConfig to handle it and provides it's own switch ...

I think we can mix our two approaches, and as @emmanuelbernard said it's important to have a global switch, a per-extension switch, and no runtime dependency if quarkus-smallrye-mertric is not in the application's pom.xml.

I think about it and what can be done to allow finer control to an extension author is to have a _MetricFactoryCustomizer_ interface that an extension author can extends that have a customizeMetricFactory(MetricFactory) method. Then the extension author can do what he want with the factory by implementing it. And as for providing metrics by annotation, instead of directly adding it to Arc via an AdditionalBuildItem, it must add it via a MetricCustomizerBuildItem that will reflect what I propose for the MetricBuildItem in order to hook into the global extension metric mechanism.

Then the metric extension should use all these MetricCustomizerBuildItem to gather the customizer bean and run them at runtime ...

This is close to what has been done to customizing JSON: https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/extension-authors-guide.adoc#291-customizing-json-b

[jmartisk]

The class with metric annotations will most likely contain not just metric annotations, but any useful business logic (for example, it will have a method that does something useful and is annotated with @Counted, so turning off metrics does not mean that you won't need to call that method at all). So we would need to add this bean as an additional bean even if metrics are disabled. But there is currently no way to mark one annotated class as "don't scan this one for metric annotations" if it's included in the application. What we would need to do in this case would probably be "remove" the metric annotations from the class during boot.
If we instead invented some way to mark the class as "don't scan for metrics" we would lose the possibility to remove MP Metrics API from runtime dependencies, because some annotations would still be present in the runtime classes.
If we really wanted to exclude the metric beans when metrics are disabled, we would have to restrict what the beans can contain (only things that can be completely removed without breaking the application), and that would kill the "simplicity" goal IMO.

Either way I think the annotation approach will really be useful for like 20 % of use cases, programmatic approach will be the one used more often. I'm not saying we shouldn't include the option for annotated classes, but I think it's not what will mostly be used.

So we could probably have both:

• MetricBuildItem that is currently in my design. Plus the option to turn off all metrics from extensions globally (the Metrics processor will just ignore it in that case). An extension will, as it scans the index for relevant components (REST endpoints, JPA entities,...), produce these build items for each component that it encounters. If the extension has its metrics turned off by a property for that specific extension, it will simply not produce any such build items.
• AnnotatedMetricBeanBuildItem or something (we need a good name to make the difference clear) that's in Loïc's design, and it will contain the class name of a bean that should be added to the application, but we'll have to somehow resolve the issues I mentioned in the first paragraph, because I think it will typically contain more than just the metric declarations, so it will not be possible to just exclude that bean completely

[emmanuelbernard]

@jmartisk I inferred a bit probably but here we are only talking about metrics that would be built-in and provided by extensions. Explicit user app metrics, we do them the normal MP Metrics way as indeed business logic is mingled.

So DB connection acquisition per second, cache miss / hit by Hibernate. For these, we want the user to be able to say, I don't want the extension metrics with a flag. And for them not to be activated if the metrics extension is not in the classpath

[emmanuelbernard]

Meaning I inferred things when I created the issue. Sorry

[jmartisk]

@emmanuelbernard Well yes, I am talking about metrics from Quarkus extensions only. Not sure what exactly made you think I'm dealing with application metrics here. For those, nothing will change, of course.

[jmartisk]

If you got confused by "option to turn off all metrics globally" then yeah that might not have been clear, but I meant the ones from extensions here. Sorry :)

[emmanuelbernard]

you said "business logic"

[jmartisk]

Ok, that might not have been the best term either, but I meant some code that is actually doing something, meaning that we cannot discard it without breaking the application.

[emmanuelbernard]

OK it's clearer to me now why you feel the programmatic approach will dominate for extensions. I do agree. And for programmatic, disabling them is easier of course as we simply don't enlist them.

[jmartisk]

Ok I'll update and polish my code and try to send an initial PR to potentially get more feedback. For now just with programmatic metrics, we can add annotated metrics later if we resolve the open questions that there are.

[gunnarmorling]

Hey @lhauspie, this is the issue we were talking about as a foundation for auto-exposing the metrics in the Kafka Streams extension.

I haven't followed the discussion closely, but I think (if you're still interested) you could essentially take the work that's done here and build the Kafka Streams metrics based on that. Also might be a good vehicle to provide feedback to @jmartisk to see whether the "framework" classes he's creating satisfy all the needs we'd have for Kafka Streams.

[jmartisk]

Btw the latest WIP version is at https://github.com/jmartisk/quarkus/commits/master-extension-metrics , I've just updated it. Also adding an implementation for the Agroal extension, that will serve as a better example than the MP Health example I showed earlier..

[jmartisk]

This has been fixed for some time by https://github.com/quarkusio/quarkus/pull/5272/, closing