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

Generate a CacheCreatedEvent from the Spring Caching Abstraction [SPR-17350] #21884

Open
spring-issuemaster opened this issue Oct 5, 2018 · 2 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Tyler K Van Gorder opened SPR-17350 and commented

Now that Spring Boot 2.x uses Micrometer for its metrics abstraction, it is helpful to have the caching abstraction instrumented such that metrics around cache size, hits, misses, puts, and evictions are published through Micrometer.

Spring Boot does do some limited auto-configuration to instrument the caching abstraction but is limited to only those caches that are known at startup time. This limitation is due to the fact that there is no way to observe when a new, dynamic cache has been created by the Spring Caching abstraction.

The current functionality means that any methods that are marked as @Cacheable will NOT have their metrics recorded through Micrometer.

To solve this problem without having to embed knowledge of Micrometer into each cache manager implementation, it would be better to have those cache managers publish an application event, CacheCreatedEvent.

This would allow for Spring Boot to auto-configure a listener for that event and bind all caches, dynamic or otherwise, to Micrometer's meter registry.

Looking at the code, the CaceCreateEvent should be emitted from the CacheManager.getCache(String name) method. The event should have both the cache name and the cache manager that was used to create the cache.

Most of the cache manager implementations extend AbstractCacheManager, and therefore this will be a good place to publish the event. However, there are a couple of additional CacheManager implementations (CaffineCacheMAnager, ConcurrentMapCacheManager) that do not extends the abstract base class and will also need to be modified to publish the event.


Affects: 5.0.9, 5.1 GA

Reference URL: spring-projects/spring-boot#14576

4 votes, 6 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 9, 2018

Stéphane Nicoll commented

The current functionality means that any methods that are marked as @Cacheable will NOT have their metrics recorded through Micrometer.

That's a very wide statement that doesn't seem accurate to me. If you use a cache library with a configuration file that lists the caches, which arguably is what you should be doing, they will be instrumented just fine.

it would be better to have those cache managers publish an application event, CacheCreatedEvent.

I see a semantic problem with this. When the CacheManager initializes itself, it is also going to create caches (the ones that it knows upfront because the underlying cache library provides them right away). We can't really call the event like that and not fire an event in such a case.

The other end can accommodate to this contract as long as it moves the handling of metrics to a listener only. Unfortunately, we don't manage the hazelcast cache manager so we'll have to sync with them before being able to do that.

Triggering an event is a nice idea to let other components know what is happening. There is no way to enforce that in the contract and there are several external implementations so I am not 100% sure that's the right way to go. Let's tentatively assign this to 5.2 and investigate a bit more. At this point, I am tempted to have a restricted MissingCacheCreatedEvent or something like that to narrow the contract a bit.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 9, 2018

Tyler K Van Gorder commented

Sorry, you are right, in that my statement is too broad. It really should read "The current functionality means that any methods that are marked as @Cacheable that have not had their cache names defined in configuration will not have their metrics recorded through Micrometer."

Ugh, you are totally correct in that the code paths for the initial creation of caches vs the "on the fly" cache creation are different. I like your idea of narrowing the scope of this change to MissingCacheCreatedEvent, but like you pointed out, there is no way to concretely enforce that each cache manager implementation generates that event. It is a deceivingly tricky problem and I appreciate you looking into it.

I think I have a third idea on how to fix this at the Spring Boot level, I am going to leave a comment on there. 

 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.