diff options
15 files changed, 189 insertions, 127 deletions
diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/HttpResponse.java b/container-core/src/main/java/com/yahoo/container/jdisc/HttpResponse.java index dd03d72d97d..a6042c541c0 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/HttpResponse.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/HttpResponse.java @@ -27,8 +27,6 @@ public abstract class HttpResponse { private final Response parentResponse; - private Request.RequestType requestType; - /** * Creates a new HTTP response * @@ -126,12 +124,12 @@ public abstract class HttpResponse { } /** Sets the type classification of this request for metric collection purposes */ - public void setRequestType(Request.RequestType requestType) { this.requestType = requestType; } + public void setRequestType(Request.RequestType requestType) { parentResponse.setRequestType(requestType); } /** * Returns the type classification of this request for metric collection purposes, or null if not set. * When not set, the request type will be "read" for GET requests and "write" for other request methods. */ - public Request.RequestType getRequestType() { return requestType; } + public Request.RequestType getRequestType() { return parentResponse.getRequestType(); } } diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java index ac1aa533201..3a99ee7d0c6 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java @@ -78,7 +78,6 @@ public abstract class ThreadedHttpRequestHandler extends ThreadedRequestHandler try { channel = new LazyContentChannel(httpRequest, responseHandler, metric, log); HttpResponse httpResponse = handle(httpRequest, channel); - request.setRequestType(httpResponse.getRequestType()); channel.setHttpResponse(httpResponse); // may or may not have already been done render(httpRequest, httpResponse, channel, jdiscRequest.creationTime(TimeUnit.MILLISECONDS)); } catch (Exception e) { diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java b/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java index bce6f72c1fc..895ddb30a6d 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java @@ -48,7 +48,6 @@ public class Request extends AbstractResource { private boolean serverRequest; private Long timeout; private URI uri; - private RequestType requestType; public enum RequestType { READ, WRITE, MONITORING @@ -331,12 +330,6 @@ public class Request extends AbstractResource { return unit.convert(creationTime, TimeUnit.MILLISECONDS); } - /** Sets the type classification of this request for metric collection purposes */ - public void setRequestType(RequestType requestType) { this.requestType = requestType; } - - /** Returns the type classification of this request for metric collection purposes, or null if not set */ - public RequestType getRequestType() { return requestType; } - /** * <p>Returns whether or not this Request has been cancelled. This can be thought of as the {@link * Thread#isInterrupted()} of Requests - it does not enforce anything in ways of blocking the Request, it is simply diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/Response.java b/jdisc_core/src/main/java/com/yahoo/jdisc/Response.java index ec9d2e0df84..c3d07a70e14 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/Response.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/Response.java @@ -102,6 +102,7 @@ public class Response { private final HeaderFields headers = new HeaderFields(); private Throwable error; private int status; + private Request.RequestType requestType; /** * Creates a new instance of this class. @@ -205,6 +206,12 @@ public class Response { return this; } + /** Sets the type classification of this request for metric collection purposes */ + public void setRequestType(Request.RequestType requestType) { this.requestType = requestType; } + + /** Returns the type classification of this request for metric collection purposes, or null if not set */ + public Request.RequestType getRequestType() { return requestType; } + /** * This is a convenience method for creating a Response with status {@link Status#GATEWAY_TIMEOUT} and passing * that to the given {@link ResponseHandler#handleResponse(Response)}. For trivial implementations of {@link diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/ServletFilterRequest.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/ServletFilterRequest.java index 3dccb09c971..f06f9e256ff 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/ServletFilterRequest.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/ServletFilterRequest.java @@ -18,8 +18,6 @@ import java.util.Set; /** * Servlet implementation for JDisc filter requests. - * - * @since 5.27 */ class ServletFilterRequest extends DiscFilterRequest { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java index 53d775d4349..7085f07585a 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java @@ -44,12 +44,12 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog private final AccessLog accessLog; - public AccessLogRequestLog(final AccessLog accessLog) { + public AccessLogRequestLog(AccessLog accessLog) { this.accessLog = accessLog; } @Override - public void log(final Request request, final Response response) { + public void log(Request request, Response response) { try { AccessLogEntry accessLogEntry = Optional.ofNullable(request.getAttribute(JDiscHttpServlet.ATTRIBUTE_NAME_ACCESS_LOG_ENTRY)) .map(AccessLogEntry.class::cast) @@ -100,8 +100,8 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog accessLogEntry.addKeyValue("cipher-suite", cipherSuite); } - final long startTime = request.getTimeStamp(); - final long endTime = System.currentTimeMillis(); + long startTime = request.getTimeStamp(); + long endTime = System.currentTimeMillis(); accessLogEntry.setTimeStamp(startTime); accessLogEntry.setDurationBetweenRequestResponse(endTime - startTime); accessLogEntry.setReturnedContentSize(response.getHttpChannel().getBytesWritten()); @@ -121,7 +121,7 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog } } - private static String getRemoteAddress(final HttpServletRequest request) { + private static String getRemoteAddress(HttpServletRequest request) { return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_FOR)) .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RA))) .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_YAHOOREMOTEIP))) @@ -129,7 +129,7 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog .orElseGet(request::getRemoteAddr); } - private static int getRemotePort(final HttpServletRequest request) { + private static int getRemotePort(HttpServletRequest request) { return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_PORT)) .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RP))) .flatMap(AccessLogRequestLog::parsePort) @@ -143,4 +143,5 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog return Optional.empty(); } } + } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java index 81577561c5b..cc7ed7ac3e0 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java @@ -175,9 +175,6 @@ class HttpRequestDispatch { try (ResourceReference ref = References.fromResource(jdiscRequest)) { HttpRequestFactory.copyHeaders(jettyRequest, jdiscRequest); requestContentChannel = requestHandler.handleRequest(jdiscRequest, servletResponseController.responseHandler); - if (jdiscRequest.getRequestType() != null) - jettyRequest.setAttribute(HttpResponseStatisticsCollector.requestTypeAttribute, - jdiscRequest.getRequestType()); } ServletInputStream servletInputStream = jettyRequest.getInputStream(); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java index d2ac0cb7f6a..31a8303ab4b 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java @@ -287,6 +287,15 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G this.value = value; } + @Override + public String toString() { + return "scheme: " + scheme + + ", method: " + method + + ", name: " + name + + ", requestType: " + requestType + + ", value: " + value; + } + } } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java index ba477f9d32f..b050a9a6d1c 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java @@ -338,6 +338,8 @@ public class JettyHttpServer extends AbstractServerProvider { return ((ServerConnector)server.getConnectors()[0]).getLocalPort(); } + Server server() { return server; } + private class MetricTask implements Runnable { @Override public void run() { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java index 0188e7c2f09..5dd6b72dc20 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java @@ -164,6 +164,7 @@ public class ServletResponseController { private void setResponse(Response jdiscResponse) { synchronized (monitor) { + servletRequest.setAttribute(HttpResponseStatisticsCollector.requestTypeAttribute, jdiscResponse.getRequestType()); if (responseCommitted) { log.log(Level.FINE, jdiscResponse.getError(), diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/HttpRequestTestCase.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/HttpRequestTestCase.java index eeae1fa74bc..a3cb31d5ecb 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/HttpRequestTestCase.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/HttpRequestTestCase.java @@ -28,7 +28,7 @@ public class HttpRequestTestCase { @Test public void requireThatSimpleServerConstructorsUseReasonableDefaults() { - final URI uri = URI.create("http://localhost/"); + URI uri = URI.create("http://localhost/"); HttpRequest request = HttpRequest.newServerRequest(mockContainer(), uri); assertTrue(request.isServerRequest()); assertEquals(uri, request.getUri()); @@ -50,9 +50,9 @@ public class HttpRequestTestCase { @Test public void requireThatSimpleClientConstructorsUseReasonableDefaults() { - final Request parent = new Request(mockContainer(), URI.create("http://localhost/")); + Request parent = new Request(mockContainer(), URI.create("http://localhost/")); - final URI uri = URI.create("http://remotehost/"); + URI uri = URI.create("http://remotehost/"); HttpRequest request = HttpRequest.newClientRequest(parent, uri); assertFalse(request.isServerRequest()); assertEquals(uri, request.getUri()); diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java index a67f6919727..b6bd017a6d9 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java @@ -40,6 +40,7 @@ import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.ProxyProtocolClientConnectionFactory.V1; import org.eclipse.jetty.client.ProxyProtocolClientConnectionFactory.V2; import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.server.handler.AbstractHandlerContainer; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.junit.Rule; import org.junit.Test; @@ -97,13 +98,12 @@ import static org.cthul.matchers.CthulMatchers.matchesPattern; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasSize; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.Mockito.mock; @@ -125,8 +125,8 @@ public class HttpServerTest { @Test public void requireThatServerCanListenToRandomPort() throws Exception { final TestDriver driver = TestDrivers.newInstance(mockRequestHandler()); - assertThat(driver.server().getListenPort(), is(not(0))); - assertThat(driver.close(), is(true)); + assertNotEquals(0, driver.server().getListenPort()); + assertTrue(driver.close()); } @Test @@ -142,7 +142,7 @@ public class HttpServerTest { } catch (final Throwable t) { assertThat(t.getCause(), instanceOf(BindException.class)); } - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -159,7 +159,7 @@ public class HttpServerTest { Pattern.quote(BindingSetNotFoundException.class.getName()) + ": No binding set named 'unknown'\\.\n\tat .+", Pattern.DOTALL | Pattern.MULTILINE))); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -171,7 +171,7 @@ public class HttpServerTest { .requestHeaderSize(1)); driver.client().get("/status.html") .expectStatusCode(is(REQUEST_URI_TOO_LONG)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -188,7 +188,7 @@ public class HttpServerTest { assertThat(accessLogMock.logEntries.size(), equalTo(1)); AccessLogEntry accessLogEntry = accessLogMock.logEntries.get(0); - assertThat(accessLogEntry.getStatusCode(), equalTo(414)); + assertEquals(414, accessLogEntry.getStatusCode()); } private static class AccessLogMock extends AccessLog { @@ -208,7 +208,7 @@ public class HttpServerTest { final TestDriver driver = TestDrivers.newInstance(new EchoRequestHandler()); driver.client().get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -217,7 +217,7 @@ public class HttpServerTest { SimpleHttpClient client = driver.newClient(true); client.get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -227,7 +227,7 @@ public class HttpServerTest { .expectStatusCode(is(OK)); driver.client().get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -241,7 +241,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith('{' + requestContent + "=[]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -254,7 +254,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{foo=[bar]}foo=bar")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -267,7 +267,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{foo=[bar]}foo=bar")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -280,7 +280,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{foo=[bar]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -295,7 +295,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith('{' + requestContent + "=[]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -307,7 +307,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -320,7 +320,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith("{a=[b], c=[d]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -332,7 +332,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{a=[b], c=[d]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -345,7 +345,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith("{a=[b], c=[d1, d2], e=[f]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -358,7 +358,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(is("{B\u00e6r=[bl\u00e5]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -370,7 +370,7 @@ public class HttpServerTest { .setContent("a=b") .execute(); response.expectStatusCode(is(UNSUPPORTED_MEDIA_TYPE)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -383,7 +383,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith("{ =\u00d8=[\"% ]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -395,7 +395,7 @@ public class HttpServerTest { .setContent("a=b") .execute(); response.expectStatusCode(is(INTERNAL_SERVER_ERROR)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -426,7 +426,7 @@ public class HttpServerTest { .execute(); response.expectStatusCode(is(OK)) .expectContent(containsString("[foo=bar]")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -441,7 +441,7 @@ public class HttpServerTest { .expectStatusCode(is(OK)) .expectHeader("Set-Cookie", is("foo=bar; Path=/foopath; Domain=.localhost; Secure; HttpOnly")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -451,7 +451,7 @@ public class HttpServerTest { driver.client().get("/status.html") .expectStatusCode(is(GATEWAY_TIMEOUT)); ResponseDispatch.newInstance(OK).dispatch(requestHandler.responseHandler); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } // Header with no value is disallowed by https://tools.ietf.org/html/rfc7230#section-3.2 @@ -462,7 +462,7 @@ public class HttpServerTest { driver.client().get("/status.html") .expectStatusCode(is(OK)) .expectNoHeader("X-Foo"); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } // Header with empty value is allowed by https://tools.ietf.org/html/rfc7230#section-3.2 @@ -473,7 +473,7 @@ public class HttpServerTest { driver.client().get("/status.html") .expectStatusCode(is(OK)) .expectHeader("X-Foo", is("")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -498,7 +498,7 @@ public class HttpServerTest { driver.client().get("/status.html") .expectStatusCode(is(OK)) .expectHeader(CONNECTION, is(CLOSE)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -510,7 +510,7 @@ public class HttpServerTest { final TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile, TlsClientAuth.WANT); driver.client().get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -528,7 +528,7 @@ public class HttpServerTest { .get("/dummy.html") .expectStatusCode(is(UNAUTHORIZED)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -546,7 +546,7 @@ public class HttpServerTest { .get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -560,21 +560,65 @@ public class HttpServerTest { @Test public void requireThatGzipEncodingRequestsAreAutomaticallyDecompressed() throws Exception { - final TestDriver driver = TestDrivers.newInstance(new ParameterPrinterRequestHandler()); - final String requestContent = generateContent('a', 30); - final ResponseValidator response = - driver.client().newPost("/status.html") - .addHeader(CONTENT_TYPE, APPLICATION_X_WWW_FORM_URLENCODED) - .setGzipContent(requestContent) - .execute(); + TestDriver driver = TestDrivers.newInstance(new ParameterPrinterRequestHandler()); + String requestContent = generateContent('a', 30); + ResponseValidator response = driver.client().newPost("/status.html") + .addHeader(CONTENT_TYPE, APPLICATION_X_WWW_FORM_URLENCODED) + .setGzipContent(requestContent) + .execute(); response.expectStatusCode(is(OK)) .expectContent(startsWith('{' + requestContent + "=[]}")); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); + } + + @Test + public void requireThatResponseStatsAreCollected() throws Exception { + RequestTypeHandler handler = new RequestTypeHandler(); + TestDriver driver = TestDrivers.newInstance(handler); + HttpResponseStatisticsCollector statisticsCollector = ((AbstractHandlerContainer) driver.server().server().getHandler()) + .getChildHandlerByClass(HttpResponseStatisticsCollector.class); + + { + List<HttpResponseStatisticsCollector.StatisticsEntry> stats = statisticsCollector.takeStatistics(); + assertEquals(0, stats.size()); + } + + { + driver.client().newPost("/status.html").execute(); + List<HttpResponseStatisticsCollector.StatisticsEntry> stats = statisticsCollector.takeStatistics(); + assertEquals(1, stats.size()); + assertEquals("http", stats.get(0).scheme); + assertEquals("POST", stats.get(0).method); + assertEquals("http.status.2xx", stats.get(0).name); + assertEquals("write", stats.get(0).requestType); + assertEquals(1, stats.get(0).value); + } + + { + driver.client().newGet("/status.html").execute(); + List<HttpResponseStatisticsCollector.StatisticsEntry> stats = statisticsCollector.takeStatistics(); + assertEquals(1, stats.size()); + assertEquals("http", stats.get(0).scheme); + assertEquals("GET", stats.get(0).method); + assertEquals("http.status.2xx", stats.get(0).name); + assertEquals("read", stats.get(0).requestType); + assertEquals(1, stats.get(0).value); + } + + { + handler.setRequestType(Request.RequestType.READ); + driver.client().newPost("/status.html").execute(); + List<HttpResponseStatisticsCollector.StatisticsEntry> stats = statisticsCollector.takeStatistics(); + assertEquals(1, stats.size()); + assertEquals("Handler overrides request type", "read", stats.get(0).requestType); + } + + assertTrue(driver.close()); } @Test public void requireThatConnectionThrottleDoesNotBlockConnectionsBelowThreshold() throws Exception { - final TestDriver driver = TestDrivers.newConfiguredInstance( + TestDriver driver = TestDrivers.newConfiguredInstance( new EchoRequestHandler(), new ServerConfig.Builder(), new ConnectorConfig.Builder() @@ -585,7 +629,7 @@ public class HttpServerTest { .maxConnections(10))); driver.client().get("/status.html") .expectStatusCode(is(OK)); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -603,7 +647,7 @@ public class HttpServerTest { driver, clientCtx, null, null, "Received fatal alert: bad_certificate"); verify(metricConsumer.mockitoMock()) .add(Metrics.SSL_HANDSHAKE_FAILURE_MISSING_CLIENT_CERT, 1L, MetricConsumerMock.STATIC_CONTEXT); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -623,7 +667,7 @@ public class HttpServerTest { driver, clientCtx, "TLSv1.3", null, "Received fatal alert: protocol_version"); verify(metricConsumer.mockitoMock()) .add(Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_PROTOCOLS, 1L, MetricConsumerMock.STATIC_CONTEXT); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -643,7 +687,7 @@ public class HttpServerTest { driver, clientCtx, null, "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", "Received fatal alert: handshake_failure"); verify(metricConsumer.mockitoMock()) .add(Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_CIPHERS, 1L, MetricConsumerMock.STATIC_CONTEXT); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -667,7 +711,7 @@ public class HttpServerTest { driver, clientCtx, null, null, "Received fatal alert: certificate_unknown"); verify(metricConsumer.mockitoMock()) .add(Metrics.SSL_HANDSHAKE_FAILURE_INVALID_CLIENT_CERT, 1L, MetricConsumerMock.STATIC_CONTEXT); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -690,7 +734,7 @@ public class HttpServerTest { driver, clientCtx, null, null, "Received fatal alert: certificate_unknown"); verify(metricConsumer.mockitoMock()) .add(Metrics.SSL_HANDSHAKE_FAILURE_EXPIRED_CLIENT_CERT, 1L, MetricConsumerMock.STATIC_CONTEXT); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); } @Test @@ -707,9 +751,9 @@ public class HttpServerTest { sendJettyClientRequest(driver, client, new V1.Tag(proxiedRemoteAddress, proxiedRemotePort)); sendJettyClientRequest(driver, client, new V2.Tag(proxiedRemoteAddress, proxiedRemotePort)); client.stop(); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); - assertThat(accessLogMock.logEntries, hasSize(2)); + assertEquals(2, accessLogMock.logEntries.size()); assertLogEntryHasRemote(accessLogMock.logEntries.get(0), proxiedRemoteAddress, proxiedRemotePort); assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, proxiedRemotePort); } @@ -727,9 +771,9 @@ public class HttpServerTest { sendJettyClientRequest(driver, client, null); sendJettyClientRequest(driver, client, new V2.Tag(proxiedRemoteAddress, 12345)); client.stop(); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); - assertThat(accessLogMock.logEntries, hasSize(2)); + assertEquals(2, accessLogMock.logEntries.size()); assertLogEntryHasRemote(accessLogMock.logEntries.get(0), "127.0.0.1", 0); assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, 0); } @@ -751,7 +795,7 @@ public class HttpServerTest { proxiedRemoteAddress, proxiedRemotePort, proxyLocalAddress, proxyLocalPort, null); ContentResponse response = sendJettyClientRequest(driver, client, v2Tag); client.stop(); - assertThat(driver.close(), is(true)); + assertTrue(driver.close()); int clientPort = Integer.parseInt(response.getHeaders().get("Jdisc-Local-Port")); assertNotEquals(proxyLocalPort, clientPort); @@ -879,8 +923,8 @@ public class HttpServerTest { return ret.toString(); } - private static TestDriver newDriverWithFormPostContentRemoved( - final RequestHandler requestHandler, final boolean removeFormPostBody) throws Exception { + private static TestDriver newDriverWithFormPostContentRemoved(RequestHandler requestHandler, + boolean removeFormPostBody) throws Exception { return TestDrivers.newConfiguredInstance( requestHandler, new ServerConfig.Builder() @@ -888,8 +932,7 @@ public class HttpServerTest { new ConnectorConfig.Builder()); } - private static FormBodyPart newFileBody(final String parameterName, final String fileName, final String fileContent) - throws Exception { + private static FormBodyPart newFileBody(final String parameterName, final String fileName, final String fileContent) { return new FormBodyPart( parameterName, new StringBody(fileContent, ContentType.TEXT_PLAIN) { @@ -959,23 +1002,37 @@ public class HttpServerTest { } private static class ParameterPrinterRequestHandler extends AbstractRequestHandler { + private static final CompletionHandler NULL_COMPLETION_HANDLER = null; @Override - public ContentChannel handleRequest(final Request request, final ResponseHandler handler) { - final Map<String, List<String>> parameters = - new TreeMap<>(((HttpRequest)request).parameters()); - final ContentChannel responseContentChannel - = ResponseDispatch.newInstance(Response.Status.OK).connect(handler); - responseContentChannel.write( - ByteBuffer.wrap(parameters.toString().getBytes(StandardCharsets.UTF_8)), - NULL_COMPLETION_HANDLER); + public ContentChannel handleRequest(Request request, ResponseHandler handler) { + Map<String, List<String>> parameters = new TreeMap<>(((HttpRequest)request).parameters()); + ContentChannel responseContentChannel = ResponseDispatch.newInstance(Response.Status.OK).connect(handler); + responseContentChannel.write(ByteBuffer.wrap(parameters.toString().getBytes(StandardCharsets.UTF_8)), + NULL_COMPLETION_HANDLER); // Have the request content written back to the response. return responseContentChannel; } } + private static class RequestTypeHandler extends AbstractRequestHandler { + + private Request.RequestType requestType = null; + + public void setRequestType(Request.RequestType requestType) { + this.requestType = requestType; + } + + @Override + public ContentChannel handleRequest(Request request, ResponseHandler handler) { + Response response = new Response(OK); + response.setRequestType(requestType); + return handler.handleResponse(response); + } + } + private static class ThrowingHandler extends AbstractRequestHandler { @Override public ContentChannel handleRequest(final Request request, final ResponseHandler handler) { diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java index 8035734a76c..f1d710bd10f 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. 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.Request; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; @@ -36,6 +37,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertNotNull; /** * A simple http client for testing @@ -87,23 +89,23 @@ public class SimpleHttpClient implements AutoCloseable { return URI.create(scheme + "://localhost:" + listenPort + path); } - public RequestExecutor newGet(final String path) { + public RequestExecutor newGet(String path) { return newRequest(new HttpGet(newUri(path))); } - public RequestExecutor newPost(final String path) { + public RequestExecutor newPost(String path) { return newRequest(new HttpPost(newUri(path))); } - public RequestExecutor newRequest(final HttpUriRequest request) { + public RequestExecutor newRequest(HttpUriRequest request) { return new RequestExecutor().setRequest(request); } - public ResponseValidator execute(final HttpUriRequest request) throws IOException { + public ResponseValidator execute(HttpUriRequest request) throws IOException { return newRequest(request).execute(); } - public ResponseValidator get(final String path) throws IOException { + public ResponseValidator get(String path) throws IOException { return newGet(path).execute(); } @@ -164,37 +166,37 @@ public class SimpleHttpClient implements AutoCloseable { private final HttpResponse response; private final String content; - public ResponseValidator(final HttpResponse response) throws IOException { + public ResponseValidator(HttpResponse response) throws IOException { this.response = response; - final HttpEntity entity = response.getEntity(); - this.content = entity == null ? null : - EntityUtils.toString(entity, StandardCharsets.UTF_8); + HttpEntity entity = response.getEntity(); + this.content = entity == null ? null : EntityUtils.toString(entity, StandardCharsets.UTF_8); } - public ResponseValidator expectStatusCode(final Matcher<Integer> matcher) { + public ResponseValidator expectStatusCode(Matcher<Integer> matcher) { MatcherAssert.assertThat(response.getStatusLine().getStatusCode(), matcher); return this; } - public ResponseValidator expectHeader(final String headerName, final Matcher<String> matcher) { - final Header firstHeader = response.getFirstHeader(headerName); - final String headerValue = firstHeader != null ? firstHeader.getValue() : null; + public ResponseValidator expectHeader(String headerName, Matcher<String> matcher) { + Header firstHeader = response.getFirstHeader(headerName); + String headerValue = firstHeader != null ? firstHeader.getValue() : null; MatcherAssert.assertThat(headerValue, matcher); - assertThat(firstHeader, is(not(nullValue()))); + assertNotNull(firstHeader); return this; } - public ResponseValidator expectNoHeader(final String headerName) { - final Header firstHeader = response.getFirstHeader(headerName); + public ResponseValidator expectNoHeader(String headerName) { + Header firstHeader = response.getFirstHeader(headerName); assertThat(firstHeader, is(nullValue())); return this; } - public ResponseValidator expectContent(final Matcher<String> matcher) throws IOException { + public ResponseValidator expectContent(final Matcher<String> matcher) { MatcherAssert.assertThat(content, matcher); return this; } } + } diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java index ed571c6b07a..913b22bc0e3 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java @@ -24,8 +24,7 @@ public class TestDriver { private final JettyHttpServer server; private final SimpleHttpClient client; - private TestDriver(com.yahoo.jdisc.test.TestDriver driver, JettyHttpServer server, SimpleHttpClient client) - throws IOException { + private TestDriver(com.yahoo.jdisc.test.TestDriver driver, JettyHttpServer server, SimpleHttpClient client) { this.driver = driver; this.server = server; this.client = client; @@ -33,7 +32,7 @@ public class TestDriver { public static TestDriver newInstance(Class<? extends JettyHttpServer> serverClass, RequestHandler requestHandler, - Module testConfig) throws IOException { + Module testConfig) { com.yahoo.jdisc.test.TestDriver driver = com.yahoo.jdisc.test.TestDriver.newSimpleApplicationInstance(testConfig); ContainerBuilder builder = driver.newContainerBuilder(); @@ -47,7 +46,7 @@ public class TestDriver { return new TestDriver(driver, server, client); } - public boolean close() throws IOException { + public boolean close() { server.close(); server.release(); return driver.close(); @@ -65,7 +64,7 @@ public class TestDriver { return newSslContext(driver.newContainerBuilder()); } - private static SSLContext newSslContext(final ContainerBuilder builder) { + private static SSLContext newSslContext(ContainerBuilder builder) { ConnectorConfig.Ssl sslConfig = builder.getInstance(ConnectorConfig.class).ssl(); if (!sslConfig.enabled()) return null; diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java index 4908da2ba75..255e42fb886 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java @@ -24,18 +24,17 @@ import java.nio.file.Path; */ public class TestDrivers { - public static TestDriver newConfiguredInstance(final RequestHandler requestHandler, - final ServerConfig.Builder serverConfig, - final ConnectorConfig.Builder connectorConfig, - final Module... guiceModules) throws IOException { + public static TestDriver newConfiguredInstance(RequestHandler requestHandler, + ServerConfig.Builder serverConfig, + ConnectorConfig.Builder connectorConfig, + Module... guiceModules) throws IOException { return TestDriver.newInstance( JettyHttpServer.class, requestHandler, newConfigModule(serverConfig, connectorConfig, guiceModules)); } - public static TestDriver newInstance(final RequestHandler requestHandler, - final Module... guiceModules) throws IOException { + public static TestDriver newInstance(RequestHandler requestHandler, Module... guiceModules) throws IOException { return TestDriver.newInstance( JettyHttpServer.class, requestHandler, @@ -48,11 +47,11 @@ public class TestDrivers { public enum TlsClientAuth { NEED, WANT } - public static TestDriver newInstanceWithSsl(final RequestHandler requestHandler, + public static TestDriver newInstanceWithSsl(RequestHandler requestHandler, Path certificateFile, Path privateKeyFile, TlsClientAuth tlsClientAuth, - final Module... guiceModules) throws IOException { + Module... guiceModules) throws IOException { return TestDriver.newInstance( JettyHttpServer.class, requestHandler, @@ -74,10 +73,9 @@ public class TestDrivers { Modules.combine(guiceModules))); } - private static Module newConfigModule( - final ServerConfig.Builder serverConfig, - final ConnectorConfig.Builder connectorConfigBuilder, - final Module... guiceModules) { + private static Module newConfigModule(ServerConfig.Builder serverConfig, + ConnectorConfig.Builder connectorConfigBuilder, + Module... guiceModules) { return Modules.combine( new AbstractModule() { @Override @@ -87,12 +85,13 @@ public class TestDrivers { bind(ConnectorConfig.class).toInstance(new ConnectorConfig(connectorConfigBuilder)); bind(FilterBindings.class).toInstance( new FilterBindings( - new BindingRepository<RequestFilter>(), - new BindingRepository<ResponseFilter>())); + new BindingRepository<>(), + new BindingRepository<>())); } }, new ConnectorFactoryRegistryModule(connectorConfigBuilder), new ServletModule(), Modules.combine(guiceModules)); } + } |