From 18ce89fbe004c90d4e2eae58cd1788d521898be0 Mon Sep 17 00:00:00 2001 From: "Raphael P. Barazzutti" Date: Sun, 5 Aug 2018 07:00:59 +0200 Subject: [PATCH 1/5] MetricsFilter to report on HTTP status code Signed-off-by: Raphael P. Barazzutti --- .../client/filter/MetricsFilter.java | 22 ++++++++++++------- .../client/filter/MetricsFilterTest.java | 19 +++++++++++----- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java index 026a8f73e..6e9838f31 100644 --- a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java +++ b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java @@ -1,6 +1,7 @@ package io.prometheus.client.filter; import io.prometheus.client.Histogram; +import io.prometheus.client.SimpleTimer; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -9,6 +10,7 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import java.io.IOException; /** @@ -65,7 +67,8 @@ public class MetricsFilter implements Filter { private String help = "The time taken fulfilling servlet requests"; private double[] buckets = null; - public MetricsFilter() {} + public MetricsFilter() { + } public MetricsFilter( String metricName, @@ -91,7 +94,7 @@ private String getComponents(String str) { return str; } int count = 0; - int i = -1; + int i = -1; do { i = str.indexOf("/", i + 1); if (i < 0) { @@ -107,7 +110,7 @@ private String getComponents(String str) { @Override public void init(FilterConfig filterConfig) throws ServletException { Histogram.Builder builder = Histogram.build() - .labelNames("path", "method"); + .labelNames("path", "method", "status_code"); if (filterConfig == null && isEmpty(metricName)) { throw new ServletException("No configuration object provided, and no metricName passed via constructor"); @@ -153,23 +156,26 @@ public void init(FilterConfig filterConfig) throws ServletException { @Override public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException { - if (!(servletRequest instanceof HttpServletRequest)) { + if (!(servletRequest instanceof HttpServletRequest) || !(servletResponse instanceof HttpServletResponse)) { filterChain.doFilter(servletRequest, servletResponse); return; } HttpServletRequest request = (HttpServletRequest) servletRequest; + HttpServletResponse response = (HttpServletResponse) servletResponse; + String path = request.getRequestURI(); - Histogram.Timer timer = histogram - .labels(getComponents(path), request.getMethod()) - .startTimer(); + SimpleTimer timer = new SimpleTimer(); try { filterChain.doFilter(servletRequest, servletResponse); } finally { - timer.observeDuration(); + double time =timer.elapsedSeconds(); + histogram + .labels(getComponents(path), request.getMethod(), Integer.toString(response.getStatus())) + .observe(time); } } diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java index 4c69f0b1f..77596da0c 100644 --- a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java @@ -51,13 +51,15 @@ public void init() throws Exception { when(req.getMethod()).thenReturn(HttpMethods.GET); HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); + FilterChain c = mock(FilterChain.class); f.doFilter(req, res, c); verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status_code"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET,Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -71,6 +73,8 @@ public void doFilter() throws Exception { when(req.getMethod()).thenReturn(HttpMethods.GET); HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); + FilterChain c = mock(FilterChain.class); String name = "foo"; @@ -84,7 +88,7 @@ public void doFilter() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method"}, new String[]{path, HttpMethods.GET}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method","status_code"}, new String[]{path, HttpMethods.GET,Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -114,9 +118,11 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { constructed.init(mock(FilterConfig.class)); HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); + constructed.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method"}, new String[]{path, HttpMethods.POST}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method","status_code"}, new String[]{path, HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sum); assertEquals(0.1, sum, 0.01); } @@ -143,18 +149,19 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn("foo"); HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); f.init(cfg); f.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method"}, new String[]{"/foo", HttpMethods.POST}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status_code"}, new String[]{"/foo", HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK)}); assertEquals(0.1, sum, 0.01); - final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "le"}, new String[]{"/foo", HttpMethods.POST, "0.05"}); + final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK), "0.05"}); assertNotNull(le05); assertEquals(0, le05, 0.01); - final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "le"}, new String[]{"/foo", HttpMethods.POST, "0.15"}); + final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK),"0.15"}); assertNotNull(le15); assertEquals(1, le15, 0.01); From 1fddaefe3dd206cd088530155557dae8d26b6304 Mon Sep 17 00:00:00 2001 From: "Raphael P. Barazzutti" Date: Sun, 5 Aug 2018 08:45:56 +0200 Subject: [PATCH 2/5] added extra test for status code Signed-off-by: Raphael P. Barazzutti --- .../client/filter/MetricsFilterTest.java | 64 ++++++++++++++++--- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java index 77596da0c..276eda652 100644 --- a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java @@ -13,6 +13,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.util.Enumeration; +import java.util.HashMap; +import java.util.Map; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -59,7 +61,7 @@ public void init() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status_code"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET,Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status_code"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -88,7 +90,7 @@ public void doFilter() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method","status_code"}, new String[]{path, HttpMethods.GET,Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method", "status_code"}, new String[]{path, HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -122,7 +124,7 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { constructed.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method","status_code"}, new String[]{path, HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method", "status_code"}, new String[]{path, HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sum); assertEquals(0.1, sum, 0.01); } @@ -155,20 +157,20 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { f.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status_code"}, new String[]{"/foo", HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status_code"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); assertEquals(0.1, sum, 0.01); - final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST,Integer.toString(HttpServletResponse.SC_OK), "0.05"}); + final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.05"}); assertNotNull(le05); assertEquals(0, le05, 0.01); - final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK),"0.15"}); + final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.15"}); assertNotNull(le15); assertEquals(1, le15, 0.01); final Enumeration samples = CollectorRegistry.defaultRegistry.metricFamilySamples(); Collector.MetricFamilySamples sample = null; - while(samples.hasMoreElements()) { + while (samples.hasMoreElements()) { sample = samples.nextElement(); if (sample.name.equals("foo")) { break; @@ -184,7 +186,53 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { } } // +1 because of the final le=+infinity bucket - assertEquals(buckets.split(",").length+1, count); + assertEquals(buckets.split(",").length + 1, count); + } + + + @Test + public void testStatusCode() throws Exception { + Map sampleStatusCodes = new HashMap(); + sampleStatusCodes.put("/a/page/that/exists", HttpServletResponse.SC_OK); + sampleStatusCodes.put("/a/page/that/doesn-t-exist", HttpServletResponse.SC_NOT_FOUND); + sampleStatusCodes.put("/a/page/that/crashes", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + + FilterConfig cfg = mock(FilterConfig.class); + when(cfg.getInitParameter(anyString())).thenReturn(null); + + String metricName = "foo"; + + when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn(metricName); + when(cfg.getInitParameter(MetricsFilter.PATH_COMPONENT_PARAM)).thenReturn("4"); + + f.init(cfg); + + for (String uri : sampleStatusCodes.keySet()) { + HttpServletRequest req = mock(HttpServletRequest.class); + + when(req.getRequestURI()).thenReturn(uri); + when(req.getMethod()).thenReturn(HttpMethods.GET); + + HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(sampleStatusCodes.get(uri)); + + FilterChain c = mock(FilterChain.class); + + f.doFilter(req, res, c); + + verify(c).doFilter(req, res); + } + + for (String uri : sampleStatusCodes.keySet()) { + + final Double sampleValue = CollectorRegistry.defaultRegistry + .getSampleValue(metricName + "_count", + new String[]{"path", "method", "status_code"}, + new String[]{uri, HttpMethods.GET, + Integer.toString(sampleStatusCodes.get(uri))}); + assertNotNull(sampleValue); + assertEquals(1, sampleValue, 0.0001); + } } } From 9d683d583a9a2ae6f10e1c2bc33a6bdf0cefc04b Mon Sep 17 00:00:00 2001 From: "Raphael P. Barazzutti" Date: Sun, 5 Aug 2018 11:58:42 +0200 Subject: [PATCH 3/5] renaming field name "status" like in Prometheus' docs example Signed-off-by: Raphael P. Barazzutti --- .../io/prometheus/client/filter/MetricsFilter.java | 2 +- .../client/filter/MetricsFilterTest.java | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java index 6e9838f31..ac8e37a37 100644 --- a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java +++ b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java @@ -110,7 +110,7 @@ private String getComponents(String str) { @Override public void init(FilterConfig filterConfig) throws ServletException { Histogram.Builder builder = Histogram.build() - .labelNames("path", "method", "status_code"); + .labelNames("path", "method", "status"); if (filterConfig == null && isEmpty(metricName)) { throw new ServletException("No configuration object provided, and no metricName passed via constructor"); diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java index 276eda652..00dc4166c 100644 --- a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java @@ -61,7 +61,7 @@ public void init() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status_code"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -90,7 +90,7 @@ public void doFilter() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method", "status_code"}, new String[]{path, HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method", "status"}, new String[]{path, HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -124,7 +124,7 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { constructed.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method", "status_code"}, new String[]{path, HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method", "status"}, new String[]{path, HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); assertNotNull(sum); assertEquals(0.1, sum, 0.01); } @@ -157,13 +157,13 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { f.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status_code"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); assertEquals(0.1, sum, 0.01); - final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.05"}); + final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.05"}); assertNotNull(le05); assertEquals(0, le05, 0.01); - final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status_code", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.15"}); + final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.15"}); assertNotNull(le15); assertEquals(1, le15, 0.01); @@ -227,7 +227,7 @@ public void testStatusCode() throws Exception { final Double sampleValue = CollectorRegistry.defaultRegistry .getSampleValue(metricName + "_count", - new String[]{"path", "method", "status_code"}, + new String[]{"path", "method", "status"}, new String[]{uri, HttpMethods.GET, Integer.toString(sampleStatusCodes.get(uri))}); assertNotNull(sampleValue); From 1cee57486d3de68e835e0f238fde5bcee9662c0c Mon Sep 17 00:00:00 2001 From: "Raphael P. Barazzutti" Date: Sun, 12 Aug 2018 13:40:37 +0200 Subject: [PATCH 4/5] redefined status-code feature as an option (disabled by default) Signed-off-by: Raphael P. Barazzutti --- .../client/filter/MetricsFilter.java | 40 ++++++++++++++----- .../client/filter/MetricsFilterTest.java | 25 +++++------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java index ac8e37a37..9ee60051c 100644 --- a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java +++ b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java @@ -48,6 +48,10 @@ * path-components * 0 * + * + * report-status + * false + * * * } * @@ -58,6 +62,7 @@ public class MetricsFilter implements Filter { static final String HELP_PARAM = "help"; static final String METRIC_NAME_PARAM = "metric-name"; static final String BUCKET_CONFIG_PARAM = "buckets"; + static final String REPORT_STATUS = "report-status"; private Histogram histogram = null; @@ -66,6 +71,7 @@ public class MetricsFilter implements Filter { private String metricName = null; private String help = "The time taken fulfilling servlet requests"; private double[] buckets = null; + private boolean reportStatus; public MetricsFilter() { } @@ -109,8 +115,7 @@ private String getComponents(String str) { @Override public void init(FilterConfig filterConfig) throws ServletException { - Histogram.Builder builder = Histogram.build() - .labelNames("path", "method", "status"); + Histogram.Builder builder = Histogram.build(); if (filterConfig == null && isEmpty(metricName)) { throw new ServletException("No configuration object provided, and no metricName passed via constructor"); @@ -142,8 +147,18 @@ public void init(FilterConfig filterConfig) throws ServletException { buckets[i] = Double.parseDouble(bucketParams[i]); } } + + // Allow users to track the HTTP status of responses (disabled by default) + reportStatus = (!isEmpty(filterConfig.getInitParameter(REPORT_STATUS)) && filterConfig.getInitParameter(REPORT_STATUS).toUpperCase().equals("TRUE")); } + if (!reportStatus) + builder = builder + .labelNames("path", "method"); + else + builder = builder + .labelNames("path", "method", "status"); + if (buckets != null) { builder = builder.buckets(buckets); } @@ -156,15 +171,13 @@ public void init(FilterConfig filterConfig) throws ServletException { @Override public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException { - if (!(servletRequest instanceof HttpServletRequest) || !(servletResponse instanceof HttpServletResponse)) { + if (!(servletRequest instanceof HttpServletRequest)) { filterChain.doFilter(servletRequest, servletResponse); return; } HttpServletRequest request = (HttpServletRequest) servletRequest; - HttpServletResponse response = (HttpServletResponse) servletResponse; - String path = request.getRequestURI(); SimpleTimer timer = new SimpleTimer(); @@ -172,10 +185,19 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo try { filterChain.doFilter(servletRequest, servletResponse); } finally { - double time =timer.elapsedSeconds(); - histogram - .labels(getComponents(path), request.getMethod(), Integer.toString(response.getStatus())) - .observe(time); + double time = timer.elapsedSeconds(); + Histogram.Child child; + if (!reportStatus) + child = histogram + .labels(getComponents(path), request.getMethod()); + else { + String status = (servletResponse instanceof HttpServletResponse) ? + Integer.toString(((HttpServletResponse) servletResponse).getStatus()) : "undef"; + child = histogram + .labels(getComponents(path), request.getMethod(), status); + } + + child.observe(time); } } diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java index 00dc4166c..25f876738 100644 --- a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java @@ -53,15 +53,13 @@ public void init() throws Exception { when(req.getMethod()).thenReturn(HttpMethods.GET); HttpServletResponse res = mock(HttpServletResponse.class); - when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); - FilterChain c = mock(FilterChain.class); f.doFilter(req, res, c); verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method", "status"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(metricName + "_count", new String[]{"path", "method"}, new String[]{"/foo/bar/baz/bang", HttpMethods.GET}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -75,8 +73,6 @@ public void doFilter() throws Exception { when(req.getMethod()).thenReturn(HttpMethods.GET); HttpServletResponse res = mock(HttpServletResponse.class); - when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); - FilterChain c = mock(FilterChain.class); String name = "foo"; @@ -90,7 +86,7 @@ public void doFilter() throws Exception { verify(c).doFilter(req, res); - final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method", "status"}, new String[]{path, HttpMethods.GET, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue(name + "_count", new String[]{"path", "method"}, new String[]{path, HttpMethods.GET}); assertNotNull(sampleValue); assertEquals(1, sampleValue, 0.0001); } @@ -120,11 +116,9 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { constructed.init(mock(FilterConfig.class)); HttpServletResponse res = mock(HttpServletResponse.class); - when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); - constructed.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method", "status"}, new String[]{path, HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foobar_baz_filter_duration_seconds_sum", new String[]{"path", "method"}, new String[]{path, HttpMethods.POST}); assertNotNull(sum); assertEquals(0.1, sum, 0.01); } @@ -151,26 +145,25 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn("foo"); HttpServletResponse res = mock(HttpServletResponse.class); - when(res.getStatus()).thenReturn(HttpServletResponse.SC_OK); f.init(cfg); f.doFilter(req, res, c); - final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method", "status"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK)}); + final Double sum = CollectorRegistry.defaultRegistry.getSampleValue("foo_sum", new String[]{"path", "method"}, new String[]{"/foo", HttpMethods.POST}); assertEquals(0.1, sum, 0.01); - final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.05"}); + final Double le05 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "le"}, new String[]{"/foo", HttpMethods.POST, "0.05"}); assertNotNull(le05); assertEquals(0, le05, 0.01); - final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "status", "le"}, new String[]{"/foo", HttpMethods.POST, Integer.toString(HttpServletResponse.SC_OK), "0.15"}); + final Double le15 = CollectorRegistry.defaultRegistry.getSampleValue("foo_bucket", new String[]{"path", "method", "le"}, new String[]{"/foo", HttpMethods.POST, "0.15"}); assertNotNull(le15); assertEquals(1, le15, 0.01); final Enumeration samples = CollectorRegistry.defaultRegistry.metricFamilySamples(); Collector.MetricFamilySamples sample = null; - while (samples.hasMoreElements()) { + while(samples.hasMoreElements()) { sample = samples.nextElement(); if (sample.name.equals("foo")) { break; @@ -186,10 +179,9 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { } } // +1 because of the final le=+infinity bucket - assertEquals(buckets.split(",").length + 1, count); + assertEquals(buckets.split(",").length+1, count); } - @Test public void testStatusCode() throws Exception { Map sampleStatusCodes = new HashMap(); @@ -202,6 +194,7 @@ public void testStatusCode() throws Exception { String metricName = "foo"; + when(cfg.getInitParameter(MetricsFilter.REPORT_STATUS)).thenReturn("true"); when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn(metricName); when(cfg.getInitParameter(MetricsFilter.PATH_COMPONENT_PARAM)).thenReturn("4"); From 9a9726af76b5770ea8c66918cc8a875825f3ecc6 Mon Sep 17 00:00:00 2001 From: "Raphael P. Barazzutti" Date: Sat, 18 Aug 2018 11:33:52 +0200 Subject: [PATCH 5/5] separate filter (StatusAwareMetricsFilter) Signed-off-by: Raphael P. Barazzutti --- .../client/filter/MetricsFilter.java | 48 ++++-------- .../filter/StatusAwareMetricsFilter.java | 53 +++++++++++++ .../filter/MetricsFilterCommonTest.java | 36 +++++++++ .../client/filter/MetricsFilterTest.java | 51 ++----------- .../filter/StatusAwareMetricsFilterTest.java | 75 +++++++++++++++++++ 5 files changed, 185 insertions(+), 78 deletions(-) create mode 100644 simpleclient_servlet/src/main/java/io/prometheus/client/filter/StatusAwareMetricsFilter.java create mode 100644 simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterCommonTest.java create mode 100644 simpleclient_servlet/src/test/java/io/prometheus/client/filter/StatusAwareMetricsFilterTest.java diff --git a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java index 9ee60051c..e101624ae 100644 --- a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java +++ b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java @@ -31,7 +31,7 @@ * {@code * * prometheusFilter - * net.cccnext.ssp.portal.spring.filter.PrometheusMetricsFilter + * io.prometheus.client.filter.MetricsFilter * * metric-name * webapp_metrics_filter @@ -48,10 +48,6 @@ * path-components * 0 * - * - * report-status - * false - * * * } * @@ -62,7 +58,6 @@ public class MetricsFilter implements Filter { static final String HELP_PARAM = "help"; static final String METRIC_NAME_PARAM = "metric-name"; static final String BUCKET_CONFIG_PARAM = "buckets"; - static final String REPORT_STATUS = "report-status"; private Histogram histogram = null; @@ -71,7 +66,6 @@ public class MetricsFilter implements Filter { private String metricName = null; private String help = "The time taken fulfilling servlet requests"; private double[] buckets = null; - private boolean reportStatus; public MetricsFilter() { } @@ -95,7 +89,7 @@ private boolean isEmpty(String s) { return s == null || s.length() == 0; } - private String getComponents(String str) { + protected String getComponents(String str) { if (str == null || pathComponents < 1) { return str; } @@ -147,17 +141,9 @@ public void init(FilterConfig filterConfig) throws ServletException { buckets[i] = Double.parseDouble(bucketParams[i]); } } - - // Allow users to track the HTTP status of responses (disabled by default) - reportStatus = (!isEmpty(filterConfig.getInitParameter(REPORT_STATUS)) && filterConfig.getInitParameter(REPORT_STATUS).toUpperCase().equals("TRUE")); } - if (!reportStatus) - builder = builder - .labelNames("path", "method"); - else - builder = builder - .labelNames("path", "method", "status"); + addLabelsNames(builder); if (buckets != null) { builder = builder.buckets(buckets); @@ -169,33 +155,31 @@ public void init(FilterConfig filterConfig) throws ServletException { .register(); } + Histogram.Builder addLabelsNames(Histogram.Builder builder) { + return builder.labelNames("path", "method"); + } + + Histogram.Child addLabelsValues(Histogram builder, HttpServletRequest request, HttpServletResponse response) { + String path = request.getRequestURI(); + + return builder.labels(getComponents(path), request.getMethod()); + } + + @Override public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException { - if (!(servletRequest instanceof HttpServletRequest)) { + if (!(servletRequest instanceof HttpServletRequest) || !(servletResponse instanceof HttpServletResponse)) { filterChain.doFilter(servletRequest, servletResponse); return; } - HttpServletRequest request = (HttpServletRequest) servletRequest; - - String path = request.getRequestURI(); - SimpleTimer timer = new SimpleTimer(); try { filterChain.doFilter(servletRequest, servletResponse); } finally { double time = timer.elapsedSeconds(); - Histogram.Child child; - if (!reportStatus) - child = histogram - .labels(getComponents(path), request.getMethod()); - else { - String status = (servletResponse instanceof HttpServletResponse) ? - Integer.toString(((HttpServletResponse) servletResponse).getStatus()) : "undef"; - child = histogram - .labels(getComponents(path), request.getMethod(), status); - } + Histogram.Child child = addLabelsValues(histogram, (HttpServletRequest) servletRequest, (HttpServletResponse) servletResponse); child.observe(time); } diff --git a/simpleclient_servlet/src/main/java/io/prometheus/client/filter/StatusAwareMetricsFilter.java b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/StatusAwareMetricsFilter.java new file mode 100644 index 000000000..06e79be24 --- /dev/null +++ b/simpleclient_servlet/src/main/java/io/prometheus/client/filter/StatusAwareMetricsFilter.java @@ -0,0 +1,53 @@ +package io.prometheus.client.filter; + +import io.prometheus.client.Histogram; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * The StatusAwareMetricsFilter class provides an histogram metrics with the following labels : path, method, and status. + * + * This filter behave similarly than MetricsFilter with an extra label that report the status. + * + * Do not use MetricsFilter and StatusAwareMetricsFilter with the same metric name. Due to their different labels doing + * so would raise cardinality issues. + * + * {@code + * + * statusAwarePrometheusFilter + * io.prometheus.client.filter.StatusAwareMetricsFilter + * + * metric-name + * webapp_metrics_filter + * + * + * help + * The time taken fulfilling servlet requests + * + * + * buckets + * 0.005,0.01,0.025,0.05,0.075,0.1,0.25,0.5,0.75,1,2.5,5,7.5,10 + * + * + * path-components + * 0 + * + * + * } + */ +public class StatusAwareMetricsFilter extends MetricsFilter { + + @Override + Histogram.Builder addLabelsNames(Histogram.Builder builder) { + return builder.labelNames("path", "method", "status"); + } + + @Override + Histogram.Child addLabelsValues(Histogram builder, HttpServletRequest request, HttpServletResponse response) { + String path = request.getRequestURI(); + String status = Integer.toString((response).getStatus()); + return builder.labels(getComponents(path), request.getMethod(), status); + } + +} diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterCommonTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterCommonTest.java new file mode 100644 index 000000000..497a6704e --- /dev/null +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterCommonTest.java @@ -0,0 +1,36 @@ +package io.prometheus.client.filter; + +import io.prometheus.client.Collector; +import io.prometheus.client.CollectorRegistry; +import org.eclipse.jetty.http.HttpMethods; +import org.junit.After; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.util.Enumeration; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +abstract public class MetricsFilterCommonTest { + MetricsFilter f =getMetricsFilter(); + + abstract public MetricsFilter getMetricsFilter(); + + @After + public void clear() { + CollectorRegistry.defaultRegistry.clear(); + } + + +} diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java index 25f876738..3e5ac8ef1 100644 --- a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java @@ -25,8 +25,11 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -public class MetricsFilterTest { - MetricsFilter f = new MetricsFilter(); +public class MetricsFilterTest extends MetricsFilterCommonTest { + public MetricsFilter getMetricsFilter(){ + return new MetricsFilter(); + } + @After public void clear() { @@ -182,50 +185,6 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { assertEquals(buckets.split(",").length+1, count); } - @Test - public void testStatusCode() throws Exception { - Map sampleStatusCodes = new HashMap(); - sampleStatusCodes.put("/a/page/that/exists", HttpServletResponse.SC_OK); - sampleStatusCodes.put("/a/page/that/doesn-t-exist", HttpServletResponse.SC_NOT_FOUND); - sampleStatusCodes.put("/a/page/that/crashes", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - - FilterConfig cfg = mock(FilterConfig.class); - when(cfg.getInitParameter(anyString())).thenReturn(null); - - String metricName = "foo"; - - when(cfg.getInitParameter(MetricsFilter.REPORT_STATUS)).thenReturn("true"); - when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn(metricName); - when(cfg.getInitParameter(MetricsFilter.PATH_COMPONENT_PARAM)).thenReturn("4"); - f.init(cfg); - - for (String uri : sampleStatusCodes.keySet()) { - HttpServletRequest req = mock(HttpServletRequest.class); - - when(req.getRequestURI()).thenReturn(uri); - when(req.getMethod()).thenReturn(HttpMethods.GET); - - HttpServletResponse res = mock(HttpServletResponse.class); - when(res.getStatus()).thenReturn(sampleStatusCodes.get(uri)); - - FilterChain c = mock(FilterChain.class); - - f.doFilter(req, res, c); - - verify(c).doFilter(req, res); - } - - for (String uri : sampleStatusCodes.keySet()) { - - final Double sampleValue = CollectorRegistry.defaultRegistry - .getSampleValue(metricName + "_count", - new String[]{"path", "method", "status"}, - new String[]{uri, HttpMethods.GET, - Integer.toString(sampleStatusCodes.get(uri))}); - assertNotNull(sampleValue); - assertEquals(1, sampleValue, 0.0001); - } - } } diff --git a/simpleclient_servlet/src/test/java/io/prometheus/client/filter/StatusAwareMetricsFilterTest.java b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/StatusAwareMetricsFilterTest.java new file mode 100644 index 000000000..556e52316 --- /dev/null +++ b/simpleclient_servlet/src/test/java/io/prometheus/client/filter/StatusAwareMetricsFilterTest.java @@ -0,0 +1,75 @@ +package io.prometheus.client.filter; + +import io.prometheus.client.Collector; +import io.prometheus.client.CollectorRegistry; +import org.eclipse.jetty.http.HttpMethods; +import org.junit.After; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.*; + +public class StatusAwareMetricsFilterTest extends MetricsFilterCommonTest { + public MetricsFilter getMetricsFilter(){ + return new StatusAwareMetricsFilter(); + } + + @Test + public void testStatusCode() throws Exception { + Map sampleStatusCodes = new HashMap(); + sampleStatusCodes.put("/a/page/that/exists", HttpServletResponse.SC_OK); + sampleStatusCodes.put("/a/page/that/doesn-t-exist", HttpServletResponse.SC_NOT_FOUND); + sampleStatusCodes.put("/a/page/that/crashes", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + + FilterConfig cfg = mock(FilterConfig.class); + when(cfg.getInitParameter(anyString())).thenReturn(null); + + String metricName = "foo"; + + when(cfg.getInitParameter(MetricsFilter.METRIC_NAME_PARAM)).thenReturn(metricName); + when(cfg.getInitParameter(MetricsFilter.PATH_COMPONENT_PARAM)).thenReturn("4"); + + f.init(cfg); + + for (String uri : sampleStatusCodes.keySet()) { + HttpServletRequest req = mock(HttpServletRequest.class); + + when(req.getRequestURI()).thenReturn(uri); + when(req.getMethod()).thenReturn(HttpMethods.GET); + + HttpServletResponse res = mock(HttpServletResponse.class); + when(res.getStatus()).thenReturn(sampleStatusCodes.get(uri)); + + FilterChain c = mock(FilterChain.class); + + f.doFilter(req, res, c); + + verify(c).doFilter(req, res); + } + + for (String uri : sampleStatusCodes.keySet()) { + + final Double sampleValue = CollectorRegistry.defaultRegistry + .getSampleValue(metricName + "_count", + new String[]{"path", "method", "status"}, + new String[]{uri, HttpMethods.GET, + Integer.toString(sampleStatusCodes.get(uri))}); + assertNotNull(sampleValue); + assertEquals(1, sampleValue, 0.0001); + } + } + +}