Merge pull request #40145 from gwenneg/issue-40042

Do not increment metrics on CaffeineCache#getIfPresent call
quarkusio · Apr 19, 2024 · c85f5e2 · c85f5e2
2 parents 3afccc5 + 1c9f618
commit c85f5e2
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 10 deletions.
diff --git a/...ions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java b/...ions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java
@@ -127,13 +127,10 @@ public <V> CompletableFuture<V> getIfPresent(Object key) {
         Objects.requireNonNull(key, NULL_KEYS_NOT_SUPPORTED_MSG);
         CompletableFuture<Object> 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
@@ -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
@@ -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<String> 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
@@ -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
@@ -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)));
+    }
 }