Update to SmallRye Reactive Messaging 1.1.0

[cescoffier]

This version brings a new set of features:

• Metadata propagation
• New Emitter support (part of the spec, the old one is deprecated but still supported)
• @Stream has been removed (already deprecated)

[cescoffier]

@kenfinnigan @geoand @mkouba There is something where I'm not quite sure: the metric support.
SR Reactive Messaging defines a metric (defined in the spec) : https://github.com/smallrye/smallrye-reactive-messaging/blob/master/smallrye-reactive-messaging-provider/src/main/java/io/smallrye/reactive/messaging/metrics/MetricDecorator.java (1 metric per channel).

In the PR, I just add this bean if the metric capability is enabled. Is there anything else required?

[geoand]

I don't really know about metrics, so either the aforementioned people can help out or @jmartisk can chime in

[geoand]

I like it, 👍

[jmartisk]

IIUC the MetricDecorator puts the metrics into the application metric registry, I think vendor registry would be more correct for this, as these are not declared directly by an application. That can be achieved by injecting it together with @org.eclipse.microprofile.metrics.annotation.RegistryType(type=VENDOR)

EDIT: or rather base! The RM spec does not say explicitly (maybe it should), but Metrics spec says that generally metrics defined by other MP specs should go into base

Also we usually introduce a config property like quarkus.EXTENSION.metrics.enabled to enable metrics for a particular extension and the default value is false, see https://github.com/quarkusio/quarkus/blob/master/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/AgroalBuildTimeConfig.java#L40 so that metrics for each extension have to be enabled separately

[jmartisk]

Also if the set of channels (and therefore metrics) is known already at build time, the processor could optionally emit MetricBuildItem-s to get the metrics registered eagerly during build. But this currently can be done only for vendor registry, so if these should go into base then we will need to improve the metric registration mechanism to allow that too :)

[cescoffier]

@jmartisk Thanks!

So, unfortunately right now the set of channels is not known at build time. But I really want to go in this direction (it requires a bit more code and configuration analysis, nothing very hard).

I will add the flag to enable and disable the metric, that makes sense! So, we expose them only if 1. the metric capability is enabled 2. the extension metrics are enabled. Am I right?

[jmartisk]

So, we expose them only if 1. the metric capability is enabled 2. the extension metrics are enabled. Am I right?

Yes.

[cescoffier]

@jmartisk About vendor, do you think the spec should mention it (See https://github.com/eclipse/microprofile-reactive-messaging/blob/22b40135a6dbffca17599ac79b89bd19174f2f98/spec/src/main/asciidoc/architecture.asciidoc#metrics).

Right now the TCK uses the "default" registry:
https://github.com/eclipse/microprofile-reactive-messaging/blob/22b40135a6dbffca17599ac79b89bd19174f2f98/tck/src/main/java/org/eclipse/microprofile/reactive/messaging/tck/metrics/MetricsTest.java#L69

[jmartisk]

Hm, if the TCKs already expect them in default (which is application), then I suppose we'll have to leave them there.. This has unfortunately only very recently been clarified in the MP Metrics spec (https://github.com/eclipse/microprofile-metrics/blob/master/spec/src/main/asciidoc/architecture.adoc#required-base-metrics) - this will be in the upcoming 2.3 version:

The base scope is used for, and only for, any metrics that are defined in MicroProfile specifications. Metrics in the base scope are intended to be portable between different MicroProfile-compatible runtimes.

So I would suggest the next version of RM spec should move them to base

[cescoffier]

@jmartisk Actually, the spec has not been released with these changes yet... So, there is still time to fix it :-). Let me create an issue in the spec (will ping you from there).

[jmartisk]

@cescoffier Oh, that's good news then :) sure.

[geoand]

The TCK failures seem related

[kenfinnigan]

For the TCKs, I think https://github.com/quarkusio/quarkus/blob/master/tcks/microprofile-reactive-messaging/pom.xml#L36 needs to add the Metrics extension, as by default it's not brought in through a required dependency

[cescoffier]

The TCK failure is expected, the failing tests have been removed from the TCK, it was not correct.

[cescoffier]

I've applied a workaround for the TCK.

[cescoffier]

About the metrics change, let's wait until it's part of the upstream specification. It is going to happen soon I believe.

[geoand]

LGTM but let's get an approval from @kenfinnigan and @jmartisk as well :)