Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global MeterFilters with dynamic content are bypassed by cache #5406

Open
brunobat opened this issue Aug 29, 2024 · 4 comments
Open

Global MeterFilters with dynamic content are bypassed by cache #5406

brunobat opened this issue Aug 29, 2024 · 4 comments

Comments

@brunobat
Copy link

brunobat commented Aug 29, 2024

Describe the bug
We have a test that executes requests to the same endpoint but with different parameters and headers.
The parametric test runs 4 times and in the last execution it fails because the right meter, with the right tag is not present.

When executed alone, that test passes because there is no Micrometer caching involved.

The previous test will add a very similar meter that will be resolved in the cache here:

The preFilterIdToMeterMap in

Meter m = preFilterIdToMeterMap.get(originalId);

This gets returned and the global meter filters that would otherwise be applied in

... are never executed.

If one of those filters is a bean returning dynamic content, in this case, based on specifics of the request: https://github.com/quarkiverse/quarkus-cxf/blob/6cea9f2acb0127f3b4d17273578a3a4fc353ee27/integration-tests/hc5/src/main/java/io/quarkiverse/cxf/hc5/it/MeterFilterProducer.java#L12
Those tags are never applied and the wrong meeter is effectively returned.

See example:
Screenshot 2024-08-29 at 09 16 57

Environment

x@x quarkus-cxf % mvn --version
Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)
Maven home: x/.sdkman/candidates/maven/current
Java version: 22.0.2, vendor: Eclipse Adoptium, runtime: x/.sdkman/candidates/java/22.0.2-tem
Default locale: en_PT, platform encoding: UTF-8
OS name: "mac os x", version: "13.6.9", arch: "aarch64", family: "mac"

Micromenter 1.13.3 on Quarkus main branch (unreleased).
Using the Prometheus simple client.

To Reproduce
The bug can be reproduced following the steps of this bug report: quarkusio/quarkus#42800

Expected behavior
If dynamic content can be added by global MeterFilters caching must not bypass them.

@brunobat
Copy link
Author

Having a look at the code it seems that it's assumed that global MeterFilters only provide static data, however, that is not mentioned in the documentation: https://docs.micrometer.io/micrometer/reference/concepts/meter-filters.html
I wonder if this is a bad use of the API that seems ok if you read the docs.

Also, Wouldn't it be great to have static and dynamic meeter filters in order to better cache things?

@brunobat
Copy link
Author

CC @jonatan-ivanov

@ppalaga
Copy link

ppalaga commented Sep 9, 2024

@jonatan-ivanov it would be great if the non-volatility and cacheability requirements on MeterFilter methods could become a part of your official documentation. Otherwise it is very hard for us to explain to our users why this kind of use case stopped working after the upgrade to Micrometer 1.13

@johnnywiller
Copy link

We are running into the same issue, having HTTP headers (tenantIDs and other low cardinality IDs) that should be applied to all metrics as tags, but doing so in a decoupled way from where the metric is created.
Since what we provide is a spring boot starter for users just plug and have that behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants