Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Count HTTP statuses returned along with the HTTP response times #560

Merged
merged 3 commits into from
Jul 16, 2020
Merged

Count HTTP statuses returned along with the HTTP response times #560

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5343921
Count HTTP statuses returned along with the HTTP response times
kdombeck Jul 15, 2020
7043489
Change count metric to end in _total to match standards
kdombeck Jul 15, 2020
cdef5ee
Add status prefix to prevent collision with histogram
kdombeck Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions simpleclient_servlet/src/main/java/io/prometheus/client/filter/MetricsFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.prometheus.client.filter;

import io.prometheus.client.Counter;
import io.prometheus.client.Histogram;

import javax.servlet.Filter;
Expand All @@ -9,13 +10,14 @@
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;

/**
* The MetricsFilter class exists to provide a high-level filter that enables tunable collection of metrics for Servlet
* performance.
*
* <p>The Histogram name itself is required, and configured with a {@code metric-name} init parameter.
* <p>The metric name itself is required, and configured with a {@code metric-name} init parameter.
*
* <p>The help parameter, configured with the {@code help} init parameter, is not required but strongly recommended.
*
Expand All @@ -26,6 +28,8 @@
* <p>The Histogram buckets can be configured with a {@code buckets} init parameter whose value is a comma-separated list
* of valid {@code double} values.
*
* <p>HTTP statuses will be aggregated via Counter. The name for this counter will be derived from the {@code metric-name} init parameter.
*
* <pre>{@code
* <filter>
* <filter-name>prometheusFilter</filter-name>
Expand All @@ -34,7 +38,7 @@
* <param-name>metric-name</param-name>
* <param-value>webapp_metrics_filter</param-value>
* </init-param>
* <init-param>
* <init-param>
* <param-name>help</param-name>
* <param-value>The time taken fulfilling servlet requests</param-value>
* </init-param>
Expand All @@ -56,8 +60,10 @@ 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 UNKNOWN_HTTP_STATUS_CODE = "";

private Histogram histogram = null;
private Counter statusCounter = null;

// Package-level for testing purposes.
int pathComponents = 1;
Expand Down Expand Up @@ -149,6 +155,10 @@ public void init(FilterConfig filterConfig) throws ServletException {
.help(help)
.name(metricName)
.register();

statusCounter = Counter.build(metricName + "_status", "HTTP status codes of " + help)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counters should end in _total

Having a label name in a metric name is generally an anti-pattern, as it won't make sense if that label is aggregated away. It may be okay here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove status from the metric name. It will now look like this

# HELP http_request_status HTTP status codes of The time taken fulfilling servlet requests
# TYPE http_request_status counter
http_request_total{path="/my/api/1",method="GET",status="200",} 4.0
http_request_total{path="/my/api/2",method="GET",status="500",} 2.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd end up kinda clashing with the histogram though, which isn't desirable.

Copy link
Contributor Author

@kdombeck kdombeck Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added status back in. It will now look like this.

# HELP http_request_status HTTP status codes of The time taken fulfilling servlet requests
# TYPE http_request_status counter
http_request_status_total{path="/my/api/1",method="GET",status="200",} 4.0
http_request_status_total{path="/my/api/2",method="GET",status="500",} 2.0

I took your "may be okay here though" as a sign that you wanted it removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do want it removed, but we can't for other reasons :)

.labelNames("path", "method", "status")
.register();
}

@Override
Expand All @@ -162,17 +172,28 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo

String path = request.getRequestURI();

String components = getComponents(path);
String method = request.getMethod();
Histogram.Timer timer = histogram
.labels(getComponents(path), request.getMethod())
.labels(components, method)
.startTimer();

try {
filterChain.doFilter(servletRequest, servletResponse);
} finally {
timer.observeDuration();
statusCounter.labels(components, method, getStatusCode(servletResponse)).inc();
}
}

private String getStatusCode(ServletResponse servletResponse) {
if (!(servletResponse instanceof HttpServletResponse)) {
return UNKNOWN_HTTP_STATUS_CODE;
}

return Integer.toString(((HttpServletResponse) servletResponse).getStatus());
}

@Override
public void destroy() {
}
Expand Down
51 changes: 51 additions & 0 deletions simpleclient_servlet/src/test/java/io/prometheus/client/filter/MetricsFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.util.Enumeration;
Expand Down Expand Up @@ -180,4 +181,54 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
assertEquals(buckets.split(",").length+1, count);
}

@Test
public void testStatusCode() throws Exception {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getRequestURI()).thenReturn("/foo/bar/baz/bang");
when(req.getMethod()).thenReturn(HttpMethods.GET);

HttpServletResponse res = mock(HttpServletResponse.class);
when(res.getStatus()).thenReturn(200);

FilterChain c = mock(FilterChain.class);

MetricsFilter constructed = new MetricsFilter(
"foobar_filter",
"Help for my filter",
2,
null
);
constructed.init(mock(FilterConfig.class));

constructed.doFilter(req, res, c);

final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue("foobar_filter_status", new String[]{"path", "method", "status"}, new String[]{"/foo/bar", HttpMethods.GET, "200"});
assertNotNull(sampleValue);
assertEquals(1, sampleValue, 0.0001);
}

@Test
public void testStatusCodeWithNonHttpServletResponse() throws Exception {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getRequestURI()).thenReturn("/foo/bar/baz/bang");
when(req.getMethod()).thenReturn(HttpMethods.GET);

ServletResponse res = mock(ServletResponse.class);

FilterChain c = mock(FilterChain.class);

MetricsFilter constructed = new MetricsFilter(
"foobar_filter",
"Help for my filter",
2,
null
);
constructed.init(mock(FilterConfig.class));

constructed.doFilter(req, res, c);

final Double sampleValue = CollectorRegistry.defaultRegistry.getSampleValue("foobar_filter_status", new String[]{"path", "method", "status"}, new String[]{"/foo/bar", HttpMethods.GET, MetricsFilter.UNKNOWN_HTTP_STATUS_CODE});
assertNotNull(sampleValue);
assertEquals(1, sampleValue, 0.0001);
}
}