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

Support for removing metrics registered by CaffeineCacheMetrics #5297

Open
anuragagarwal561994 opened this issue Jul 10, 2024 · 12 comments
Open
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Milestone

Comments

@anuragagarwal561994
Copy link

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
Copy link
Member

jonatan-ivanov commented Jul 10, 2024

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?

@jonatan-ivanov jonatan-ivanov added question A user question, probably better suited for StackOverflow waiting for feedback We need additional information before we can continue and removed waiting-for-triage labels Jul 10, 2024
@anuragagarwal561994
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

@jonatan-ivanov jonatan-ivanov added waiting for team An issue we need members of the team to review and removed question A user question, probably better suited for StackOverflow waiting for feedback We need additional information before we can continue labels Jul 10, 2024
@anuragagarwal561994
Copy link
Author

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

@ben-manes
Copy link

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
Copy link
Author

@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
Copy link
Member

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

@jonatan-ivanov jonatan-ivanov added waiting for feedback We need additional information before we can continue and removed waiting for team An issue we need members of the team to review labels Jul 13, 2024
@anuragagarwal561994
Copy link
Author

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
Copy link
Member

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.

@shakuzen shakuzen added enhancement A general enhancement help wanted An issue that a contributor can help us with module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component and removed waiting for feedback We need additional information before we can continue labels Jul 18, 2024
@shakuzen shakuzen added this to the 1.x milestone Jul 18, 2024
@shakuzen shakuzen changed the title Support for removing custom instrumentation meters Support for removing metrics registered by CaffeineCacheMetrics Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement help wanted An issue that a contributor can help us with instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

No branches or pull requests

4 participants