Replace AccessLogEntry with non-blocking RequestLogEntry

Keep AccessLogEntry as interface for adding extra information in handlers, but use the new RequestLogEntry for access log serialization.
Introduce new interface RequestLog that AccessLog class implements (to simplify unit testing).
Rename AccessLogInterface to RequestLogHandler.
Remove unused class AccessLogSampler.

4 files changed, 87 insertions, 72 deletions

diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
index 68ab41e756f..c9b466517b3 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
@@ -3,6 +3,7 @@ package com.yahoo.jdisc.http.server.jetty;
 
 import com.yahoo.container.logging.AccessLogEntry;
 import com.yahoo.container.logging.RequestLog;
+import com.yahoo.container.logging.RequestLogEntry;
 import com.yahoo.jdisc.http.ServerConfig;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.server.HttpChannel;
@@ -19,6 +20,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.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -30,103 +32,115 @@ import static org.mockito.Mockito.when;
 public class AccessLogRequestLogTest {
     @Test
     public void requireThatQueryWithUnquotedSpecialCharactersIsHandled() {
-        final AccessLogEntry accessLogEntry = new AccessLogEntry();
-        final Request jettyRequest = createRequestMock(accessLogEntry);
+        final Request jettyRequest = createRequestMock();
         when(jettyRequest.getRequestURI()).thenReturn("/search/");
         when(jettyRequest.getQueryString()).thenReturn("query=year:>2010");
 
-        doAccessLoggingOfRequest(jettyRequest);
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
 
-        assertThat(accessLogEntry.getRawPath(), is(not(nullValue())));
-        assertTrue(accessLogEntry.getRawQuery().isPresent());
+        assertThat(entry.rawPath().get(), is(not(nullValue())));
+        assertTrue(entry.rawQuery().isPresent());
     }
 
     @Test
     public void requireThatDoubleQuotingIsNotPerformed() {
-        final AccessLogEntry accessLogEntry = new AccessLogEntry();
-        final Request jettyRequest = createRequestMock(accessLogEntry);
+        final Request jettyRequest = createRequestMock();
         final String path = "/search/";
         when(jettyRequest.getRequestURI()).thenReturn(path);
         final String query = "query=year%252010+%3B&customParameter=something";
         when(jettyRequest.getQueryString()).thenReturn(query);
 
-        doAccessLoggingOfRequest(jettyRequest);
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
 
-        assertThat(accessLogEntry.getRawPath(), is(path));
-        assertThat(accessLogEntry.getRawQuery().get(), is(query));
+        assertThat(entry.rawPath().get(), is(path));
+        assertThat(entry.rawQuery().get(), is(query));
 
     }
 
     @Test
     public void raw_path_and_query_are_set_from_request() {
-        AccessLogEntry accessLogEntry = new AccessLogEntry();
-        Request jettyRequest = createRequestMock(accessLogEntry);
+        Request jettyRequest = createRequestMock();
         String rawPath = "//search/";
         when(jettyRequest.getRequestURI()).thenReturn(rawPath);
         String rawQuery = "q=%%2";
         when(jettyRequest.getQueryString()).thenReturn(rawQuery);
 
-        doAccessLoggingOfRequest(jettyRequest);
-        assertThat(accessLogEntry.getRawPath(), is(rawPath));
-        Optional<String> actualRawQuery = accessLogEntry.getRawQuery();
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
+        assertThat(entry.rawPath().get(), is(rawPath));
+        Optional<String> actualRawQuery = entry.rawQuery();
         assertThat(actualRawQuery.isPresent(), is(true));
         assertThat(actualRawQuery.get(), is(rawQuery));
     }
 
     @Test
     public void verify_x_forwarded_for_precedence () {
-        AccessLogEntry accessLogEntry = new AccessLogEntry();
-        Request jettyRequest = createRequestMock(accessLogEntry);
+        Request jettyRequest = createRequestMock();
         when(jettyRequest.getRequestURI()).thenReturn("//search/");
         when(jettyRequest.getQueryString()).thenReturn("q=%%2");
         when(jettyRequest.getHeader("x-forwarded-for")).thenReturn("1.2.3.4");
         when(jettyRequest.getHeader("y-ra")).thenReturn("2.3.4.5");
 
-        doAccessLoggingOfRequest(jettyRequest);
-        assertThat(accessLogEntry.getRemoteAddress(), is("1.2.3.4"));
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
+        assertThat(entry.remoteAddress().get(), is("1.2.3.4"));
     }
 
     @Test
     public void verify_x_forwarded_port_precedence () {
-        AccessLogEntry accessLogEntry = new AccessLogEntry();
-        Request jettyRequest = createRequestMock(accessLogEntry);
+        Request jettyRequest = createRequestMock();
         when(jettyRequest.getRequestURI()).thenReturn("//search/");
         when(jettyRequest.getQueryString()).thenReturn("q=%%2");
         when(jettyRequest.getHeader("X-Forwarded-Port")).thenReturn("80");
         when(jettyRequest.getHeader("y-rp")).thenReturn("8080");
 
-        doAccessLoggingOfRequest(jettyRequest);
-        assertThat(accessLogEntry.getRemotePort(), is(80));
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
+        assertThat(entry.remotePort().getAsInt(), is(80));
     }
 
     @Test
     public void defaults_to_peer_port_if_remote_port_header_is_invalid() {
-        final AccessLogEntry accessLogEntry = new AccessLogEntry();
-        final Request jettyRequest = createRequestMock(accessLogEntry);
+        final Request jettyRequest = createRequestMock();
         when(jettyRequest.getRequestURI()).thenReturn("/search/");
         when(jettyRequest.getHeader("X-Forwarded-Port")).thenReturn("8o8o");
         when(jettyRequest.getRemotePort()).thenReturn(80);
 
-        doAccessLoggingOfRequest(jettyRequest);
-        assertThat(accessLogEntry.getRemotePort(), is(0));
-        assertThat(accessLogEntry.getPeerPort(), is(80));
+        InMemoryRequestLog requestLog = new InMemoryRequestLog();
+        doAccessLoggingOfRequest(requestLog, jettyRequest);
+        RequestLogEntry entry = requestLog.entries().get(0);
+        assertFalse(entry.remotePort().isPresent());
+        assertThat(entry.peerPort().getAsInt(), is(80));
     }
 
-    private void doAccessLoggingOfRequest(Request jettyRequest) {
+    private void doAccessLoggingOfRequest(RequestLog requestLog, Request jettyRequest) {
         ServerConfig.AccessLog config = new ServerConfig.AccessLog(
                 new ServerConfig.AccessLog.Builder()
                         .remoteAddressHeaders(List.of("x-forwarded-for", "y-ra"))
                         .remotePortHeaders(List.of("X-Forwarded-Port", "y-rp")));
-        new AccessLogRequestLog(mock(RequestLog.class), config).log(jettyRequest, createResponseMock());
+        new AccessLogRequestLog(requestLog, config).log(jettyRequest, createResponseMock());
     }
 
-    private static Request createRequestMock(AccessLogEntry entry) {
+    private static Request createRequestMock() {
         ServerConnector serverConnector = mock(ServerConnector.class);
         when(serverConnector.getLocalPort()).thenReturn(1234);
         HttpConnection httpConnection = mock(HttpConnection.class);
         when(httpConnection.getConnector()).thenReturn(serverConnector);
         Request request = mock(Request.class);
-        when(request.getAttribute(JDiscHttpServlet.ATTRIBUTE_NAME_ACCESS_LOG_ENTRY)).thenReturn(entry);
+        when(request.getMethod()).thenReturn("GET");
+        when(request.getRemoteAddr()).thenReturn("localhost");
+        when(request.getRemotePort()).thenReturn(12345);
+        when(request.getProtocol()).thenReturn("HTTP/1.1");
+        when(request.getScheme()).thenReturn("http");
+        when(request.getTimeStamp()).thenReturn(0L);
+        when(request.getAttribute(JDiscHttpServlet.ATTRIBUTE_NAME_ACCESS_LOG_ENTRY)).thenReturn(new AccessLogEntry());
         when(request.getAttribute("org.eclipse.jetty.server.HttpConnection")).thenReturn(httpConnection);
         return request;
     }
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 2e460288cdc..96464a48938 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
@@ -3,7 +3,6 @@ package com.yahoo.jdisc.http.server.jetty;
 
 import com.google.inject.AbstractModule;
 import com.google.inject.Module;
-import com.yahoo.container.logging.AccessLogEntry;
 import com.yahoo.container.logging.ConnectionLog;
 import com.yahoo.container.logging.ConnectionLogEntry;
 import com.yahoo.container.logging.RequestLog;
@@ -72,7 +71,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.UUID;
-import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
@@ -177,7 +175,7 @@ public class HttpServerTest {
 
     @Test
     public void requireThatAccessLogIsCalledForRequestRejectedByJetty() throws Exception {
-        RequestLogMock requestLogMock = new RequestLogMock();
+        InMemoryRequestLog requestLogMock = new InMemoryRequestLog();
         final TestDriver driver = TestDrivers.newConfiguredInstance(
                 mockRequestHandler(),
                 new ServerConfig.Builder(),
@@ -186,26 +184,9 @@ public class HttpServerTest {
         driver.client().get("/status.html")
                 .expectStatusCode(is(REQUEST_URI_TOO_LONG));
         assertThat(driver.close(), is(true));
-
-        assertThat(requestLogMock.logEntries.size(), equalTo(1));
-        AccessLogEntry accessLogEntry = requestLogMock.logEntries.get(0);
-        assertEquals(414, accessLogEntry.getStatusCode());
-    }
-
-    private static class RequestLogMock implements RequestLog {
-
-        final List<AccessLogEntry> logEntries = new CopyOnWriteArrayList<>();
-
-        @Override
-        public void log(AccessLogEntry accessLogEntry) {
-            logEntries.add(accessLogEntry);
-        }
-
-        @Override
-        public void log(RequestLogEntry entry) {
-            // TODO Implement
-        }
-
+        assertThat(requestLogMock.entries().size(), equalTo(1));
+        RequestLogEntry entry = requestLogMock.entries().get(0);
+        assertEquals(414, entry.statusCode().getAsInt());
     }
 
     @Test
@@ -781,7 +762,7 @@ public class HttpServerTest {
         Path privateKeyFile = tmpFolder.newFile().toPath();
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
-        RequestLogMock requestLogMock = new RequestLogMock();
+        InMemoryRequestLog requestLogMock = new InMemoryRequestLog();
         InMemoryConnectionLog connectionLog = new InMemoryConnectionLog();
         TestDriver driver = createSslWithProxyProtocolTestDriver(certificateFile, privateKeyFile, requestLogMock, /*mixedMode*/connectionLog, false);
 
@@ -791,9 +772,9 @@ public class HttpServerTest {
         sendJettyClientRequest(driver, certificateFile, new V2.Tag(proxiedRemoteAddress, proxiedRemotePort));
         assertTrue(driver.close());
 
-        assertEquals(2, requestLogMock.logEntries.size());
-        assertLogEntryHasRemote(requestLogMock.logEntries.get(0), proxiedRemoteAddress, proxiedRemotePort);
-        assertLogEntryHasRemote(requestLogMock.logEntries.get(1), proxiedRemoteAddress, proxiedRemotePort);
+        assertEquals(2, requestLogMock.entries().size());
+        assertLogEntryHasRemote(requestLogMock.entries().get(0), proxiedRemoteAddress, proxiedRemotePort);
+        assertLogEntryHasRemote(requestLogMock.entries().get(1), proxiedRemoteAddress, proxiedRemotePort);
         Assertions.assertThat(connectionLog.logEntries()).hasSize(2);
         assertLogEntryHasRemote(connectionLog.logEntries().get(0), proxiedRemoteAddress, proxiedRemotePort);
         assertLogEntryHasRemote(connectionLog.logEntries().get(1), proxiedRemoteAddress, proxiedRemotePort);
@@ -804,7 +785,7 @@ public class HttpServerTest {
         Path privateKeyFile = tmpFolder.newFile().toPath();
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
-        RequestLogMock requestLogMock = new RequestLogMock();
+        InMemoryRequestLog requestLogMock = new InMemoryRequestLog();
         InMemoryConnectionLog connectionLog = new InMemoryConnectionLog();
         TestDriver driver = createSslWithProxyProtocolTestDriver(certificateFile, privateKeyFile, requestLogMock, /*mixedMode*/connectionLog, true);
 
@@ -813,9 +794,9 @@ public class HttpServerTest {
         sendJettyClientRequest(driver, certificateFile, new V2.Tag(proxiedRemoteAddress, 12345));
         assertTrue(driver.close());
 
-        assertEquals(2, requestLogMock.logEntries.size());
-        assertLogEntryHasRemote(requestLogMock.logEntries.get(0), "127.0.0.1", 0);
-        assertLogEntryHasRemote(requestLogMock.logEntries.get(1), proxiedRemoteAddress, 0);
+        assertEquals(2, requestLogMock.entries().size());
+        assertLogEntryHasRemote(requestLogMock.entries().get(0), "127.0.0.1", 0);
+        assertLogEntryHasRemote(requestLogMock.entries().get(1), proxiedRemoteAddress, 0);
         Assertions.assertThat(connectionLog.logEntries()).hasSize(2);
         assertLogEntryHasRemote(connectionLog.logEntries().get(0), null, 0);
         assertLogEntryHasRemote(connectionLog.logEntries().get(1), proxiedRemoteAddress, 12345);
@@ -826,7 +807,7 @@ public class HttpServerTest {
         Path privateKeyFile = tmpFolder.newFile().toPath();
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
-        RequestLogMock requestLogMock = new RequestLogMock();
+        InMemoryRequestLog requestLogMock = new InMemoryRequestLog();
         InMemoryConnectionLog connectionLog = new InMemoryConnectionLog();
         TestDriver driver = createSslWithProxyProtocolTestDriver(certificateFile, privateKeyFile, requestLogMock, connectionLog, /*mixedMode*/false);
 
@@ -911,10 +892,10 @@ public class HttpServerTest {
         return client;
     }
 
-    private static void assertLogEntryHasRemote(AccessLogEntry entry, String expectedAddress, int expectedPort) {
-        assertEquals(expectedAddress, entry.getPeerAddress());
+    private static void assertLogEntryHasRemote(RequestLogEntry entry, String expectedAddress, int expectedPort) {
+        assertEquals(expectedAddress, entry.peerAddress().get());
         if (expectedPort > 0) {
-            assertEquals(expectedPort, entry.getPeerPort());
+            assertEquals(expectedPort, entry.peerPort().getAsInt());
         }
     }
 
@@ -932,8 +913,8 @@ public class HttpServerTest {
     }
 
     private static TestDriver createSslWithProxyProtocolTestDriver(
-            Path certificateFile, Path privateKeyFile, RequestLogMock requestLogMock,
-            InMemoryConnectionLog connectionLog, boolean mixedMode) {
+            Path certificateFile, Path privateKeyFile, RequestLog requestLog,
+            ConnectionLog connectionLog, boolean mixedMode) {
         ConnectorConfig.Builder connectorConfig = new ConnectorConfig.Builder()
                 .proxyProtocol(new ConnectorConfig.ProxyProtocol.Builder()
                                        .enabled(true)
@@ -948,7 +929,7 @@ public class HttpServerTest {
                 new ServerConfig.Builder().connectionLog(new ServerConfig.ConnectionLog.Builder().enabled(true)),
                 connectorConfig,
                 binder -> {
-                    binder.bind(RequestLog.class).toInstance(requestLogMock);
+                    binder.bind(RequestLog.class).toInstance(requestLog);
                     binder.bind(ConnectionLog.class).toInstance(connectionLog);
                 });
     }
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/InMemoryRequestLog.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/InMemoryRequestLog.java
new file mode 100644
index 00000000000..b87ec5e8b8b
--- /dev/null
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/InMemoryRequestLog.java
@@ -0,0 +1,20 @@
+// Copyright Verizon Media. 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.container.logging.RequestLog;
+import com.yahoo.container.logging.RequestLogEntry;
+
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ * @author bjorncs
+ */
+public class InMemoryRequestLog implements RequestLog {
+
+    private final List<RequestLogEntry> entries = new CopyOnWriteArrayList<>();
+
+    @Override public void log(RequestLogEntry entry) { entries.add(entry); }
+
+    List<RequestLogEntry> entries() { return entries; }
+}
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/servlet/ServletAccessLoggingTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/servlet/ServletAccessLoggingTest.java
index ffd58902256..a533a447f6a 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/servlet/ServletAccessLoggingTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/servlet/ServletAccessLoggingTest.java
@@ -5,8 +5,8 @@ import com.google.inject.AbstractModule;
 import com.google.inject.Module;
 import com.google.inject.util.Modules;
 import com.yahoo.container.logging.AccessLog;
-import com.yahoo.container.logging.AccessLogEntry;
 import com.yahoo.container.logging.RequestLog;
+import com.yahoo.container.logging.RequestLogEntry;
 import com.yahoo.jdisc.http.server.jetty.TestDriver;
 import com.yahoo.jdisc.http.server.jetty.TestDrivers;
 import org.junit.Test;
@@ -44,7 +44,7 @@ public class ServletAccessLoggingTest extends ServletTestBase {
     }
 
     private void verifyCallsLog(RequestLog requestLog, final VerificationMode verificationMode) {
-        verify(requestLog, verificationMode).log(any(AccessLogEntry.class));
+        verify(requestLog, verificationMode).log(any(RequestLogEntry.class));
     }
 
     private TestDriver newTestDriver(RequestLog requestLog) throws IOException {