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

Metrics from Caffeine caches #1608

Merged
merged 9 commits into from
Jun 23, 2020
Merged

Conversation

john-karp
Copy link
Contributor

@john-karp john-karp commented Jun 17, 2020

Add ability to gather metrics from Caffeine cache library. An instance of MetricsStatsCounter should be passed to the Caffeine cache builder, resulting in Caffeine using that class to publish metrics to the desired metrics registry. (An example of this usage is in the UT.)

Code was taken from example given by Caffeine library author provided at https://github.com/ben-manes/caffeine/blob/master/examples/stats-metrics/src/test/java/com/github/benmanes/caffeine/examples/stats/metrics/MetricsStatsCounterTest.java

Adding it into official dropwizard metrics repo should make it easier for dependent projects to take advantage of.

It has the Apache Public License 2.0, same as this project, so I assume it is okay to integrate?

Changes made to copied sources:

  • Package names made consistent rest of project
  • Switched UT to use JUnit, added more UT
  • Used Histogram/Timer where possible
  • Break down evictions by RemovalCause
  • Documentation

@john-karp john-karp marked this pull request as ready for review June 17, 2020 23:22
@john-karp
Copy link
Contributor Author

@ben-manes see any reason not to?

@ben-manes
Copy link

The only limitation is sometimes users add cache.estimatedSize, e.g. CaffeineCacheMetrics in Micrometer. I am not sure why that's desired unless one is using an unbounded cache, at which point there's inherent risk in production. The Dropwizard Ehcache2 metrics have an objects for the cache size, whereas the JCache one does not. Ideally size would be flat when it reaches the logical or bounded size threshold and not offer a meaningful devops data point.

Based on that, I think the push model is nice though polling like Guava's CacheStats is also an option. You are welcome to take that example code, or not, as you see fit.

@joschi joschi requested review from a team June 18, 2020 07:06
@joschi joschi added the feature label Jun 18, 2020
Copy link

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

Copy link
Member

@joschi joschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

Would you please add a page in the documentation similar to the EhCache documentation (https://metrics.dropwizard.io/4.1.2/manual/ehcache.html)?

Copy link
Member

@arteam arteam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Looking forward for Caffeine metrics.

@john-karp
Copy link
Contributor Author

@joschi - added documentation

Copy link
Member

@joschi joschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-karp @ben-manes LGTM, thanks a lot!

@joschi joschi added this to the 4.1.10 milestone Jun 23, 2020
@joschi joschi merged commit e4d8a84 into dropwizard:release/4.1.x Jun 23, 2020
@ben-manes
Copy link

Thank you @john-karp for taking my simple example and making it production worthy! :)

@john-karp john-karp deleted the caffeine branch June 23, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants