Do not re-apply MeterFilters to IDs when registering if unnecessary

[lenin-jaganathan]

Please describe the feature request.
To interact with a meter (recording values), the preferred pattern documented is to use the Builder to create a meter and interact with it. For example, the following code would create a counter,

Counter counter = Counter
    .builder("counter")
    .baseUnit("beans") // optional
    .description("a description of what this counter does") // optional
    .tags("region", "test") // optional
    .register(registry);

Once the counter is created we would call the counter.increment() to increment the counter. For a simple counter like this, we can save it in a variable and call the increment method whenever we want to increment. Now, if we want to add a tag whose value will be available only during runtime, we cannot store it in a simple variable. There are two ways to do it,

1. Directly call the increment method on the object returned by the builder. Since, the registry will make sure that only one meter will be registered for a name and tag set we would get the same object all the time and can call increment on it. Again, so simple it seems but in a real-world scenario, applications would be having meterfilters registered which will have to be run every single time we invoke the builder. Since, Meter.Id is immutable we keep on creating new meters if we want to add tags/modify tags/modify the name, etc. But all the code runs and would eventually return an already created meter in the registry. This becomes a huge overhead in the real-world use case where we interact with meters thousands of times per second and generate a very high amount of garbage data.
2. To work around the problem in 1, we tried a few different things and eventually used a HashMap to store the different tags as the key and the returned meter as value. So, the next time you need the meter for a tag and name combination we would avoid all the intermediate steps and directly return the actual meter. This surely had a huge performance gain for us which is available in the above-mentioned repo. It opened up a few issues like,
   i. What if a meter is removed from the registry? Then our cache will be holding reference to a meter that is no longer reported and would be incrementing that. We then listened to the onMeterRemoved and removed the meters from the cache.
   ii. What if someone added a meterfilter? That would basically invalidate all the meters. Micrometer doesn't enforce strict rules about when a filter can be added. But we sort approached meter registry state as a immutable thing and didn't want to invalidate the entire cache because that micrometer doesn't invalidate meters when meter filters are registered.

To address the above mentioned problem and to improve the overall performance of micrometer meters, a built-in way to map the preMapped Id's to mappedId's with O(1) retrieval time with minimal garbage generation could be introduced.

Reference:

• MicroBenchmarking with different cache options - /~https://github.com/lenin-jaganathan/micrometer-benchmark/

Rationale
To make micrometer meter interaction faster and lightweight.

Additional Context
Some extent of this is also discussed in #3461

[lenin-jaganathan]

If we agree that #3461 should be solved by making meter filters immutable this could be very simple.

I already have an idea to tackle this issue and would open up a PR for that. The idea is to simplify the meterMap from filtered ID to meter map to a prefFilteredId to meter map. But solving #3461 would simplify this. I am open to suggestions and discussions on this.

[jonatan-ivanov]

Now, if we want to add a tag whose value will be available only during runtime, we cannot store it in a simple variable.

Have you seen this (it eliminates the dynamic Builder creation)?

public class MeterDemo {
    private static final SimpleMeterRegistry registry = new SimpleMeterRegistry();
    private static final MeterProvider<Counter> counter = Counter.builder("test")
        .tags("static", "1")
        .withRegistry(registry);

    public static void main(String[] args) {
        for (int i = 0; i < 10; i++) {
            counter.withTag("dynamic", String.valueOf(i)).increment();
        }
        System.out.println(registry.getMetersAsString());
    }
}

Since, Meter.Id is immutable we keep on creating new meters if we want to add tags/modify tags/modify the name, etc.

I think you will have a new Meter.Id, not Meter.

But all the code runs and would eventually return an already created meter in the registry.

Yes since the way you have the key for the lookup (Meter.Id) is 1. have a base Id, 2. modify it and you do the lookup with this modified Id.

This becomes a huge overhead in the real-world use case where we interact with meters thousands of times per second and generate a very high amount of garbage data.

I'm looking at the benchmarks (the first image should be avg_time.png I think). Thank you for creating them. I think both time and allocation rate could be improved with the withRegistry feature I mentioned above (should save you from the extra Builder instance).

What if a meter is removed from the registry?

I would recommend what you wrote: create a listener to listen to event when meters are removed.

What if someone added a meterfilter?

Yepp, that's the issue, this is also why Micrometer does things in the way you explained above.

To address the above mentioned problem and to improve the overall performance of micrometer meters, a built-in way to map the preMapped Id's to mappedId's with O(1) retrieval time with minimal garbage generation could be introduced.

Could you please elaborate on this? You want Micrometer to store the Id before the filter and after too and do not run the filter when you found that mapping exists?
I think this could help but I'm not sure how much (we are still well in the bytes and ns range). Also, we really need to think through the consequences:

