MeterFilter not invoked after "Upgrade micrometer to 1.13. Use prometheus simple client"

[ppalaga]

Describe the bug

quarkus-cxf-integration-test-hc5 in Quarkus CXF is failing after bfaee7b

Expected behavior

The test should pass

Actual behavior

The test fails

How to Reproduce?

# checkout the problematic Quarkus commit
cd Quarkus
git checkout bfaee7bf9cdc3993885f11d632fce36299a6dbfe
# Build Quarkus
mvnd clean install -Dquickly

# Checkout Quarkus CXF
cd ~/projects
git clone [email protected]:quarkiverse/quarkus-cxf.git
# set quarkus.version
sed -i 's|<quarkus.version>[^<]*</quarkus.version>|<quarkus.version>999-SNAPSHOT</quarkus.version>|' pom.xml
# build the whole tree
mvnd clean install -DskipTests -Dquarkus.build.skip
# run the failing test
cd integration-tests/hc5
mvnd clean test
...
[INFO] Running io.quarkiverse.cxf.hc5.it.Hc5Test
[INFO] [stdout] 2024-08-27 15:07:50,615 WARN  [org.apa.cxf.fro.AbstractWSDLBasedEndpointFactory] (executor-thread-1) Could not find endpoint/port for {http://www.jboss.org/eap/quickstarts/wscalculator/Calculator}CalculatorServicePort in wsdl. Using {http://www.jboss.org/eap/quickstarts/wscalculator/Calculator}Calculator.
[INFO] [stdout] 2024-08-27 15:07:50,621 WARN  [org.apa.cxf.fro.AbstractWSDLBasedEndpointFactory] (executor-thread-1) Could not find endpoint/port for {http://www.jboss.org/eap/quickstarts/wscalculator/Calculator}CalculatorServicePort in wsdl. Using {http://www.jboss.org/eap/quickstarts/wscalculator/Calculator}Calculator.
[ERROR] Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.052 s <<< FAILURE! -- in io.quarkiverse.cxf.hc5.it.Hc5Test
[ERROR] io.quarkiverse.cxf.hc5.it.Hc5Test.add(String)[4] -- Time elapsed: 0.064 s <<< FAILURE!
java.lang.AssertionError: 

Expecting actual not to be null
        at io.quarkiverse.cxf.hc5.it.Hc5Test.add(Hc5Test.java:55)

Observation 1

When bfaee7b is reverted in Quarkus, the test passes

Observation 2

With bfaee7b in place, the MeterFilter.map() method is called only once

https://github.com/quarkiverse/quarkus-cxf/blob/main/integration-tests/hc5/src/main/java/io/quarkiverse/cxf/hc5/it/MeterFilterProducer.java#L19-L29

It can be seen when a debug statement outputting requestScopedHeader.getHeaderValue() is placed in the method body.

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @brunobat (micrometer), @ebullient (micrometer)

[ppalaga]

cc @brunobat

[brunobat]

I cannot run the test after checking out main, @ppalaga.
I get:

[ERROR] Errors: 
[ERROR]   Hc5Test.wsdlUpToDate » Runtime java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
        [error]: Build step io.quarkus.arc.deployment.ArcProcessor#validate threw an exception: jakarta.enterprise.inject.spi.DeploymentException: jakarta.enterprise.inject.UnsatisfiedResolutionException: Unsatisfied dependency for type io.quarkiverse.cxf.metrics.CxfMetricsConfig and qualifiers [@Default]
        - injection target: io.quarkiverse.cxf.metrics.MetricsCustomizer#config
        - declared on CLASS bean [types=[io.quarkiverse.cxf.metrics.MetricsCustomizer, io.quarkiverse.cxf.CxfClientProducer$ClientFactoryCustomizer, io.quarkiverse.cxf.transport.CxfHandler$EndpointFactoryCustomizer, java.lang.Object], qualifiers=[@Default, @Any], target=io.quarkiverse.cxf.metrics.MetricsCustomizer]
...

[ppalaga]

Sorry about that. Maybe the build of Quarkus CXF did not succeed? It is quite possible that the newest mvnd won't work. You may want to use ./mvnw

[brunobat]

this works: ./mvnw -DskipTests -pl '!docs' clean install
I was able to reproduce the issue. Debugging now.

[brunobat]

The timer is getting cached and the MeterFilter is not executed on the 4th and last test.
If the 4rth test is executed alone, it will pass.
@ppalaga sent you an invite for a meeting.

[brunobat]

Created this bug on the Micrometer side: micrometer-metrics/micrometer#5406
We can wait a bit for feedback and decide if we can move ahead with the backport.
CC @ppalaga @ebullient

[brunobat]

@ppalaga l just wrote this on the Micrometer side bug: micrometer-metrics/micrometer#5406 (comment)

I'm not sure a MeterFilter should be used like that with dynamic content...
In the case of HTTP requests, the Quarkus recommended way to produce those tags is using the HttpServerMetricsTagsContributor: https://quarkus.io/guides/telemetry-micrometer#use-httpservermetricstagscontributor-for-server-http-requests

Will this be a problem for CXF?

[ppalaga]

In the case of HTTP requests, the Quarkus recommended way to produce those tags is using the HttpServerMetricsTagsContributor: https://quarkus.io/guides/telemetry-micrometer#use-httpservermetricstagscontributor-for-server-http-requests

I guess our test scenario could be rewritten in that way.
The code we now have in place comes from a bug report complaining about the context not being propagated in async CXF client calls. I guess the propagation alone could be proven with HttpServerMetricsTagsContributor too.

I am not a Micrometer expert at all and I have no idea how much this kind of dynamic MeterFilter use is widespread.

However, a purely formal standpoint should be that a minor upgrade of Micrometer should stay backwards compatible.

Maybe @AndreasPetersen who filed quarkiverse/quarkus-cxf#947 has an opinion?

[AndreasPetersen]

Hello, thank you for pinging me.

The HttpServerMetricsTagsContributor only sets tags on HTTP server requests, not HTTP client requests. As far as I'm aware, there is no Quarkus equivalent for HTTP client requests. Not that there is such a need, because using request specific content in MeterFilters works for HTTP client requests. See #33313 and #29739.

In the case of quarkiverse/quarkus-cxf#947 the issue was on the HTTP client requests. We need some method of adding request specific content to the generated Micrometer Meter. If we can no longer use MeterFilter for that purpose, we would need some new mechanism of doing so.

I hope this provides some useful information. :)

[brunobat]

I'm not sure I understand, @AndreasPetersen because this MeterFilterProducer from the test it's being applied to all metrics in the server... It's a global filter.
This is the filter causing the test to fail.

[AndreasPetersen]

Ah, I haven't really looked into the failing test reported in this issue.

In our use case, we only add tags to client filters, by checking the name of the meter in the MeterFilter. We don't actually even use SOAP server requests. But for those that do, if the HttpServerMetricsTagsContributor works for SOAP server requests as well, then I see no issue.

[brunobat]

Can you guys refactor that test and not use the MeterFilterProducer or any dynamic MeterFilter?

[ppalaga]

@brunobat where can I find more info about static/dynamic MeterFilters on Quarkus? I have no idea what the difference is.

[brunobat]

This is kind of a definition that I invented. Apparently, It is not supposed to use MeterFilters in a dynamic way, meaning that the return content will change with time...
They are supposed to return static content. This is not writen in the Micrometer docs, it's my interpretation of their code and one of their developers has confirmed it on a chat.

[ppalaga]

one of their developers has confirmed it on a chat

Could you perhaps push them to make it a part of their official docs? Once there, it would be much easier to justify to our users that they should prefer HttpServerMetricsTagsContributor for dynamic scenarios.

[ebullient]

And we likely may need *ClientMetricsTagsContributor for the outbound flow as well.

[brunobat]

@ppalaga I did reach out to them and you can quote the issue I created there: micrometer-metrics/micrometer#5406
Also I included a note in the migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.16#micrometer

[ppalaga]

OK, I implemented a new CXF-like way to attach Tags to client and server requests: quarkiverse/quarkus-cxf#1497

I considered using ClientMetricsTagsContributor/HttpServerMetricsTagsContributor, but then realized that the QCXF metrics extension does not depend on quarkus-micrometer. There was no need to do so in the past and perhaps it also made it possible make our metrics extension supported before quarkus-micrometer was supported.

Anyway, once we have both ClientMetricsTagsContributor and HttpServerMetricsTagsContributor I may implement using them as well.

[ppalaga]

I implemented a new CXF-like way to attach Tags to client and server requests: quarkiverse/quarkus-cxf#1497

Having said that, I am hereby acking bacporting Micrometer 1.13 to Quarkus 3.15

[brunobat]

@ppalaga are the tests now passing after your fix?
Can you please create an issue on Quarkus to create a ClientMetricsTagsContributor?
Can we close this issue after?

[ppalaga]

@ppalaga are the tests now passing after your fix?

Yes

Can you please create an issue on Quarkus to create a ClientMetricsTagsContributor?

#43178

Can we close this issue after?

+1