Add Meter.Cache for dynamic meter registration

[qweek]

Summary
This PR aims to add Meter.Cache for dynamic meter registration. This allows to create meters with dynamic tags, making it more customizable and adaptable to systems with tags depending on external sources. This change is backward-compatible.

Changes
Introduce additional register() method in specific Meter to create Meter.Provider for ad-hoc Meter creation.

Why is this important?
Currently, Micrometer's Meter doesn't provide a straightforward way to customize tags without additional allocations in runtime. This PR eliminates the need to build, deduplicate and sort Tags on each call and allow to re-use metrics builder, thereby simplifying metric management.

Backwards Compatibility
This feature is backward-compatible because it doesn't affect existing register() method.

Test Coverage
MeterCacheTest class has been created to include tests that verify this new functionality, ensuring that all basic metrics supports dynamic registration.

Usage Example
After this change, you can use new register() method to create metric with dynamic tags.

Meter.Provider<String, Counter> provider = Counter.builder("counter")
            .register(registry, value -> Tags.of("source", value));

void request(String source) {
    provider.get(source).increment();
}

[pivotal-cla]

@qweek Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@qweek Thank you for signing the Contributor License Agreement!

[qweek]

Related issues that may have benefit from this change:
#4052
#2285

[jonatan-ivanov]

Thank you for the PR! We are planning to do something similar (please see below), it seems this PR shows a different way to implement that.
My two cents:

This allows to create meters with dynamic tags

Just to clarify things: Micrometer is capable of doing this, the register method is basically a createOrGet backed by a Map so if you call register with different (dynamic) tags, it will create new meters, if you call it with the same tags, it will return the meter that was registered previously. (It seems you are aware of this, I just wanted to call this out since other users might not be.)

This PR eliminates the need to build, deduplicate and sort Tags on each call and allow to re-use metrics builder, thereby simplifying metric management.

I'm not sure the PR does this or it works as intended. Builders are not meant to be shared (they are mutable) and as far as I can tell (maybe I misunderstood something), every time you ask for a new (tag) value, the builder will be modified (tags.and modifies the builder) so I guess we have a shared mutability (concurrency) issue through it. Or am I misunderstanding something?

I think this is somewhat similar to this approach that we are planning to implement in 1.12 (in the next 2 weeks 🤞🏼): #535 (comment)
The approach in the comment is not caching anything (it's basically a syntax-sugar/shortcut) but it feels a bit more explicit/straightforward with tags.

timerProvider.get("first.value").record(latency); // in this PR

vs.

timerProvider.apply(Tags.of("name", name)).record(latency); // in the linked comment

I'm not sure the cache is needed or it's a real benefit in general. Also, items are never removed from the cache, e.g.: meters can be removed from the registry so this is a memory leak by definition. Sometimes meters have to be removed and re-registered, see the Kafka instrumentation. 🙁
In the long run, the cache might have some memory impact. Not just because of the leak I mentioned above but because it basically doubles the references to every Meter that is created through it (the registry also stores them). Also, since the key can be anything, it might not be apparent to the users that it must have a fair haschCode and equals implementation. I'm also thinking if having a lot of short-lived builder instances is better or not from the memory perspective since they will be eliminated by the GC while the cache won't be.

[qweek]

If you call it with the same tags, it will return the meter that was registered previously.

Yep, as you mentioned, I'm aware of this and registerMeterIfNecessary() method still do unnecessary work before call to getOrCreateMeter(), because it applies all filters in getMappedId().

builder will be modified (tags.and modifies the builder)

No, I intentionally use immutable operations.
Compare existing code
register(registry, tags.and(provider.apply(key)))
and mutable "classic" version

tags(provider.apply(key));
register(registry, tags);

In second case, as side effect you will always override internal builder tags.
Expected behaviour (added similar test case)

var provider = Counter.builder("my.counter")
            .tag("default.tag", "default.value")
            .register(registry, value -> Tags.of("key", value));
provider.get("first.value"); // Tags: ("default.tag", "default.value") and ("key", "first.value")
provider.get("second.value"); // Tags: ("default.tag", "default.value") and ("key", "second.value")

#535 (comment)

Yes, it similar approach, but it will have same problems as described before: unnecessary work for process Tags and build Meter.
Anyway, it will help to solve most of our problem (our existing solution is much worse, because we need to use reflection due to discussed immutability problems with tags). But we will still have problems with dynamic gauges, because we need to create both new object (T obj) and new tags for each new gauge, which is impossible in this case.

According to memory leaks, I agree that it's possible, but can be described in documentation. It doesn't affect existing code and we still have possibility to create meters with usual approach. In some cases we already have methods which create meters, but didn't have way to close it without specific logic.

// internal gauge is not returned to user
T MeterRegistry.gauge(String name, Iterable<Tag> tags, @Nullable T stateObject, ToDoubleFunction<T> valueFunction)

having a lot of short-lived builder instances is better

Unfortunately no, it was one of the main reasons why we implement this change, because existing approaches highly affect Young GC, lead to minor pauses and also use a lot of CPU.

[jonatan-ivanov]

registerMeterIfNecessary() method still do unnecessary work before call to getOrCreateMeter(), because it applies all filters in getMappedId().

I think that is necessary in some use cases. You might not use MeterFilter but other users do.

No, I intentionally use immutable operations. Compare existing code register(registry,tags.and(provider.apply(key))) and mutable "classic" version

Indeed; sorry, somehow I misread/interpret it. That extra private register method that basically reuses all the fields of the Builder except the tags so that tags are injectable seems like a great idea. Btw this can also help to greatly simplify the approach in #535 (comment) since that always makes a new copy of the builder but with the extra register I think copying the Builder is not needed anymore since register takes care of "copying" (reusing the builder fields).

Yes, it similar approach, but it will have same problems as described before: unnecessary work for process Tags and build Meter.

With your idea using the extra register method, I think it is simple to convert the original approach (jonatan-ivanov@f47c83b) into something that does not need to copy the builder (jonatan-ivanov@228b2b0). Please notice that you might have a slightly different goal than #535. That one is about API usability while I think you also want to spare some work.

According to memory leaks, I agree that it's possible, but can be described in documentation.

I don't think this would be fortunate, users are using Micrometer in various ways and I don't think providing them a seemingly harmless method that just creates meters but can leave garbage behind is a good idea.

In some cases we already have methods which create meters, but didn't have way to close it without specific logic.

// internal gauge is not returned to user
T MeterRegistry.gauge(String name, Iterable<Tag> tags, @Nullable T stateObject, ToDoubleFunction<T> valueFunction)

This is by design but you can still get a reference to this Gauge if you want to remove it: registry.find(...).gauge().

Unfortunately no, it was one of the main reasons why we implement this change, because existing approaches highly affect Young GC, lead to minor pauses and also use a lot of CPU.

From the young-gen perspective, I think you are right, but I was talking about the overall impact: "if having a lot of short-lived builder instances is better or not from the memory perspective". The trade-off is basically more garbage in young-gen (your pain point) vs. increased heap usage on the long run.

Could you please check the changes I made on the proposed solution for #535 that now does not need a builder thanks to your idea jonatan-ivanov@228b2b0 and tell me how much would it help in your use-case?

I think I would still prefer being able to pass all the extra tags instead of just one value:

timerProvider.get("first.value").record(latency);
//vs.
timerProvider.apply(Tags.of("name", name)).record(latency);

I think that approach is more explicit (readable), looking at only timerProvider.get("first.value"), it's not apparent what should I expect.

[qweek]

You might not use MeterFilter but other users do.

I didn't mean that filters itself are unnecessary (we actually use them), but when you convert MeterId -> MappedMeterId it is usually expected that after applying all the filters you will get the same Id.

I think copying the Builder is not needed anymore since register takes care of "copying" (reusing the builder fields).

I happy that you like idea with register(registry, tags) method. The only addition to (jonatan-ivanov@228b2b0), is that I would still prefer an interface like Meter.Provider { M withTags(Tags extraTags); } instead of a generic function.

On the contrary, it seems to me that the idea of copying a builder can be very useful when implementing a factory :)
I'd rather have something like this:

