From 1c9f618d34ed01a340b7770bf44e18bcc802f606 Mon Sep 17 00:00:00 2001 From: Gwenneg Lepage Date: Thu, 18 Apr 2024 19:49:19 +0200 Subject: [PATCH] Do not increment metrics on CaffeineCache#getIfPresent call --- .../runtime/caffeine/CaffeineCacheImpl.java | 3 - .../quarkus/it/cache/ExpensiveResource.java | 4 +- .../it/cache/GetIfPresentResource.java | 36 +++++++++++ .../src/main/resources/application.properties | 1 + .../io/quarkus/it/cache/CacheTestCase.java | 60 +++++++++++++++++-- 5 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 integration-tests/cache/src/main/java/io/quarkus/it/cache/GetIfPresentResource.java diff --git a/extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java b/extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java index c8baff5992b7a..dff31eb8dab1e 100644 --- a/extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java +++ b/extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java @@ -127,13 +127,10 @@ public CompletableFuture getIfPresent(Object key) { Objects.requireNonNull(key, NULL_KEYS_NOT_SUPPORTED_MSG); CompletableFuture existingCacheValue = cache.getIfPresent(key); - // record metrics, if not null apply casting if (existingCacheValue == null) { - statsCounter.recordMisses(1); return null; } else { LOGGER.tracef("Key [%s] found in cache [%s]", key, cacheInfo.name); - statsCounter.recordHits(1); // cast, but still throw the CacheException in case it fails return unwrapCacheValueOrThrowable(existingCacheValue) diff --git a/integration-tests/cache/src/main/java/io/quarkus/it/cache/ExpensiveResource.java b/integration-tests/cache/src/main/java/io/quarkus/it/cache/ExpensiveResource.java index 8efdcf9abfdc0..84cdc5a3a75bd 100644 --- a/integration-tests/cache/src/main/java/io/quarkus/it/cache/ExpensiveResource.java +++ b/integration-tests/cache/src/main/java/io/quarkus/it/cache/ExpensiveResource.java @@ -13,11 +13,13 @@ @Path("/expensive-resource") public class ExpensiveResource { + public static final String EXPENSIVE_RESOURCE_CACHE_NAME = "expensiveResourceCache"; + private int invocations; @GET @Path("/{keyElement1}/{keyElement2}/{keyElement3}") - @CacheResult(cacheName = "expensiveResourceCache", lockTimeout = 5000) + @CacheResult(cacheName = EXPENSIVE_RESOURCE_CACHE_NAME, lockTimeout = 5000) public ExpensiveResponse getExpensiveResponse(@PathParam("keyElement1") @CacheKey String keyElement1, @PathParam("keyElement2") @CacheKey String keyElement2, @PathParam("keyElement3") @CacheKey String keyElement3, @QueryParam("foo") String foo) { diff --git a/integration-tests/cache/src/main/java/io/quarkus/it/cache/GetIfPresentResource.java b/integration-tests/cache/src/main/java/io/quarkus/it/cache/GetIfPresentResource.java new file mode 100644 index 0000000000000..14e9feb8082c4 --- /dev/null +++ b/integration-tests/cache/src/main/java/io/quarkus/it/cache/GetIfPresentResource.java @@ -0,0 +1,36 @@ +package io.quarkus.it.cache; + +import static java.util.concurrent.CompletableFuture.completedFuture; + +import java.util.concurrent.CompletionStage; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; + +import org.jboss.resteasy.reactive.RestPath; + +import io.quarkus.cache.Cache; +import io.quarkus.cache.CacheName; +import io.quarkus.cache.CaffeineCache; + +@Path("/get-if-present") +public class GetIfPresentResource { + + public static final String GET_IF_PRESENT_CACHE_NAME = "getIfPresentCache"; + + @CacheName(GET_IF_PRESENT_CACHE_NAME) + Cache cache; + + @GET + @Path("/{key}") + public CompletionStage getIfPresent(@RestPath String key) { + return cache.as(CaffeineCache.class).getIfPresent(key); + } + + @PUT + @Path("/{key}") + public void put(@RestPath String key, String value) { + cache.as(CaffeineCache.class).put(key, completedFuture(value)); + } +} diff --git a/integration-tests/cache/src/main/resources/application.properties b/integration-tests/cache/src/main/resources/application.properties index b94edbb983678..9f87c19434340 100644 --- a/integration-tests/cache/src/main/resources/application.properties +++ b/integration-tests/cache/src/main/resources/application.properties @@ -8,5 +8,6 @@ quarkus.cache.caffeine."forest".expire-after-write=10M quarkus.cache.caffeine."expensiveResourceCache".expire-after-write=10M quarkus.cache.caffeine."expensiveResourceCache".metrics-enabled=true +quarkus.cache.caffeine."getIfPresentCache".metrics-enabled=true io.quarkus.it.cache.SunriseRestClient/mp-rest/url=${test.url} diff --git a/integration-tests/cache/src/test/java/io/quarkus/it/cache/CacheTestCase.java b/integration-tests/cache/src/test/java/io/quarkus/it/cache/CacheTestCase.java index 48fc07d224685..64197281e8775 100644 --- a/integration-tests/cache/src/test/java/io/quarkus/it/cache/CacheTestCase.java +++ b/integration-tests/cache/src/test/java/io/quarkus/it/cache/CacheTestCase.java @@ -1,5 +1,8 @@ package io.quarkus.it.cache; +import static io.quarkus.it.cache.ExpensiveResource.EXPENSIVE_RESOURCE_CACHE_NAME; +import static io.quarkus.it.cache.GetIfPresentResource.GET_IF_PRESENT_CACHE_NAME; +import static io.restassured.RestAssured.given; import static io.restassured.RestAssured.when; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -14,20 +17,65 @@ public class CacheTestCase { @Test - public void testCache() { + void testCache() { + assertMetrics(EXPENSIVE_RESOURCE_CACHE_NAME, 0, 0, 0); + runExpensiveRequest(); + assertMetrics(EXPENSIVE_RESOURCE_CACHE_NAME, 1, 1, 0); + runExpensiveRequest(); + assertMetrics(EXPENSIVE_RESOURCE_CACHE_NAME, 1, 1, 1); + runExpensiveRequest(); - when().get("/expensive-resource/invocations").then().statusCode(200).body(is("1")); + assertMetrics(EXPENSIVE_RESOURCE_CACHE_NAME, 1, 1, 2); - String metricsResponse = when().get("/q/metrics").then().extract().asString(); - assertTrue(metricsResponse.contains("cache_puts_total{cache=\"expensiveResourceCache\"} 1.0")); - assertTrue(metricsResponse.contains("cache_gets_total{cache=\"expensiveResourceCache\",result=\"miss\"} 1.0")); - assertTrue(metricsResponse.contains("cache_gets_total{cache=\"expensiveResourceCache\",result=\"hit\"} 2.0")); + when().get("/expensive-resource/invocations").then().statusCode(200).body(is("1")); } private void runExpensiveRequest() { when().get("/expensive-resource/I/love/Quarkus?foo=bar").then().statusCode(200).body("result", is("I love Quarkus too!")); } + + @Test + void testGetIfPresentMetrics() { + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 0, 0, 0); + + String cacheKey = "foo"; + String cacheValue = "bar"; + + given().pathParam("key", cacheKey) + .when().get("/get-if-present/{key}") + .then().statusCode(204); + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 0, 1, 0); + + given().pathParam("key", cacheKey) + .when().get("/get-if-present/{key}") + .then().statusCode(204); + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 0, 2, 0); + + given().pathParam("key", cacheKey).body(cacheValue) + .when().put("/get-if-present/{key}") + .then().statusCode(204); + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 1, 2, 0); + + given().pathParam("key", cacheKey) + .when().get("/get-if-present/{key}") + .then().statusCode(200).body(is(cacheValue)); + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 1, 2, 1); + + given().pathParam("key", cacheKey) + .when().get("/get-if-present/{key}") + .then().statusCode(200).body(is(cacheValue)); + assertMetrics(GET_IF_PRESENT_CACHE_NAME, 1, 2, 2); + } + + private void assertMetrics(String cacheName, double expectedPuts, double expectedMisses, double expectedHits) { + String metricsResponse = when().get("/q/metrics").then().extract().asString(); + assertTrue(metricsResponse.contains(String.format("cache_puts_total{cache=\"%s\"} %.1f", cacheName, expectedPuts))); + assertTrue(metricsResponse + .contains(String.format("cache_gets_total{cache=\"%s\",result=\"miss\"} %.1f", cacheName, expectedMisses))); + assertTrue(metricsResponse + .contains(String.format("cache_gets_total{cache=\"%s\",result=\"hit\"} %.1f", cacheName, expectedHits))); + } }