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

Log a warning when instrumenting a cache that is not recording stats in CaffeineCacheMetrics #5402

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Aug 25, 2024

This PR changes to log a warning when instrumenting a cache that is not recording statistics in the CaffeineCacheMetrics.

See gh-5066

@shakuzen shakuzen added enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module labels Aug 28, 2024
@shakuzen shakuzen added this to the 1.14.0-M3 milestone Aug 28, 2024
@izeye
Copy link
Contributor Author

izeye commented Aug 28, 2024

The Guava cache also has the same concept, but it doesn't seem to have an API corresponding to Policy.isRecordingStats() from the Caffeine cache.

@izeye
Copy link
Contributor Author

izeye commented Aug 28, 2024

In addition to logging the warning, we can try not to register meters that depend on the Caffeine.recordStats() call at all instead of registering zero-valued meters.

@shakuzen
Copy link
Member

In addition to logging the warning, we can try not to register meters that depend on the Caffeine.recordStats() call at all instead of registering zero-valued meters.

That sounds like a good idea to me. It will be more clear we don't have metrics for those rather than saying the metrics for those are 0. Do you want to update this pull request with that or make a separate PR for it?

@izeye
Copy link
Contributor Author

izeye commented Aug 29, 2024

@shakuzen Thanks for the feedback!

Looking into its implementation details, it seems a bit tricky, so it would be better to be discussed in a separate issue. I can try another PR if it's proven to be okay in the issue.

@shakuzen shakuzen merged commit fc6a491 into micrometer-metrics:main Aug 29, 2024
6 checks passed
@izeye izeye deleted the gh-gh-5066 branch August 29, 2024 03:18
@izeye
Copy link
Contributor Author

izeye commented Aug 29, 2024

Looking into its implementation details, it seems a bit tricky, so it would be better to be discussed in a separate issue.

I created #5408.

@izeye
Copy link
Contributor Author

izeye commented Aug 30, 2024

The Guava cache also has the same concept, but it doesn't seem to have an API corresponding to Policy.isRecordingStats() from the Caffeine cache.

I created google/guava#7381 to see if it could be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement 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

Successfully merging this pull request may close these issues.

2 participants