Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

gsmet · 2023-02-02T17:41:13Z

gsmet · 2023-02-02T18:04:07Z

I changed the implementation a bit to keep the optimizations in place.

geoand · 2023-02-03T07:59:55Z

The native tests seem to not like this change :)

gwenneg · 2023-02-03T08:24:26Z

...ions/caffeine/deployment/src/main/java/io/quarkus/caffeine/deployment/CaffeineProcessor.java

+
+    @BuildStep(onlyIf = NativeOrNativeSourcesBuild.class)
+    NativeImageSystemPropertyBuildItem registerRecordStatsImplementationsIfMicrometerAround(
+            MetricsCapabilityBuildItem metricsCapability) {


This may help fix the native tests:

Suggested change

MetricsCapabilityBuildItem metricsCapability) {

Optional<MetricsCapabilityBuildItem> metricsCapability) {

Ah... yes... I tested the case exercising the new code but didn't test the other modules where Micrometer is not around...

…s around Fixes quarkusio#30744

gwenneg

I don't know Caffeine caches well enough to say if the new list contains everything it should.

Other than that, LGTM. Thanks!

gwenneg · 2023-02-03T10:01:46Z

...ffeine/runtime/src/main/java/io/quarkus/caffeine/runtime/graal/CacheConstructorsFeature.java

+                "com.github.benmanes.caffeine.cache.SILSMS",
+                "com.github.benmanes.caffeine.cache.SSSA",
+                "com.github.benmanes.caffeine.cache.SSLSA",
+                "com.github.benmanes.caffeine.cache.SSLSMS",
+                "com.github.benmanes.caffeine.cache.SSSMS",
+                "com.github.benmanes.caffeine.cache.SSSMSA",
+                "com.github.benmanes.caffeine.cache.SSSMSW",
+                "com.github.benmanes.caffeine.cache.SSSW"


@ben-manes Could you please confirm that this list contains all caches that should be registered for reflection when the cache metrics are enabled?

FWIW, it doesn't contain all of them. But I think it contains all the ones corresponding to the ones we already have.

How did you determine which ones we need?

If possible, you might write a unit test to assert that the bundled configurations work in native image. I don't know the class mappings offhand as it is done by logic and a test would avoid regressions if the listing became stale.

I use Guava's Sets.cartesianProduct(features) which has a 2^n growth rate, so each new feature might double the class listings. In reality that is pruned down by reusing classes if they match the required data shape, e.g. an entry with key/value/timestamp can share a single class by either expireAfterAccess, expireAfterWrite, or variable expiration configurations. Caffeine's unit tests likewise use this cartesian product trick for parameterized tests to avoid regressions as features can interact in subtle ways.

quarkus-bot · 2023-02-03T11:34:28Z

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.32.0` -> `2.33.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.1.Final` -> `2.16.2.Final` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.1.Final` -> `2.16.2.Final` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.33.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#2330---2023-01-26) ##### Added - `ProcessRunner` has added some convenience methods so it can be used for maven testing. ([#1496](diffplug/spotless#1496)) - `ProcessRunner` allows to limit captured output to a certain number of bytes. ([#1511](diffplug/spotless#1511)) - `ProcessRunner` is now capable of handling long-running tasks where waiting for exit is delegated to the caller. ([#1511](diffplug/spotless#1511)) - Allow to specify node executable for node-based formatters using `nodeExecutable` parameter ([#1500](diffplug/spotless#1500)) ##### Fixed - The default list of type annotations used by `formatAnnotations` has had 8 more annotations from the Checker Framework added [#1494](diffplug/spotless#1494) ##### Changes - **POTENTIALLY BREAKING** Bump minimum JRE from 8 to 11, next release likely to bump bytecode to Java 11 ([#1514](diffplug/spotless#1514) part 1 of [#1337](diffplug/spotless#1337)) - Rename `YamlJacksonStep` into `JacksonYamlStep` while normalizing Jackson usage ([#1492](diffplug/spotless#1492)) - Convert `gson` integration to use a compile-only source set ([#1510](diffplug/spotless#1510)). - \*\* POTENTIALLY BREAKING\*\* Removed support for KtLint 0.3x and 0.45.2 ([#1475](diffplug/spotless#1475)) - `KtLint` does not maintain a stable API - before this MR, we supported every breaking change in the API since 2019. - From now on, we will support no more than 2 breaking changes at a time. - NpmFormatterStepStateBase delays `npm install` call until the formatter is first used. This enables better integration with `gradle-node-plugin`. ([#1522](diffplug/spotless#1522)) - Bump default `ktlint` version to latest `0.48.1` -> `0.48.2` ([#1529](diffplug/spotless#1529)) - Bump default `scalafmt` version to latest `3.6.1` -> `3.7.1` ([#1529](diffplug/spotless#1529)) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v2.16.2.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.2.Final) [Compare Source](quarkusio/quarkus@2.16.1.Final...2.16.2.Final) ##### Complete changelog - [#30976](quarkusio/quarkus#30976) - Metrics - check if index contains class before attempting to use it - [#30965](quarkusio/quarkus#30965) - JandexBeanInfoAdapter.getMetricAnnotationsThroughStereotype is not null safe - [#30959](quarkusio/quarkus#30959) - Return text from /q/metrics when the Accept header contains html - [#30953](quarkusio/quarkus#30953) - Fix OIDC capability string - [#30947](quarkusio/quarkus#30947) - Ignore interface/class without default constructs fields in SB config - [#30940](quarkusio/quarkus#30940) - Use SchemaType.ARRAY instead of "ARRAY" for native support - [#30919](quarkusio/quarkus#30919) - Compilation to native fails, when quarkus-smallrye-openapi is included - [#30916](quarkusio/quarkus#30916) - Add AppCDS documentation - [#30896](quarkusio/quarkus#30896) - Quarkus spring-boot-properties extension unable to handle complex configuration. - [#30878](quarkusio/quarkus#30878) - Bump postgresql from 42.5.2 to 42.5.3 - [#30866](quarkusio/quarkus#30866) - Only run the quickstart compilation for main - [#30851](quarkusio/quarkus#30851) - Fixed return type typo in smallrye graphQL guide - [#30844](quarkusio/quarkus#30844) - Fixed greeting in getting started guide - [#30839](quarkusio/quarkus#30839) - Fix handling of Accept header in graphQL - [#30833](quarkusio/quarkus#30833) - Update docs to show BuildProducer use as method parameter instead of field - [#30828](quarkusio/quarkus#30828) - Make OIDC session cookie same site lax by default - [#30826](quarkusio/quarkus#30826) - Caffeine - Automatically register metrics cache impls if Micrometer is around - [#30825](quarkusio/quarkus#30825) - Fix comment about Caffeine optimization - [#30823](quarkusio/quarkus#30823) - Change accept header to valid plain text in micrometer documentation - [#30821](quarkusio/quarkus#30821) - Packaging type -Dquarkus.package.create-appcds=true isn't documented - [#30815](quarkusio/quarkus#30815) - Update SmallRye Config to 2.13.2 - [#30812](quarkusio/quarkus#30812) - Manage the apache-mime4j dependency - [#30806](quarkusio/quarkus#30806) - */* in Accept header is ignored if not listed as the first item - [#30805](quarkusio/quarkus#30805) - MailTemplateInstance with attachments - [#30803](quarkusio/quarkus#30803) - Support file and byte array attachments in `MailTemplateInstance` - [#30797](quarkusio/quarkus#30797) - OIDC login not work - [#30783](quarkusio/quarkus#30783) - <artifactId> uses 'quarkus.platform.artifact-id' property - [#30778](quarkusio/quarkus#30778) - Avoid creating 3 Liquibase MongoDB instances for startup operations - [#30776](quarkusio/quarkus#30776) - Ensure that AwsProxyRequestContext can be used with [@Context](https://github.com/Context) in RESTEasy Reactive - [#30767](quarkusio/quarkus#30767) - Remove duplicate notification of SseBroadcaster's onErrorListeners - [#30765](quarkusio/quarkus#30765) - Bump postgresql from 42.5.1 to 42.5.2 - [#30755](quarkusio/quarkus#30755) - Update ForwardedParser to validate the port - [#30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW - [#30536](quarkusio/quarkus#30536) - munitnyucontextmanager non helpful error reporting - [#29753](quarkusio/quarkus#29753) - Introduce ConnectionFactoryWrapperBuildItem - [#29605](quarkusio/quarkus#29605) - Update docs to reflect that injection should not - [#27774](quarkusio/quarkus#27774) - PLANNER-1709 Avoid deprecated penalize/reward overloads - [#23442](quarkusio/quarkus#23442) - problem using quarkus-resteasy-reactive-kotlin-serialization with AwsProxyRequestContext </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v2.16.2.Final`](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) [Compare Source](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

gsmet requested a review from gwenneg February 2, 2023 17:41

quarkus-bot bot added the area/cache label Feb 2, 2023

gsmet force-pushed the cache-metrics branch from 6fd51a1 to d6b5b87 Compare February 2, 2023 18:03

gsmet force-pushed the cache-metrics branch from d6b5b87 to 5638f56 Compare February 2, 2023 18:10

gsmet mentioned this pull request Feb 2, 2023

[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW #30744

Closed

gsmet added the triage/backport? label Feb 2, 2023

This comment has been minimized.

Sign in to view

gwenneg reviewed Feb 3, 2023

View reviewed changes

Caffeine - Automatically register metrics cache impls if Micrometer i…

743d6d5

…s around Fixes quarkusio#30744

gsmet force-pushed the cache-metrics branch from 5638f56 to 743d6d5 Compare February 3, 2023 09:31

gwenneg approved these changes Feb 3, 2023

View reviewed changes

gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023

gsmet merged commit 46511e3 into quarkusio:main Feb 4, 2023

quarkus-bot bot added this to the 3.0 - main milestone Feb 4, 2023

gsmet removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023

quarkus-bot bot added the kind/bugfix label Feb 4, 2023

gsmet modified the milestones: 3.0 - main, 2.16.2.Final Feb 7, 2023

gsmet removed the triage/backport? label Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

gsmet commented Feb 2, 2023 •

edited

Loading

gsmet commented Feb 2, 2023

This comment has been minimized.

geoand commented Feb 3, 2023 •

edited

Loading

gwenneg Feb 3, 2023

gsmet Feb 3, 2023

gwenneg left a comment

gwenneg Feb 3, 2023

gsmet Feb 3, 2023

gwenneg Feb 3, 2023

ben-manes Feb 3, 2023

quarkus-bot bot commented Feb 3, 2023

	MetricsCapabilityBuildItem metricsCapability) {
	Optional<MetricsCapabilityBuildItem> metricsCapability) {

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

Conversation

gsmet commented Feb 2, 2023 • edited Loading

gsmet commented Feb 2, 2023

This comment has been minimized.

geoand commented Feb 3, 2023 • edited Loading

gwenneg Feb 3, 2023

Choose a reason for hiding this comment

gsmet Feb 3, 2023

Choose a reason for hiding this comment

gwenneg left a comment

Choose a reason for hiding this comment

gwenneg Feb 3, 2023

Choose a reason for hiding this comment

gsmet Feb 3, 2023

Choose a reason for hiding this comment

gwenneg Feb 3, 2023

Choose a reason for hiding this comment

ben-manes Feb 3, 2023

Choose a reason for hiding this comment

quarkus-bot bot commented Feb 3, 2023

gsmet commented Feb 2, 2023 •

edited

Loading

geoand commented Feb 3, 2023 •

edited

Loading