1. Add private copy constructor and public Builder copy() method
2. Add method that convert builder to factory, like
   <F extends Factory> toFactory(MeterRegistry registry, BiFunction<Builder, MaterRegistry, F> constructor)
3. Factory interface should have only close() method, then users will be able to add any methods and implementations, like

Counter withTags(Tags extraTags) {
    return builder.copy().tags(extraTags).register(registry);
}

As you said, Gauge is different case, to make it work same way as other meters we need:

1. Additional methods on Builder to override stateObject(T stateObject) and valueFunction(...)
2. Have access to stateObject from Gauge class
3. Then we can implement Factory methods to add dynamic tags for cases like

GaugeFactory
    .withObject("objectName", objectA) // multiple objects as tags <- 1 property

GaugeFactory
    .withProperty("propertyName", State::getPropertyX) // 1 object -> multiple properties as tags

Tell me what you think about this, if you are interested in such an implementation, I can make a proof of concept.

[jonatan-ivanov]

when you convert MeterId -> MappedMeterId it is usually expected that after applying all the filters you will get the same Id

I'm not sure I follow/understand, you need to look up the Meters anyway, right? Should we create a new issue for this?

I would still prefer an interface like Meter.Provider { M withTags(Tags extraTags); } instead of a generic function.

We usually don't do that but I can see some advantages of it, I added a comment to the PR: #4097 (review)

On the contrary, it seems to me that the idea of copying a builder can be very useful when implementing a factory :)

Yeah, that's what I did originally (copy ctor) but with your idea, the extra Builder object can be eliminated. Also, I'm not sure it is needed to create meters with different builder settings (other than tags), what would be the use case for that? I think the other settings of the builder do not create a new meter when you change them so do not seem that useful to me.

As you said, Gauge is different case, to make it work same way as other meters we need:

I feel doing something like this to Gauge or other "async" Meters FunctionXyz is a slippery slope and can be misused easily. For dynamic tagging gauges, you might want to look at MultiGauge where you can register gauges dynamically by providing the dynamic tags: https://micrometer.io/docs/concepts#_multi_gauge

[lenin-jaganathan]

@jonatan-ivanov We have also faced the issue of using the default builder pattern to record values in a Meter.

There are 2 big concerns we had,

• Recording using the builder is a slower process and since this is in the hot path, it becomes super critical to have the recording as light as possible
• High memory allocation during the recording of data and hence increased Young Gen activity.

See this repo where I have a few benchmarks run where there was a clear issue with the current approach to record values.

In the above example, I have commented out the meter filter to just show that there is still a clear difference if there are no meter filters involved. But in real uses, we will have multiple meter filters and that will only be slower (also increase allocation). And when these meter filters involve adding tags/manipulating tags/changing the name of the meter it just gets worse.

We were forced to use a Cache for directly interacting with the meters to have better performance. This is also not straightforward, as we need to handle meter removal and invalidating the cache when meters are removed and other cases. Yet we encourage our internal users to use the cache to have an improved performance number.

[jonatan-ivanov]

@lenin-jaganathan Could you please check if the new withRegistry feature (since 1.12.0) can help mitigating/eliminating those issues? It should eliminate the need of a new builder when you would like to attach tags dynamically, see: #4097

@qweek What do you think, can #4097 supersede this PR?

[lenin-jaganathan]

I explored the option provided in the new version to attach tags dynamically. But, my ask still differs a bit from what is being discussed here. My concern is calling the register on the registry seems to be a costly operation (especially with meter filters this creates a lot of Garbage and exec time is also elevated highly).

What I was looking for is a cache-like solution that will return the existing meters for a given set of meter names, tags, etc.

[lenin-jaganathan]

I think #2275 relates more to the issue I discussed and I will add it here.

[github-actions]

If you would like us to look at this PR, please provide the requested information. If the information is not provided within the next 7 days this PR will be closed.