diff options
author | Bjørn Christian Seime <bjorncs@yahooinc.com> | 2023-01-23 15:46:12 +0100 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@yahooinc.com> | 2023-01-23 15:46:12 +0100 |
commit | 8847d9b4eef2cd159febd04cad3ebc31cc005da6 (patch) | |
tree | 74612eb3e3913095d1bc314ec38b5882402a7c21 /container-core/src/test/java/com/yahoo/jdisc/http/server | |
parent | d485fdef78704f15fbd0f988a639622ed73356c6 (diff) |
Implement response metric aggregator as a HttpChannel.Listener
Ensure that response metrics are incremented for error responses produced by Jetty's HTTP parser.
Diffstat (limited to 'container-core/src/test/java/com/yahoo/jdisc/http/server')
-rw-r--r-- | container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java | 33 | ||||
-rw-r--r-- | container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/ResponseMetricAggregatorTest.java (renamed from container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java) | 117 |
2 files changed, 57 insertions, 93 deletions
diff --git a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java index 39b6dcdc6d5..8639680e335 100644 --- a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java +++ b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java @@ -11,6 +11,7 @@ import com.yahoo.jdisc.References; import com.yahoo.jdisc.Request; import com.yahoo.jdisc.Response; import com.yahoo.jdisc.application.BindingSetSelector; +import com.yahoo.jdisc.application.MetricConsumer; import com.yahoo.jdisc.handler.AbstractRequestHandler; import com.yahoo.jdisc.handler.CompletionHandler; import com.yahoo.jdisc.handler.ContentChannel; @@ -38,7 +39,6 @@ import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient; import org.apache.hc.core5.http.ConnectionClosedException; import org.apache.hc.core5.http.ContentType; import org.assertj.core.api.Assertions; -import org.eclipse.jetty.server.handler.AbstractHandlerContainer; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -65,6 +65,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; +import static com.yahoo.jdisc.Response.Status.BAD_REQUEST; import static com.yahoo.jdisc.Response.Status.GATEWAY_TIMEOUT; import static com.yahoo.jdisc.Response.Status.INTERNAL_SERVER_ERROR; import static com.yahoo.jdisc.Response.Status.NOT_FOUND; @@ -161,6 +162,26 @@ public class HttpServerTest { } @Test + void requireThatMultipleHostHeadersReturns400() throws Exception { + var metricConsumer = new MetricConsumerMock(); + JettyTestDriver driver = JettyTestDriver.newConfiguredInstance( + mockRequestHandler(), + new ServerConfig.Builder(), + new ConnectorConfig.Builder(), + binder -> binder.bind(MetricConsumer.class).toInstance(metricConsumer.mockitoMock())); + driver.client() + .newGet("/status.html").addHeader("Host", "localhost").addHeader("Host", "vespa.ai").execute() + .expectStatusCode(is(BAD_REQUEST)).expectContent(containsString("Bad Host: multiple headers")); + assertTrue(driver.close()); + var aggregator = ResponseMetricAggregator.getBean(driver.server()); + var metrics = aggregator.takeStatistics(); + long badRequestResponses = metrics.stream() + .filter(m -> m.dimensions.statusCode == 400 && m.dimensions.method.equals("GET")) + .count(); + assertEquals(1, badRequestResponses, metrics::toString); + } + + @Test void requireThatAccessLogIsCalledForRequestRejectedByJetty() throws Exception { BlockingQueueRequestLog requestLogMock = new BlockingQueueRequestLog(); final JettyTestDriver driver = JettyTestDriver.newConfiguredInstance( @@ -584,11 +605,9 @@ public class HttpServerTest { void requireThatResponseStatsAreCollected() throws Exception { RequestTypeHandler handler = new RequestTypeHandler(); JettyTestDriver driver = JettyTestDriver.newInstance(handler); - HttpResponseStatisticsCollector statisticsCollector = ((AbstractHandlerContainer) driver.server().server().getHandler()) - .getChildHandlerByClass(HttpResponseStatisticsCollector.class); - + var statisticsCollector = ResponseMetricAggregator.getBean(driver.server());; { - List<HttpResponseStatisticsCollector.StatisticsEntry> stats = statisticsCollector.takeStatistics(); + List<ResponseMetricAggregator.StatisticsEntry> stats = statisticsCollector.takeStatistics(); assertEquals(0, stats.size()); } @@ -622,9 +641,9 @@ public class HttpServerTest { assertTrue(driver.close()); } - private HttpResponseStatisticsCollector.StatisticsEntry waitForStatistics(HttpResponseStatisticsCollector + private ResponseMetricAggregator.StatisticsEntry waitForStatistics(ResponseMetricAggregator statisticsCollector) { - List<HttpResponseStatisticsCollector.StatisticsEntry> entries = Collections.emptyList(); + List<ResponseMetricAggregator.StatisticsEntry> entries = Collections.emptyList(); int tries = 0; while (entries.isEmpty() && tries < 10000) { entries = statisticsCollector.takeStatistics(); diff --git a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/ResponseMetricAggregatorTest.java index 502702ccf35..e80b6b777db 100644 --- a/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java +++ b/container-core/src/test/java/com/yahoo/jdisc/http/server/jetty/ResponseMetricAggregatorTest.java @@ -1,47 +1,37 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.http.server.jetty; -import com.yahoo.jdisc.http.server.jetty.HttpResponseStatisticsCollector.StatisticsEntry; -import jakarta.servlet.ServletException; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; +import com.yahoo.jdisc.http.server.jetty.ResponseMetricAggregator.StatisticsEntry; import org.eclipse.jetty.http.HttpFields; -import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; -import org.eclipse.jetty.http.MetaData.Response; -import org.eclipse.jetty.server.AbstractConnector; -import org.eclipse.jetty.server.Connector; -import org.eclipse.jetty.server.HttpChannel; -import org.eclipse.jetty.server.HttpChannelOverHttp; -import org.eclipse.jetty.server.HttpConfiguration; -import org.eclipse.jetty.server.HttpTransport; import org.eclipse.jetty.server.Request; -import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.server.handler.AbstractHandler; -import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.server.Response; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.io.IOException; -import java.nio.ByteBuffer; import java.util.List; import java.util.Set; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** * @author ollivir * @author bjorncs */ -public class HttpResponseStatisticsCollectorTest { +public class ResponseMetricAggregatorTest { - private Connector connector; - private List<String> monitoringPaths = List.of("/status.html"); - private List<String> searchPaths = List.of("/search"); - private HttpResponseStatisticsCollector collector = new HttpResponseStatisticsCollector(monitoringPaths, searchPaths, Set.of()); - private int httpResponseCode = 500; + private final List<String> monitoringPaths = List.of("/status.html"); + private final List<String> searchPaths = List.of("/search"); + private final ResponseMetricAggregator collector = new ResponseMetricAggregator(monitoringPaths, searchPaths, Set.of()); + + @BeforeEach + public void initializeCollector() { + collector.takeStatistics(); + } @Test void statistics_are_aggregated_by_category() { @@ -78,7 +68,6 @@ public class HttpResponseStatisticsCollectorTest { } @Test - @SuppressWarnings("removal") void statistics_include_grouped_and_single_statuscodes() { testRequest("http", 401, "GET"); testRequest("http", 404, "GET"); @@ -132,49 +121,28 @@ public class HttpResponseStatisticsCollectorTest { assertStatisticsEntry(stats, "http", "GET", MetricDefinitions.RESPONSES_2XX, "write", 200, 1L); } - @BeforeEach - public void initializeCollector() throws Exception { - Server server = new Server(); - connector = new AbstractConnector(server, null, null, null, 0) { - @Override - protected void accept(int acceptorID) throws IOException, InterruptedException { - } - - @Override - public Object getTransport() { - return null; - } - }; - collector.setHandler(new AbstractHandler() { - @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) - throws IOException, ServletException { - baseRequest.setHandled(true); - baseRequest.getResponse().setStatus(httpResponseCode); - } - }); - server.setHandler(collector); - server.start(); - } - private Request testRequest(String scheme, int responseCode, String httpMethod) { - return testRequest(scheme, responseCode, httpMethod, "foo/bar"); + private void testRequest(String scheme, int responseCode, String httpMethod) { + testRequest(scheme, responseCode, httpMethod, "foo/bar"); } - private Request testRequest(String scheme, int responseCode, String httpMethod, String path) { - return testRequest(scheme, responseCode, httpMethod, path, null); + private void testRequest(String scheme, int responseCode, String httpMethod, String path) { + testRequest(scheme, responseCode, httpMethod, path, null); } - private Request testRequest(String scheme, int responseCode, String httpMethod, String path, + private void testRequest(String scheme, int responseCode, String httpMethod, String path, com.yahoo.jdisc.Request.RequestType explicitRequestType) { - HttpChannel channel = new HttpChannelOverHttp(null, connector, new HttpConfiguration(), null, new DummyTransport()); - MetaData.Request metaData = new MetaData.Request(httpMethod, HttpURI.build(scheme + "://" + path), HttpVersion.HTTP_1_1, HttpFields.build()); - Request req = channel.getRequest(); - if (explicitRequestType != null) - req.setAttribute("requestType", explicitRequestType); - req.setMetaData(metaData); - - this.httpResponseCode = responseCode; - channel.handle(); - return req; + + Response resp = mock(Response.class); + when(resp.getCommittedMetaData()) + .thenReturn(new MetaData.Response(HttpVersion.HTTP_1_1, responseCode, HttpFields.EMPTY)); + Request req = mock(Request.class); + when(req.getResponse()).thenReturn(resp); + when(req.getMethod()).thenReturn(httpMethod); + when(req.getScheme()).thenReturn(scheme); + when(req.getRequestURI()).thenReturn(path); + when(req.getAttribute(ResponseMetricAggregator.requestTypeAttribute)).thenReturn(explicitRequestType); + when(req.getProtocol()).thenReturn(HttpVersion.HTTP_1_1.asString()); + + collector.onResponseCommit(req); } private static void assertStatisticsEntry(List<StatisticsEntry> result, String scheme, String method, String name, @@ -191,27 +159,4 @@ public class HttpResponseStatisticsCollectorTest { assertThat(value, equalTo(expectedValue)); } - private final class DummyTransport implements HttpTransport { - @Override - public void send(MetaData.Request request, Response response, ByteBuffer byteBuffer, boolean b, Callback callback) { - callback.succeeded(); - } - - @Override - public boolean isPushSupported() { - return false; - } - - @Override - public void push(MetaData.Request request) { - } - - @Override - public void onCompleted() { - } - - @Override - public void abort(Throwable failure) { - } - } } |