SLING-11197: fix NaN value returned by cache_hit_rate metric (#27)
diff --git a/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java b/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
index 49e795b..9bba5a8 100644
--- a/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
+++ b/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
@@ -196,8 +196,11 @@
cacheMisses = metricsService.counter(METRIC_NS + "." + servletRegistrationProperties + ".cache_misses");
requestsServed = metricsService.counter(METRIC_NS + "." + servletRegistrationProperties + ".requests_total");
gaugeCacheHitRate = METRIC_NS + "." + servletRegistrationProperties + ".cache_hit_rate";
- metricRegistry.register(gaugeCacheHitRate,
- (Gauge<Float>) () -> (cacheHits.getCount() / (float) (cacheHits.getCount() + cacheMisses.getCount())));
+ metricRegistry.register(gaugeCacheHitRate, (Gauge<Float>) () -> {
+ float hitCount = cacheHits.getCount();
+ float missCount = cacheMisses.getCount();
+ return hitCount > 0 || missCount > 0 ? hitCount / (hitCount + missCount) : 0.0f;
+ });
requestTimer = metricsService.timer(METRIC_NS + "." + servletRegistrationProperties + ".requests_timer");
}
diff --git a/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java b/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
index a7a2f52..1fe9d84 100644
--- a/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
@@ -21,7 +21,6 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Map;
-import java.util.stream.Stream;
import javax.servlet.Servlet;
@@ -42,14 +41,20 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;
+import com.codahale.metrics.Gauge;
import com.codahale.metrics.MetricRegistry;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@RunWith(MockitoJUnitRunner.class)
@@ -62,17 +67,22 @@
private static final String TEST_QUERY = "{\"query\": \"{ currentResource { resourceType name } }\" }";
private Resource resource;
+ @Mock
+ private MetricsService metricsService;
+ @Spy
+ private MetricRegistry metricRegistry = new MetricRegistry();
+ @Mock
+ private Counter counter;
+ @Mock
+ private Timer timer;
+
@Before
public void setUp() {
- MetricsService metricsService = mock(MetricsService.class);
- when(metricsService.counter(any(String.class))).thenReturn(mock(Counter.class));
-
- Timer timer = mock(Timer.class);
when(timer.time()).thenReturn(mock(Timer.Context.class));
+ when(metricsService.counter(any(String.class))).thenReturn(counter);
when(metricsService.timer(any(String.class))).thenReturn(timer);
context.registerService(MetricsService.class, metricsService);
- MetricRegistry metricRegistry = mock(MetricRegistry.class);
context.registerService(MetricRegistry.class, metricRegistry, "name", "sling");
QueryExecutor queryExecutor = mock(QueryExecutor.class);
@@ -132,6 +142,38 @@
assertEquals("Persisted queries are disabled.", response.getStatusMessage());
}
+ @Test
+ public void testMetricsRegistered() {
+ context.registerInjectActivateService(new SimpleGraphQLCacheProvider());
+ context.registerInjectActivateService(new GraphQLServlet(), ServletResolverConstants.SLING_SERVLET_RESOURCE_TYPES, TEST_RESOURCE_TYPE,
+ "persistedQueries.suffix", "");
+
+ String expectedMetricPrefix = "org.apache.sling.graphql.core.servlet.GraphQLServlet.rt:" + TEST_RESOURCE_TYPE + ".m:GET.e:gql";
+
+ verify(metricsService).counter(expectedMetricPrefix + ".cache_hits");
+ verify(metricsService).counter(expectedMetricPrefix + ".requests_total");
+ verify(metricsService).timer(expectedMetricPrefix + ".requests_timer");
+ verify(metricRegistry).register(eq(expectedMetricPrefix + ".cache_hit_rate"), any(Gauge.class));
+ }
+
+ @Test
+ public void testCacheHitRatioMetric () {
+ context.registerInjectActivateService(new SimpleGraphQLCacheProvider());
+ context.registerInjectActivateService(new GraphQLServlet(), ServletResolverConstants.SLING_SERVLET_RESOURCE_TYPES, TEST_RESOURCE_TYPE,
+ "persistedQueries.suffix", "/persisted");
+
+ // test resource type, default method, default extension
+ String expectedMetric = "org.apache.sling.graphql.core.servlet.GraphQLServlet.rt:" + TEST_RESOURCE_TYPE + ".m:GET.e:gql.cache_hit_rate";
+
+ assertTrue(metricRegistry.getGauges().containsKey(expectedMetric));
+ assertEquals(0.0f, metricRegistry.getGauges().get(expectedMetric).getValue());
+
+ // increments both the hit and miss metric
+ when(counter.getCount()).thenReturn(1L);
+
+ assertEquals(0.5f, metricRegistry.getGauges().get(expectedMetric).getValue());
+ }
+
private void assertPostWithBody(String contentType, int expectedStatus) throws IOException {
context.registerInjectActivateService(new SimpleGraphQLCacheProvider());
context.registerInjectActivateService(new GraphQLServlet(), ServletResolverConstants.SLING_SERVLET_RESOURCE_TYPES, TEST_RESOURCE_TYPE,
