diff options
17 files changed, 183 insertions, 165 deletions
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/SessionContentReadResponse.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/SessionContentReadResponse.java index 07d27ffd5de..a1a41cc0472 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/SessionContentReadResponse.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/SessionContentReadResponse.java @@ -14,9 +14,9 @@ import static com.yahoo.jdisc.http.HttpResponse.Status.OK; * Represents a response for a request to read contents of a file. * * @author Ulf Lilleengen - * @since 5.1 */ public class SessionContentReadResponse extends HttpResponse { + private final ApplicationFile file; public SessionContentReadResponse(ApplicationFile file) { @@ -35,4 +35,5 @@ public class SessionContentReadResponse extends HttpResponse { public String getContentType() { return HttpResponse.DEFAULT_MIME_TYPE; } + } diff --git a/container-core/abi-spec.json b/container-core/abi-spec.json index 9292a946e82..244087e0271 100644 --- a/container-core/abi-spec.json +++ b/container-core/abi-spec.json @@ -464,7 +464,9 @@ "public java.lang.String getCharacterEncoding()", "public void populateAccessLogEntry(com.yahoo.container.logging.AccessLogEntry)", "public void complete()", - "public java.lang.Iterable getLogValues()" + "public java.lang.Iterable getLogValues()", + "public void setRequestType(com.yahoo.jdisc.Request$RequestType)", + "public com.yahoo.jdisc.Request$RequestType getRequestType()" ], "fields": [ "public static final java.lang.String DEFAULT_MIME_TYPE", 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 b4fcd044e50..dd03d72d97d 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 @@ -3,6 +3,7 @@ package com.yahoo.container.jdisc; import com.yahoo.container.logging.AccessLogEntry; import com.yahoo.jdisc.HeaderFields; +import com.yahoo.jdisc.Request; import com.yahoo.jdisc.Response; import com.yahoo.processing.execution.Execution.Trace.LogValue; @@ -18,21 +19,18 @@ import java.util.Collections; */ public abstract class HttpResponse { - /** - * Default response content type; text/plain. - */ + /** Default response content type; text/plain. */ public static final String DEFAULT_MIME_TYPE = "text/plain"; - /** - * Default encoding/character set of a HTTP response; UTF-8. - */ + /** Default encoding/character set of a HTTP response; UTF-8. */ public static final String DEFAULT_CHARACTER_ENCODING = "UTF-8"; - private final Response parentResponse; + private Request.RequestType requestType; + /** - * Create a new HTTP response. + * Creates a new HTTP response * * @param status the HTTP status code to return with this response (may be changed later) * @see Response @@ -41,13 +39,11 @@ public abstract class HttpResponse { parentResponse = com.yahoo.jdisc.http.HttpResponse.newInstance(status); } - /** - * Marshal this response to the network layer. The caller is responsible for flushing and closing outputStream. - */ + /** Marshals this response to the network layer. The caller is responsible for flushing and closing outputStream. */ public abstract void render(OutputStream outputStream) throws IOException; /** - * The numeric HTTP status code, e.g. 200, 404 and so on. + * Returns the numeric HTTP status code, e.g. 200, 404 and so on. * * @return the numeric HTTP status code */ @@ -129,4 +125,13 @@ public abstract class HttpResponse { return Collections::emptyIterator; } + /** 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. + * When not set, the request type will be "read" for GET requests and "write" for other request methods. + */ + public Request.RequestType getRequestType() { return requestType; } + } 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 3a99ee7d0c6..ac1aa533201 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,6 +78,7 @@ 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/container-search-and-docproc/src/main/java/com/yahoo/container/handler/observability/ApplicationStatusHandler.java b/container-search-and-docproc/src/main/java/com/yahoo/container/handler/observability/ApplicationStatusHandler.java index 05d14e6e1d6..a58aabfce17 100644 --- a/container-search-and-docproc/src/main/java/com/yahoo/container/handler/observability/ApplicationStatusHandler.java +++ b/container-search-and-docproc/src/main/java/com/yahoo/container/handler/observability/ApplicationStatusHandler.java @@ -332,4 +332,5 @@ public class ApplicationStatusHandler extends AbstractRequestHandler { handler.completed(); } } + } diff --git a/container-search-gui/src/main/java/com/yahoo/search/query/gui/GUIHandler.java b/container-search-gui/src/main/java/com/yahoo/search/query/gui/GUIHandler.java index bafe1dbd43f..9ddcc7a7e69 100644 --- a/container-search-gui/src/main/java/com/yahoo/search/query/gui/GUIHandler.java +++ b/container-search-gui/src/main/java/com/yahoo/search/query/gui/GUIHandler.java @@ -40,7 +40,6 @@ import java.util.logging.Level; * * @author Henrik Høiness */ - public class GUIHandler extends LoggingRequestHandler { private final IndexModel indexModel; diff --git a/container-search/src/main/java/com/yahoo/search/Result.java b/container-search/src/main/java/com/yahoo/search/Result.java index ab48d5797b2..6a875851ca9 100644 --- a/container-search/src/main/java/com/yahoo/search/Result.java +++ b/container-search/src/main/java/com/yahoo/search/Result.java @@ -51,7 +51,7 @@ public final class Result extends com.yahoo.processing.Response implements Clone * Headers containing "envelope" meta information to be returned with this result. * Used for HTTP getHeaders when the return protocol is HTTP. */ - private ListMap<String,String> headers = null; + private ListMap<String, String> headers = null; /** Creates a new Result where the top level hit group has id "toplevel" */ public Result(Query query) { @@ -66,7 +66,6 @@ public final class Result extends com.yahoo.processing.Response implements Clone * @param query the query which produced this result * @param hits the hit container which this will return from {@link #hits()} */ - @SuppressWarnings("deprecation") public Result(Query query, HitGroup hits) { super(query); if (query==null) throw new NullPointerException("The query reference in a result cannot be null"); diff --git a/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java b/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java index b0c8fbba059..bb4df325762 100644 --- a/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java +++ b/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java @@ -17,6 +17,7 @@ import com.yahoo.container.jdisc.VespaHeaders; import com.yahoo.container.logging.AccessLog; import com.yahoo.io.IOUtils; import com.yahoo.jdisc.Metric; +import com.yahoo.jdisc.Request; import com.yahoo.language.Linguistics; import java.util.logging.Level; import com.yahoo.net.HostName; @@ -314,6 +315,7 @@ public class SearchHandler extends LoggingRequestHandler { HttpSearchResponse response = new HttpSearchResponse(getHttpResponseStatus(request, result), result, query, renderer, extractTraceNode(query)); + response.setRequestType(Request.RequestType.READ); if (hostResponseHeaderKey.isPresent()) response.headers().add(hostResponseHeaderKey.get(), selfHostname); 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 e0292bbe026..bce6f72c1fc 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java @@ -48,6 +48,11 @@ public class Request extends AbstractResource { private boolean serverRequest; private Long timeout; private URI uri; + private RequestType requestType; + + public enum RequestType { + READ, WRITE, MONITORING + } /** * <p>Creates a new instance of this class. As a {@link ServerProvider} you need to inject a {@link @@ -326,6 +331,12 @@ 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_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 b9d686c1d6b..81577561c5b 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 @@ -67,12 +67,11 @@ class HttpRequestDispatch { this.jettyRequest = (Request) servletRequest; this.metricReporter = new MetricReporter(jDiscContext.metric, metricContext, jettyRequest.getTimeStamp()); - this.servletResponseController = new ServletResponseController( - servletRequest, - servletResponse, - jDiscContext.janitor, - metricReporter, - jDiscContext.developerMode()); + this.servletResponseController = new ServletResponseController(servletRequest, + servletResponse, + jDiscContext.janitor, + metricReporter, + jDiscContext.developerMode()); markConnectionAsNonPersistentIfThresholdReached(servletRequest); this.async = servletRequest.startAsync(); async.setTimeout(0); @@ -86,17 +85,13 @@ class HttpRequestDispatch { } catch (Throwable throwable) { servletResponseController.trySendError(throwable); servletResponseController.finishedFuture().whenComplete((result, exception) -> - completeRequestCallback.accept(null, throwable)); + completeRequestCallback.accept(null, throwable)); return; } try { - onError(servletRequestReader.finishedFuture, - servletResponseController::trySendError); - - onError(servletResponseController.finishedFuture(), - servletRequestReader::onError); - + onError(servletRequestReader.finishedFuture, servletResponseController::trySendError); + onError(servletResponseController.finishedFuture(), servletRequestReader::onError); CompletableFuture.allOf(servletRequestReader.finishedFuture, servletResponseController.finishedFuture()) .whenComplete(completeRequestCallback); } catch (Throwable throwable) { @@ -104,7 +99,7 @@ class HttpRequestDispatch { } } - private BiConsumer<Void, Throwable> completeRequestCallback; + private final BiConsumer<Void, Throwable> completeRequestCallback; { AtomicBoolean completeRequestCalled = new AtomicBoolean(false); HttpRequestDispatch parent = this; //used to avoid binding uninitialized variables @@ -139,7 +134,7 @@ class HttpRequestDispatch { log.finest(() -> "Request completed successfully: " + parent.jettyRequest.getRequestURI()); } catch (Throwable throwable) { Level level = reportedError ? Level.FINE: Level.WARNING; - log.log(level, "async.complete failed", throwable); + log.log(level, "Async.complete failed", throwable); } }; } @@ -180,16 +175,17 @@ 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(); - ServletRequestReader servletRequestReader = - new ServletRequestReader( - servletInputStream, - requestContentChannel, - jDiscContext.janitor, - metricReporter); + ServletRequestReader servletRequestReader = new ServletRequestReader(servletInputStream, + requestContentChannel, + jDiscContext.janitor, + metricReporter); servletInputStream.setReadListener(servletRequestReader); return servletRequestReader; 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 13abb8ddd4d..d2ac0cb7f6a 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 @@ -1,6 +1,7 @@ // Copyright 2018 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.http.HttpRequest; import com.yahoo.jdisc.http.server.jetty.JettyHttpServer.Metrics; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.AsyncContextEvent; @@ -26,18 +27,22 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.LongAdder; /** - * HttpResponseStatisticsCollector collects statistics about HTTP response types aggregated by category (1xx, 2xx, etc). It is similar to - * {@link org.eclipse.jetty.server.handler.StatisticsHandler} with the distinction that this class collects response type statistics grouped + * HttpResponseStatisticsCollector collects statistics about HTTP response types aggregated by category + * (1xx, 2xx, etc). It is similar to {@link org.eclipse.jetty.server.handler.StatisticsHandler} + * with the distinction that this class collects response type statistics grouped * by HTTP method and only collects the numbers that are reported as metrics from Vespa. * * @author ollivir */ public class HttpResponseStatisticsCollector extends HandlerWrapper implements Graceful { + + static final String requestTypeAttribute = "requestType"; + private final AtomicReference<FutureCallback> shutdown = new AtomicReference<>(); private final List<String> monitoringHandlerPaths; private final List<String> searchHandlerPaths; - public static enum HttpMethod { + public enum HttpMethod { GET, PATCH, POST, PUT, DELETE, OPTIONS, HEAD, OTHER } @@ -45,18 +50,20 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G HTTP, HTTPS, OTHER } - public enum RequestType { - READ, WRITE, MONITORING - } - - private static final String[] HTTP_RESPONSE_GROUPS = { Metrics.RESPONSES_1XX, Metrics.RESPONSES_2XX, Metrics.RESPONSES_3XX, - Metrics.RESPONSES_4XX, Metrics.RESPONSES_5XX, Metrics.RESPONSES_401, Metrics.RESPONSES_403}; + private static final String[] HTTP_RESPONSE_GROUPS = { + Metrics.RESPONSES_1XX, + Metrics.RESPONSES_2XX, + Metrics.RESPONSES_3XX, + Metrics.RESPONSES_4XX, + Metrics.RESPONSES_5XX, + Metrics.RESPONSES_401, + Metrics.RESPONSES_403 + }; private final AtomicLong inFlight = new AtomicLong(); - private final LongAdder statistics[][][][]; + private final LongAdder[][][][] statistics; public HttpResponseStatisticsCollector(List<String> monitoringHandlerPaths, List<String> searchHandlerPaths) { - super(); this.monitoringHandlerPaths = monitoringHandlerPaths; this.searchHandlerPaths = searchHandlerPaths; statistics = new LongAdder[HttpScheme.values().length][HttpMethod.values().length][][]; @@ -64,8 +71,8 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G for (int method = 0; method < HttpMethod.values().length; method++) { statistics[scheme][method] = new LongAdder[HTTP_RESPONSE_GROUPS.length][]; for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) { - statistics[scheme][method][group] = new LongAdder[RequestType.values().length]; - for (int requestType = 0 ; requestType < RequestType.values().length; requestType++) { + statistics[scheme][method][group] = new LongAdder[HttpRequest.RequestType.values().length]; + for (int requestType = 0; requestType < HttpRequest.RequestType.values().length; requestType++) { statistics[scheme][method][group][requestType] = new LongAdder(); } } @@ -74,18 +81,17 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G } private final AsyncListener completionWatcher = new AsyncListener() { + @Override - public void onTimeout(AsyncEvent event) throws IOException { - } + public void onTimeout(AsyncEvent event) { } @Override - public void onStartAsync(AsyncEvent event) throws IOException { + public void onStartAsync(AsyncEvent event) { event.getAsyncContext().addListener(this); } @Override - public void onError(AsyncEvent event) throws IOException { - } + public void onError(AsyncEvent event) { } @Override public void onComplete(AsyncEvent event) throws IOException { @@ -101,18 +107,16 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G throws IOException, ServletException { inFlight.incrementAndGet(); - /* The control flow logic here is mostly a copy from org.eclipse.jetty.server.handler.StatisticsHandler.handle(..) */ try { Handler handler = getHandler(); if (handler != null && shutdown.get() == null && isStarted()) { handler.handle(path, baseRequest, request, response); - } else if (!baseRequest.isHandled()) { + } else if ( ! baseRequest.isHandled()) { baseRequest.setHandled(true); response.sendError(HttpStatus.SERVICE_UNAVAILABLE_503); } } finally { HttpChannelState state = baseRequest.getHttpChannelState(); - if (state.isSuspended()) { if (state.isInitial()) { state.addListener(completionWatcher); @@ -128,7 +132,7 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G if (group >= 0) { HttpScheme scheme = getScheme(request); HttpMethod method = getMethod(request); - RequestType requestType = getRequestType(request); + HttpRequest.RequestType requestType = getRequestType(request); statistics[scheme.ordinal()][method.ordinal()][group][requestType.ordinal()].increment(); if (group == 5 || group == 6) { // if 401/403, also increment 4xx @@ -197,18 +201,22 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G } } - private RequestType getRequestType(Request request) { + private HttpRequest.RequestType getRequestType(Request request) { + HttpRequest.RequestType requestType = (HttpRequest.RequestType)request.getAttribute(requestTypeAttribute); + if (requestType != null) return requestType; + + // Deduce from path and method: String path = request.getRequestURI(); for (String monitoringHandlerPath : monitoringHandlerPaths) { - if (path.startsWith(monitoringHandlerPath)) return RequestType.MONITORING; + if (path.startsWith(monitoringHandlerPath)) return HttpRequest.RequestType.MONITORING; } for (String searchHandlerPath : searchHandlerPaths) { - if (path.startsWith(searchHandlerPath)) return RequestType.READ; + if (path.startsWith(searchHandlerPath)) return HttpRequest.RequestType.READ; } if ("GET".equals(request.getMethod())) { - return RequestType.READ; + return HttpRequest.RequestType.READ; } else { - return RequestType.WRITE; + return HttpRequest.RequestType.WRITE; } } @@ -219,7 +227,7 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G for (HttpMethod method : HttpMethod.values()) { int methodIndex = method.ordinal(); for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) { - for (RequestType type : RequestType.values()) { + for (HttpRequest.RequestType type : HttpRequest.RequestType.values()) { long value = statistics[schemeIndex][methodIndex][group][type.ordinal()].sumThenReset(); if (value > 0) { ret.add(new StatisticsEntry(scheme.name().toLowerCase(), method.name(), HTTP_RESPONSE_GROUPS[group], type.name().toLowerCase(), value)); @@ -241,15 +249,13 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G protected void doStop() throws Exception { super.doStop(); FutureCallback shutdownCb = shutdown.get(); - if (shutdown != null && !shutdownCb.isDone()) { + if ( ! shutdownCb.isDone()) { shutdownCb.failed(new TimeoutException()); } } @Override public Future<Void> shutdown() { - /* This shutdown callback logic is a copy from org.eclipse.jetty.server.handler.StatisticsHandler */ - FutureCallback shutdownCb = new FutureCallback(false); shutdown.compareAndSet(null, shutdownCb); shutdownCb = shutdown.get(); @@ -266,13 +272,13 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G } public static class StatisticsEntry { + public final String scheme; public final String method; public final String name; public final String requestType; public final long value; - public StatisticsEntry(String scheme, String method, String name, String requestType, long value) { this.scheme = scheme; this.method = method; @@ -280,5 +286,7 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G this.requestType = requestType; this.value = value; } + } + } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java index a6b2deb4681..dfbcfb741f5 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java @@ -33,49 +33,47 @@ class JDiscHttpServlet extends HttpServlet { private final static Logger log = Logger.getLogger(JDiscHttpServlet.class.getName()); private final JDiscContext context; + private static final Set<String> servletSupportedMethods = + Stream.of(Method.OPTIONS, Method.GET, Method.HEAD, Method.POST, Method.PUT, Method.DELETE, Method.TRACE) + .map(Method::name) + .collect(Collectors.toSet()); + public JDiscHttpServlet(JDiscContext context) { this.context = context; } @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doPost(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doHead(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doHead(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doPut(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doPut(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doDelete(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doDelete(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doOptions(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doOptions(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @Override - protected void doTrace(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { + protected void doTrace(HttpServletRequest request, HttpServletResponse response) throws IOException { dispatchHttpRequest(request, response); } @@ -92,11 +90,6 @@ class JDiscHttpServlet extends HttpServlet { context.metric.add(JettyHttpServer.Metrics.NUM_REQUESTS, 1, metricContext); context.metric.add(JettyHttpServer.Metrics.JDISC_HTTP_REQUESTS, 1, metricContext); - - Set<String> servletSupportedMethods = - Stream.of(Method.OPTIONS, Method.GET, Method.HEAD, Method.POST, Method.PUT, Method.DELETE, Method.TRACE) - .map(Method::name) - .collect(Collectors.toSet()); String method = request.getMethod().toUpperCase(); if (servletSupportedMethods.contains(method)) { super.service(request, response); @@ -109,8 +102,6 @@ class JDiscHttpServlet extends HttpServlet { } } - - static JDiscServerConnector getConnector(HttpServletRequest request) { return (JDiscServerConnector)getConnection(request).getConnector(); } @@ -121,8 +112,7 @@ class JDiscHttpServlet extends HttpServlet { try { switch (request.getDispatcherType()) { case REQUEST: - new HttpRequestDispatch(context, accessLogEntry, getMetricContext(request), request, response) - .dispatch(); + new HttpRequestDispatch(context, accessLogEntry, getMetricContext(request), request, response).dispatch(); break; default: if (log.isLoggable(Level.INFO)) { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java index 51bcb892591..f480c659578 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java @@ -21,6 +21,7 @@ import java.util.concurrent.ConcurrentHashMap; * @author bjorncs */ class JDiscServerConnector extends ServerConnector { + public static final String REQUEST_ATTRIBUTE = JDiscServerConnector.class.getName(); private final Metric.Context metricCtx; private final Map<RequestDimensions, Metric.Context> requestMetricContextCache = new ConcurrentHashMap<>(); 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 386704a5cc2..ba477f9d32f 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 @@ -128,17 +128,16 @@ public class JettyHttpServer extends AbstractServerProvider { private final List<Integer> listenedPorts = new ArrayList<>(); @Inject - public JettyHttpServer( - final CurrentContainer container, - final Metric metric, - final ServerConfig serverConfig, - final ServletPathsConfig servletPathsConfig, - final ThreadFactory threadFactory, - final FilterBindings filterBindings, - final ComponentRegistry<ConnectorFactory> connectorFactories, - final ComponentRegistry<ServletHolder> servletHolders, - final FilterInvoker filterInvoker, - final AccessLog accessLog) { + public JettyHttpServer(CurrentContainer container, + Metric metric, + ServerConfig serverConfig, + ServletPathsConfig servletPathsConfig, + ThreadFactory threadFactory, + FilterBindings filterBindings, + ComponentRegistry<ConnectorFactory> connectorFactories, + ComponentRegistry<ServletHolder> servletHolders, + FilterInvoker filterInvoker, + AccessLog accessLog) { super(container); if (connectorFactories.allComponents().isEmpty()) throw new IllegalArgumentException("No connectors configured."); @@ -160,44 +159,40 @@ public class JettyHttpServer extends AbstractServerProvider { janitor = newJanitor(threadFactory); - JDiscContext jDiscContext = new JDiscContext( - filterBindings.getRequestFilters().activate(), - filterBindings.getResponseFilters().activate(), - container, - janitor, - metric, - serverConfig); + JDiscContext jDiscContext = new JDiscContext(filterBindings.getRequestFilters().activate(), + filterBindings.getResponseFilters().activate(), + container, + janitor, + metric, + serverConfig); ServletHolder jdiscServlet = new ServletHolder(new JDiscHttpServlet(jDiscContext)); FilterHolder jDiscFilterInvokerFilter = new FilterHolder(new JDiscFilterInvokerFilter(jDiscContext, filterInvoker)); List<JDiscServerConnector> connectors = Arrays.stream(server.getConnectors()) - .map(JDiscServerConnector.class::cast) - .collect(toList()); - - server.setHandler( - getHandlerCollection( - serverConfig, - servletPathsConfig, - connectors, - jdiscServlet, - servletHolders, - jDiscFilterInvokerFilter)); + .map(JDiscServerConnector.class::cast) + .collect(toList()); + + server.setHandler(getHandlerCollection(serverConfig, + servletPathsConfig, + connectors, + jdiscServlet, + servletHolders, + jDiscFilterInvokerFilter)); int numMetricReporterThreads = 1; - metricReporterExecutor = Executors.newScheduledThreadPool( - numMetricReporterThreads, - new ThreadFactoryBuilder() - .setDaemon(true) - .setNameFormat(JettyHttpServer.class.getName() + "-MetricReporter-%d") - .setThreadFactory(threadFactory) - .build() - ); + metricReporterExecutor = + Executors.newScheduledThreadPool(numMetricReporterThreads, + new ThreadFactoryBuilder() + .setDaemon(true) + .setNameFormat(JettyHttpServer.class.getName() + "-MetricReporter-%d") + .setThreadFactory(threadFactory) + .build()); metricReporterExecutor.scheduleAtFixedRate(new MetricTask(), 0, 2, TimeUnit.SECONDS); } private static void initializeJettyLogging() { - // Note: Jetty is logging stderr if no logger is explicitly configured. + // Note: Jetty is logging stderr if no logger is explicitly configured try { Log.setLog(new JavaUtilLog()); } catch (Exception e) { @@ -208,32 +203,26 @@ public class JettyHttpServer extends AbstractServerProvider { private static void setupJmx(Server server, ServerConfig serverConfig) { if (serverConfig.jmx().enabled()) { System.setProperty("java.rmi.server.hostname", "localhost"); - server.addBean( - new MBeanContainer(ManagementFactory.getPlatformMBeanServer())); - server.addBean( - new ConnectorServer( - createJmxLoopbackOnlyServiceUrl(serverConfig.jmx().listenPort()), - "org.eclipse.jetty.jmx:name=rmiconnectorserver")); + server.addBean(new MBeanContainer(ManagementFactory.getPlatformMBeanServer())); + server.addBean(new ConnectorServer(createJmxLoopbackOnlyServiceUrl(serverConfig.jmx().listenPort()), + "org.eclipse.jetty.jmx:name=rmiconnectorserver")); } } private static JMXServiceURL createJmxLoopbackOnlyServiceUrl(int port) { try { - return new JMXServiceURL( - "rmi", "localhost", port, "/jndi/rmi://localhost:" + port + "/jmxrmi"); + return new JMXServiceURL("rmi", "localhost", port, "/jndi/rmi://localhost:" + port + "/jmxrmi"); } catch (MalformedURLException e) { throw new RuntimeException(e); } } - private HandlerCollection getHandlerCollection( - ServerConfig serverConfig, - ServletPathsConfig servletPathsConfig, - List<JDiscServerConnector> connectors, - ServletHolder jdiscServlet, - ComponentRegistry<ServletHolder> servletHolders, - FilterHolder jDiscFilterInvokerFilter) { - + private HandlerCollection getHandlerCollection(ServerConfig serverConfig, + ServletPathsConfig servletPathsConfig, + List<JDiscServerConnector> connectors, + ServletHolder jdiscServlet, + ComponentRegistry<ServletHolder> servletHolders, + FilterHolder jDiscFilterInvokerFilter) { ServletContextHandler servletContextHandler = createServletContextHandler(); servletHolders.allComponentsById().forEach((id, servlet) -> { @@ -257,7 +246,9 @@ public class JettyHttpServer extends AbstractServerProvider { GzipHandler gzipHandler = newGzipHandler(serverConfig); gzipHandler.setHandler(authEnforcer); - HttpResponseStatisticsCollector statisticsCollector = new HttpResponseStatisticsCollector(serverConfig.metric().monitoringHandlerPaths(), serverConfig.metric().searchHandlerPaths()); + HttpResponseStatisticsCollector statisticsCollector = + new HttpResponseStatisticsCollector(serverConfig.metric().monitoringHandlerPaths(), + serverConfig.metric().searchHandlerPaths()); statisticsCollector.setHandler(gzipHandler); StatisticsHandler statisticsHandler = newStatisticsHandler(); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java index 9314247b83b..fd1f84f7d49 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java @@ -28,6 +28,7 @@ import java.util.logging.Logger; * it's important that errors are delivered synchronously. */ class ServletRequestReader implements ReadListener { + private enum State { READING, ALL_DATA_READ, REQUEST_CONTENT_CLOSED } @@ -136,7 +137,7 @@ class ServletRequestReader implements ReadListener { requestContentChannel.write(buf, writeCompletionHandler); metricReporter.successfulRead(bytesReceived); bytesRead += bytesReceived; - } catch (final Throwable t) { + } catch (Throwable t) { finishedFuture.completeExceptionally(t); } finally { //decrease due to this method completing. @@ -145,7 +146,7 @@ class ServletRequestReader implements ReadListener { } private void decreaseOutstandingUserCallsAndCloseRequestContentChannelConditionally() { - final boolean shouldCloseRequestContentChannel; + boolean shouldCloseRequestContentChannel; synchronized (monitor) { assertStateNotEquals(state, State.REQUEST_CONTENT_CLOSED); @@ -154,7 +155,7 @@ class ServletRequestReader implements ReadListener { numberOfOutstandingUserCalls -= 1; shouldCloseRequestContentChannel = numberOfOutstandingUserCalls == 0 && - (finishedFuture.isDone() || state == State.ALL_DATA_READ); + (finishedFuture.isDone() || state == State.ALL_DATA_READ); if (shouldCloseRequestContentChannel) { state = State.REQUEST_CONTENT_CLOSED; diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java index 1a8aa23668b..4ae824e2b7a 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java @@ -34,6 +34,7 @@ import static org.hamcrest.Matchers.equalTo; * @author ollivir */ public class HttpResponseStatisticsCollectorTest { + private Connector connector; private List<String> monitoringPaths = List.of("/status.html"); private List<String> searchPaths = List.of("/search"); @@ -41,7 +42,7 @@ public class HttpResponseStatisticsCollectorTest { private int httpResponseCode = 500; @Test - public void statistics_are_aggregated_by_category() throws Exception { + public void statistics_are_aggregated_by_category() { testRequest("http", 300, "GET"); testRequest("http", 301, "GET"); testRequest("http", 200, "GET"); @@ -52,7 +53,7 @@ public class HttpResponseStatisticsCollectorTest { } @Test - public void statistics_are_grouped_by_http_method_and_scheme() throws Exception { + public void statistics_are_grouped_by_http_method_and_scheme() { testRequest("http", 200, "GET"); testRequest("http", 200, "PUT"); testRequest("http", 200, "POST"); @@ -74,7 +75,7 @@ public class HttpResponseStatisticsCollectorTest { } @Test - public void statistics_include_grouped_and_single_statuscodes() throws Exception { + public void statistics_include_grouped_and_single_statuscodes() { testRequest("http", 401, "GET"); testRequest("http", 404, "GET"); testRequest("http", 403, "GET"); @@ -87,7 +88,7 @@ public class HttpResponseStatisticsCollectorTest { } @Test - public void retrieving_statistics_resets_the_counters() throws Exception { + public void retrieving_statistics_resets_the_counters() { testRequest("http", 200, "GET"); testRequest("http", 200, "GET"); @@ -101,7 +102,7 @@ public class HttpResponseStatisticsCollectorTest { } @Test - public void statistics_include_request_type_dimension() throws Exception { + public void statistics_include_request_type_dimension() { testRequest("http", 200, "GET", "/search"); testRequest("http", 200, "POST", "/search"); testRequest("http", 200, "POST", "/feed"); @@ -117,7 +118,14 @@ public class HttpResponseStatisticsCollectorTest { stats = collector.takeStatistics(); assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_2XX, 1L); + } + + @Test + public void request_type_can_be_set_explicitly() { + testRequest("http", 200, "GET", "/search", com.yahoo.jdisc.Request.RequestType.WRITE); + var stats = collector.takeStatistics(); + assertStatisticsEntryWithRequestTypePresent(stats, "http", "GET", Metrics.RESPONSES_2XX, "write", 1L); } @Before @@ -145,13 +153,19 @@ public class HttpResponseStatisticsCollectorTest { server.start(); } - private Request testRequest(String scheme, int responseCode, String httpMethod) throws Exception { + private Request testRequest(String scheme, int responseCode, String httpMethod) { return testRequest(scheme, responseCode, httpMethod, "foo/bar"); } - private Request testRequest(String scheme, int responseCode, String httpMethod, String path) throws Exception { + private Request testRequest(String scheme, int responseCode, String httpMethod, String path) { + return testRequest(scheme, responseCode, httpMethod, path, null); + } + private Request testRequest(String scheme, int responseCode, String httpMethod, String path, + com.yahoo.jdisc.Request.RequestType explicitRequestType) { HttpChannel channel = new HttpChannel(connector, new HttpConfiguration(), null, new DummyTransport()); MetaData.Request metaData = new MetaData.Request(httpMethod, new HttpURI(scheme + "://" + path), HttpVersion.HTTP_1_1, new HttpFields()); Request req = channel.getRequest(); + if (explicitRequestType != null) + req.setAttribute("requestType", explicitRequestType); req.setMetaData(metaData); this.httpResponseCode = responseCode; diff --git a/processing/src/main/java/com/yahoo/processing/Response.java b/processing/src/main/java/com/yahoo/processing/Response.java index e8504eb5087..59e6c3d22ab 100644 --- a/processing/src/main/java/com/yahoo/processing/Response.java +++ b/processing/src/main/java/com/yahoo/processing/Response.java @@ -39,16 +39,12 @@ public class Response extends ListenableFreezableClass { private final DataList<?> data; - /** - * Creates a request containing an empty array data list - */ + /** Creates a request containing an empty array data list */ public Response(Request request) { this(ArrayDataList.create(request)); } - /** - * Creates a response containing a list of data - */ + /** Creates a response containing a list of data */ public Response(DataList<?> data) { this.data = data; @@ -104,7 +100,7 @@ public class Response extends ListenableFreezableClass { return new CompleteAllOnGetFuture<D>(futures); } - @SuppressWarnings("unchecked") + @SuppressWarnings("unchecked") private static <D extends Data> void collectCompletionFutures(DataList<D> dataList, List<ListenableFuture<DataList<D>>> futures) { futures.add(dataList.complete()); for (D data : dataList.asList()) { |