micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.41k stars 974 forks source link

Support for removing metrics registered by CaffeineCacheMetrics #5297

Open anuragagarwal561994 opened 2 weeks ago

anuragagarwal561994 commented 2 weeks ago

Please describe the feature request. Currently we are able to add instrumentations of lets say caffeine cache based on a reference but we can't remove them.

Rationale The rationale behind this is we have a caffiene cache that is configurable from database config and hence we change the reference for the same and they are re-constructed. In this case we remove the reference to old caches and add new ones but we want to re-register the new ones but since one is already registered with the ealier name the new one is not registered and honoured, hence we should have a way to remove the old ones and be able to replace it with the new ones.

jonatan-ivanov commented 2 weeks ago

The MeterRegistry has two remove methods: registry.remove(Meter) and registry.remove(Meter.Id). KafkaMetrics does something similar because of a similar reason: Kafka drops data holder instances and recreates them: Micrometer needs to re-register them, you might want to take a look and see how it is doing this.

Btw we have instrumentation for Caffeine, see CaffeineCacheMetrics doesn't that provide the data you need?

anuragagarwal561994 commented 2 weeks ago

Yes I am using CaffeineCacheMetrics only but it doens't provide a remove method of the metrics it is registering so for example if I do something like:

var cache = Caffeine.newBuilder().build();
Metrics.addRegistry(new SimpleMeterRegistry());
CaffeineCacheMetrics.monitor(Metrics.globalRegistry, cache, "cache");
cache = Caffeine.newBuilder().build();
CaffeineCacheMetrics.monitor(Metrics.globalRegistry, cache, "cache");

Then second instance of cache is not replaced by earlier instance of cahce, I would be adding my values in the new instance but still the meters will be recording the ealrier instance or since it is WeakReference not sure what will be the behaviour.

anuragagarwal561994 commented 2 weeks ago

I have to do something like this, myself before adding another cache which seems like can be provided as a method in CaffeineCacheMetrics.unmonitor(registry, cacheName) or something similar:

List<Meter> metersToRemove = new ArrayList<>();
    MeterRegistry meterRegistry = Metrics.globalRegistry;
    meterRegistry.forEachMeter(meter -> {
      if (StringUtils.equals(meter.getId().getTag("cache"), cacheName)) {
        metersToRemove.add(meter);
      }
    });
    metersToRemove.forEach(meterRegistry::remove);
jonatan-ivanov commented 2 weeks ago

Oh I think I misunderstood what you are trying to do.

Yes I am using CaffeineCacheMetrics only but it doens't provide a remove method of the metrics it is registering

It should not, why would it need to?

Then second instance of cache is not replaced by earlier instance of cahce, I would be adding my values in the new instance but still the meters will be recording the ealrier instance

That's expected, why do you want to use the binder twice?

anuragagarwal561994 commented 2 weeks ago

Because I want to re-register the cache with the new config. I have a cache which I have configured with lets say a refresh interval of 30 mins and now in the runitme I would want to change it to 1 hour. So I would create a new cache instance and replace the old one in my code and would like to re-register the metrics in micrometer for the new cache instance.

jonatan-ivanov commented 2 weeks ago

I see, so you are dropping your whole cache with its config runtime and create a new one from scratch. I think a close/de-register on the cache binder could be possible but this could be an even broader possible feature for every binder.

Let me discuss this with @shakuzen and see what we could do.

anuragagarwal561994 commented 2 weeks ago

I haven't checked other metrics, I think there might be some instrumentation where this can be possible

ben-manes commented 2 weeks ago

fwiw, you can reconfigure the cache at runtime using cache.policy(), e.g. setRefreshesAfter(duration). The features have to be defined at construction time, but afterwards you can perform feature-specific inspections and operations.

anuragagarwal561994 commented 2 weeks ago

@ben-manes this I seem is a hidden gem and I believe anywhere on the internet if I talk about caffeine I can find you :)

jonatan-ivanov commented 2 weeks ago

@anuragagarwal561994 With the option to reconfigure caffeine runtime using the same instance as Ben suggested, do you still want this change?

anuragagarwal561994 commented 2 weeks ago

I think that still would very well needed, because I understand there are options to be configured by there can be other use cases like changing the loader of the cache or something, basically creating the new cache and registering its metrics under the same name.

Mainly I believe we should look out these instrumentations created by micrometer as a user should be able to unregister them when they please to and again may be re-register them if needed. For the metrics that users record there is an option to unregister, even the prometheus library provides it, it would be better for micrometer to provide these options as well.

shakuzen commented 1 week ago

I think that still would very well needed, because I understand there are options to be configured by there can be other use cases like changing the loader of the cache or something, basically creating the new cache and registering its metrics under the same name.

I understand it's possible to think of a use case in which it is still needed but it seems very niche and we've never had anyone ask for it until now. Given the tip @ben-manes shared about changing config at runtime, it would be hard for us to justify prioritizing this feature unless there is more interest from users shown for it. We could consider a pull request if someone makes one. I'll mark this as help wanted.