1. If there is a MeterFilter that is somehow dynamic, i.e.: it can give you different outputs to the same input if you invoke it multiple times, this mapping will not make that behavior possible (not sure if this is a valid use-case).
2. MeterFilter has more responsibilities :( it is not just maps the Ids but it can also configure (modify) DistributionStatisticConfig.

[lenin-jaganathan]

Have you seen this (it eliminates the dynamic Builder creation)?

Yeah, I have seen it. This problem particularly deals with unneccessary overheads caused by running meterfilters everysingle time.

Could you please elaborate on this? You want Micrometer to store the Id before the filter and after too and do not run the filter when you found that mapping exists?

Yeah, exactly.

If there is a MeterFilter that is somehow dynamic, i.e.: it can give you different outputs to the same input if you invoke it multiple times, this mapping will not make that behavior possible (not sure if this is a valid use-case).

I don't think why someone would invoke same meter filter twice. But, even with that the mapping should be good since we are mapping from pre filtered id to final meter.

MeterFilter has more responsibilities :( it is not just maps the Ids but it can also configure (modify) DistributionStatisticConfig.

Yeah that's right. But, DistributionStatisticConfig is applied only when a meter is created for the first time. And this change wouldn't affect that. I have a PR with the initial thoughts here. Please have a look and we can discuss on that.

(we are still well in the bytes and ns range)

This is probably because I chose the simplest of the examples out there to demonstrate the changes. Usually, there will be multiple meter filters updating the Id's on the fly and that gets very complicated, and the memory could reach up to a few Kb's if we do just 2-3 mutations on the meter.

[lenin-jaganathan]

On another note, regarding the MeterProvider if the docs didn't cover it we should try to promote this in the docs.

But in my initial benchmarks, I don't really see any sort of improvement using meter provider for this use-case. Never know I could have messed up and didn't measure the right thing.

[jonatan-ivanov]

I don't think why someone would invoke same meter filter twice. But, even with that the mapping should be good since we are mapping from pre filtered id to final meter.

Right now you can write a filter that behaves like this: before noon it adds a tag color=red and after noon it adds color=blue. Again, I'm not sure if this is a real use-case (but a possibility) and you don't have this possibility anyways if you assign the meter to a variable so probably not an issue.

[shakuzen]

But in my initial benchmarks, I don't really see any sort of improvement using meter provider for this use-case. Never know I could have messed up and didn't measure the right thing.

Are those benchmarks somewhere we could look at and run?

1. If there is a MeterFilter that is somehow dynamic

We did have a user recently report having such a NamingConvention in #4608 (comment), and that was surprising to me. I don't think MeterFilters that map IDs should produce different results for the same input, but we haven't documented or enforced that (through things like the TCK) well up to this point.

[lenin-jaganathan]

Are those benchmarks somewhere we could look at and run?

I have something you can try at /~https://github.com/lenin-jaganathan/micrometer-benchmark/blob/main/src/main/java/org/example/Comparison.java

[tsegismont]

Right now you can write a filter that behaves like this: before noon it adds a tag color=red and after noon it adds color=blue.

I hadn't thought about this possibility. From my perspective, it's a bad idea to use a filter for this. Using a MeterProvider sounds more appropriate.

[tsegismont]

Have you seen this (it eliminates the dynamic Builder creation)?

@jonatan-ivanov I wasn't aware of the MeterProvider API. Thanks for pointing it out, I will give it a try.

I noticed it was available for all meters except gauges, why?

[jonatan-ivanov]

Mostly because gauges are async so you don't register them the same way you do other with Meters. How would you use the MeterProvider for gauges? I think that use-case should be covered by the MultiGauge.

[tsegismont]

Mostly because gauges are async so you don't register them the same way you do other with Meters.

I'm not sure what you mean with this, can you elaborate?

How would you use the MeterProvider for gauges? I think that use-case should be covered by the MultiGauge.

Let's take an example. In Vert.x we want to gauge the number of connections established on a TCP server. We have a Gauge named vertx_net_server_active_connections. The possible tag keys are local and remote, and the values are the socket address. These tags could me modified by any filter created by the user.

I want a LongAdder to be bound to the gauge, so that I can increment/decrement when a connection is opened/closed.

I cannot use a LongAdder bound to the server itself, or maintain a map per local address and/or remote address, because I still don't know how the user could transform the tags in a filter.

As a workaround, we use a wrapped builder that bounds a LongAdder to a registered Gauge.

I don't think a MultiGauge is a solution to this problem, is it?

Ideally, I'd like to be able to:

• retrieve an arbitrary object bound to a registered Gauge
• use MeterProvider<Gauge> to select the right gauge for a given set of tags only known when an event occurs.

[tsegismont]

Have you seen this (it eliminates the dynamic Builder creation)?

@jonatan-ivanov I wasn't aware of the MeterProvider API. Thanks for pointing it out, I will give it a try.

I gave MeterProvider a try, with the Micrometer version we're currently using (1.12).
tsegismont/vertx-micrometer-metrics@05809d8

I did some performance testing in our lab, using the plaintext and fortunes Techempower benchmarks.

• Yellow bars: without monitoring
• Blue bars: monitoring with Vert.x Micrometer metrics (main branch)
• Red bars: monitoring with Vert.x Micrometer metrics, including the change above (custom meter cache removal and meter provider)

In the plaintext benchmark, the change produced slightly less good results, but it's not bad at all considering there isn't a cache anymore.

In the fortunes benchmark, which is more realistic (it involves a db query and html templating), the results are equivalent.

So I think in Vert.x Micrometer Metrics we could consider dropping the custom cache at least.

I'm going to give the benchmarks another try using the changes in #4857

[tsegismont]

@shakuzen I gave 1.13.0-RC1 a try and here are the results

Plaintext benchmark:

Fortunes benchmark:

So 1.13.0-RC1 (with internal cache) provides slightly less performance than 1.12.4 (without cache).

[shakuzen]

@tsegismont thank you for trying things out and providing the benchmark results. I'll keep looking into things as much as I can to see if we can't close the gap more. Let me know if you find anything in particular we can still improve.

So 1.13.0-RC1 (with internal cache) provides slightly less performance than 1.12.4 (without cache).

Just to be clear, this means Micrometer 1.12.4 and Vert.x's meter cache?

[tsegismont]

Just to be clear, this means Micrometer 1.12.4 and Vert.x's meter cache?

This was a comparison between:

• Vert.x and Micrometer 1.13.0-RC1 (internal cache)
• Vert.x and Micrometer 1.12.4 without external cache