fix: add default time range filter for ListLogEntries API (#304)

Simon Zeltser · web-flow · commit c2f40df6ed81 · 2020-11-11T11:40:08.000-08:00
* feat!: listLogEntries now appends default 24h filter

BREAKING CHANGE: previously if the user called listLogEntries API with no parameters, the query would try to retrieve all log entries, taking up time and eventually crashing on projects with high volume logs. The new behavior complies with gcloud: if no timestamp filter was specified, it appends a filter covering last 24 hours.
diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/ITimestampDefaultFilter.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/ITimestampDefaultFilter.java
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.cloud.logging;
+
+/**
+ * Encapsulates implementation of default time filter. This is needed for testing since we can't
+ * mock static classes with EasyMock
+ */
+public interface ITimestampDefaultFilter {
+
+  /**
+   * Creates a default filter with timestamp to query Cloud Logging
+   *
+   * @return The filter using timestamp field
+   */
+  String createDefaultTimestampFilter();
+}
diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
@@ -77,9 +77,7 @@
 import com.google.logging.v2.WriteLogEntriesRequest;
 import com.google.logging.v2.WriteLogEntriesResponse;
 import com.google.protobuf.Empty;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -111,6 +109,9 @@ public Void apply(WriteLogEntriesResponse input) {
       };
   private static final ThreadLocal<Boolean> inWriteCall = new ThreadLocal<>();
 
+  @VisibleForTesting
+  static ITimestampDefaultFilter defaultTimestampFilterCreator = new TimestampDefaultFilter();
+
   LoggingImpl(LoggingOptions options) {
     super(options);
     rpc = options.getLoggingRpcV2();
@@ -705,8 +706,7 @@ public void write(Iterable<LogEntry> logEntries, WriteOption... options) {
   public void flush() {
     // BUG(1795): We should force batcher to issue RPC call for buffered messages,
     // so the code below doesn't wait uselessly.
-    ArrayList<ApiFuture<Void>> writesToFlush = new ArrayList<>();
-    writesToFlush.addAll(pendingWrites.values());
+    ArrayList<ApiFuture<Void>> writesToFlush = new ArrayList<>(pendingWrites.values());
 
     try {
       ApiFutures.allAsList(writesToFlush).get(FLUSH_WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS);
@@ -779,9 +779,21 @@ static ListLogEntriesRequest listLogEntriesRequest(
       builder.setOrderBy(orderBy);
     }
     String filter = FILTER.get(options);
+    // Make sure timestamp filter is either explicitly specified or we add a default time filter
+    // of 24 hours back to be inline with gcloud behavior for the same API
     if (filter != null) {
+      if (!filter.toLowerCase().contains("timestamp")) {
+        filter =
+            String.format(
+                "%s AND %s", filter, defaultTimestampFilterCreator.createDefaultTimestampFilter());
+      }
       builder.setFilter(filter);
+    } else {
+      // If filter is not specified, default filter is looking back 24 hours in line with gcloud
+      // behavior
+      builder.setFilter(defaultTimestampFilterCreator.createDefaultTimestampFilter());
     }
+
     return builder.build();
   }
 
@@ -794,16 +806,16 @@ private static ApiFuture<AsyncPage<LogEntry>> listLogEntriesAsync(
         list,
         new Function<ListLogEntriesResponse, AsyncPage<LogEntry>>() {
           @Override
-          public AsyncPage<LogEntry> apply(ListLogEntriesResponse listLogEntrysResponse) {
+          public AsyncPage<LogEntry> apply(ListLogEntriesResponse listLogEntriesResponse) {
             List<LogEntry> entries =
-                listLogEntrysResponse.getEntriesList() == null
+                listLogEntriesResponse.getEntriesList() == null
                     ? ImmutableList.<LogEntry>of()
                     : Lists.transform(
-                        listLogEntrysResponse.getEntriesList(), LogEntry.FROM_PB_FUNCTION);
+                        listLogEntriesResponse.getEntriesList(), LogEntry.FROM_PB_FUNCTION);
             String cursor =
-                listLogEntrysResponse.getNextPageToken().equals("")
+                listLogEntriesResponse.getNextPageToken().equals("")
                     ? null
-                    : listLogEntrysResponse.getNextPageToken();
+                    : listLogEntriesResponse.getNextPageToken();
             return new AsyncPageImpl<>(
                 new LogEntryPageFetcher(serviceOptions, cursor, options), cursor, entries);
           }
diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/TimestampDefaultFilter.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/TimestampDefaultFilter.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.cloud.logging;
+
+import java.text.DateFormat;
+import java.text.SimpleDateFormat;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
+public class TimestampDefaultFilter implements ITimestampDefaultFilter {
+  @Override
+  public String createDefaultTimestampFilter() {
+    DateFormat rfcDateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'");
+    rfcDateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));
+    return "timestamp>=\"" + rfcDateFormat.format(yesterday()) + "\"";
+  }
+
+  private Date yesterday() {
+    TimeZone timeZone = TimeZone.getTimeZone("UTC");
+    Calendar calendar = Calendar.getInstance(timeZone);
+    calendar.add(Calendar.DATE, -1);
+
+    return calendar.getTime();
+  }
+}
diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
@@ -154,7 +154,7 @@ public com.google.api.MonitoredResourceDescriptor apply(
   private static final String NEXT_CURSOR = "nextCursor";
   private static final Boolean DISABLED = Boolean.FALSE;
   private static final Timestamp EXCLUSION_CREATED_TIME = fromMillis(System.currentTimeMillis());
-  private static final Timestamp EXCLUSION_UPDATED_TIME = fromMillis(System.currentTimeMillis());;
+  private static final Timestamp EXCLUSION_UPDATED_TIME = fromMillis(System.currentTimeMillis());
   private static final Exclusion EXCLUSION =
       Exclusion.newBuilder(EXCLUSION_NAME, EXCLUSION_FILTER)
           .setDisabled(DISABLED)
@@ -185,6 +185,18 @@ public void setUp() {
             .setServiceRpcFactory(rpcFactoryMock)
             .setRetrySettings(ServiceOptions.getNoRetrySettings())
             .build();
+
+    // By default when calling ListLogEntries, we append a filter of last 24 hours.
+    // However when testing, the time when it was called by the test and by the method
+    // implementation might differ by microseconds so we use the same time filter implementation
+    // for test and in "real" method
+    LoggingImpl.defaultTimestampFilterCreator =
+        new ITimestampDefaultFilter() {
+          @Override
+          public String createDefaultTimestampFilter() {
+            return "timestamp>=\"2020-10-30T15:39:09Z\"";
+          }
+        };
   }
 
   @After
@@ -1585,7 +1597,7 @@ public void testDeleteLog_Null() {
   }
 
   @Test
-  public void testDeleteLogAync() throws ExecutionException, InterruptedException {
+  public void testDeleteLogAsync() throws ExecutionException, InterruptedException {
     DeleteLogRequest request = DeleteLogRequest.newBuilder().setLogName(LOG_NAME_PB).build();
     ApiFuture<Empty> response = ApiFutures.immediateFuture(Empty.getDefaultInstance());
     EasyMock.expect(loggingRpcMock.delete(request)).andReturn(response);
@@ -1621,7 +1633,7 @@ public void testWriteLogEntries() {
   }
 
   @Test
-  public void testWriteLogEntriesDoesnotEnableFlushByDefault() {
+  public void testWriteLogEntriesDoesNotEnableFlushByDefault() {
     WriteLogEntriesRequest request =
         WriteLogEntriesRequest.newBuilder()
             .addAllEntries(
@@ -1679,7 +1691,7 @@ public void testWriteLogEntriesWithOptions() {
   }
 
   @Test
-  public void testWriteLogEntriesAsync() throws ExecutionException, InterruptedException {
+  public void testWriteLogEntriesAsync() {
     WriteLogEntriesRequest request =
         WriteLogEntriesRequest.newBuilder()
             .addAllEntries(
@@ -1723,16 +1735,16 @@ public void testListLogEntries() {
     String cursor = "cursor";
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
-    ListLogEntriesRequest request =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+
     List<LogEntry> entriesList = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
     ListLogEntriesResponse response =
         ListLogEntriesResponse.newBuilder()
             .setNextPageToken(cursor)
             .addAllEntries(Lists.transform(entriesList, LogEntry.toPbFunction(PROJECT)))
             .build();
     ApiFuture<ListLogEntriesResponse> futureResponse = ApiFutures.immediateFuture(response);
-    EasyMock.expect(loggingRpcMock.list(request)).andReturn(futureResponse);
+    EasyMock.expect(loggingRpcMock.list(EasyMock.anyObject(ListLogEntriesRequest.class)))
+        .andReturn(futureResponse);
     EasyMock.replay(loggingRpcMock);
     Page<LogEntry> page = logging.listLogEntries();
     assertEquals(cursor, page.getNextPageToken());
@@ -1744,11 +1756,18 @@ public void testListLogEntriesNextPage() throws ExecutionException, InterruptedE
     String cursor1 = "cursor";
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
+
+    String defaultTimeFilter =
+        LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();
     ListLogEntriesRequest request1 =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+        ListLogEntriesRequest.newBuilder()
+            .addResourceNames(PROJECT_PB)
+            .setFilter(defaultTimeFilter)
+            .build();
     ListLogEntriesRequest request2 =
         ListLogEntriesRequest.newBuilder()
             .addResourceNames(PROJECT_PB)
+            .setFilter(defaultTimeFilter)
             .setPageToken(cursor1)
             .build();
     List<LogEntry> descriptorList1 = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
@@ -1785,7 +1804,11 @@ public void testListLogEntriesEmpty() {
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
     ListLogEntriesRequest request =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+        ListLogEntriesRequest.newBuilder()
+            .addResourceNames(PROJECT_PB)
+            .setFilter(LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter())
+            .build();
+
     List<LogEntry> entriesList = ImmutableList.of();
     ListLogEntriesResponse response =
         ListLogEntriesResponse.newBuilder()
@@ -1809,7 +1832,10 @@ public void testListLogEntriesWithOptions() {
         ListLogEntriesRequest.newBuilder()
             .addResourceNames(PROJECT_PB)
             .setOrderBy("timestamp desc")
-            .setFilter("logName:syslog")
+            .setFilter(
+                String.format(
+                    "logName:syslog AND %s",
+                    LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter()))
             .build();
     List<LogEntry> entriesList = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
     ListLogEntriesResponse response =
@@ -1834,7 +1860,10 @@ public void testListLogEntriesAsync() throws ExecutionException, InterruptedExce
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
     ListLogEntriesRequest request =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+        ListLogEntriesRequest.newBuilder()
+            .addResourceNames(PROJECT_PB)
+            .setFilter(LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter())
+            .build();
     List<LogEntry> entriesList = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
     ListLogEntriesResponse response =
         ListLogEntriesResponse.newBuilder()
@@ -1855,10 +1884,14 @@ public void testListLogEntriesAsyncNextPage() {
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
     ListLogEntriesRequest request1 =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+        ListLogEntriesRequest.newBuilder()
+            .addResourceNames(PROJECT_PB)
+            .setFilter(LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter())
+            .build();
     ListLogEntriesRequest request2 =
         ListLogEntriesRequest.newBuilder()
             .addResourceNames(PROJECT_PB)
+            .setFilter(LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter())
             .setPageToken(cursor1)
             .build();
     List<LogEntry> descriptorList1 = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
@@ -1890,12 +1923,15 @@ public void testListLogEntriesAsyncNextPage() {
   }
 
   @Test
-  public void testListLogEntriesAyncEmpty() throws ExecutionException, InterruptedException {
+  public void testListLogEntriesAsyncEmpty() throws ExecutionException, InterruptedException {
     String cursor = "cursor";
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
     ListLogEntriesRequest request =
-        ListLogEntriesRequest.newBuilder().addResourceNames(PROJECT_PB).build();
+        ListLogEntriesRequest.newBuilder()
+            .addResourceNames(PROJECT_PB)
+            .setFilter(LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter())
+            .build();
     List<LogEntry> entriesList = ImmutableList.of();
     ListLogEntriesResponse response =
         ListLogEntriesResponse.newBuilder()
@@ -1915,11 +1951,15 @@ public void testListLogEntriesAsyncWithOptions() throws ExecutionException, Inte
     String cursor = "cursor";
     EasyMock.replay(rpcFactoryMock);
     logging = options.getService();
+    String filter =
+        String.format(
+            "logName:syslog AND %s",
+            LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter());
     ListLogEntriesRequest request =
         ListLogEntriesRequest.newBuilder()
             .addResourceNames(PROJECT_PB)
             .setOrderBy("timestamp desc")
-            .setFilter("logName:syslog")
+            .setFilter(filter)
             .build();
     List<LogEntry> entriesList = ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2);
     ListLogEntriesResponse response =
@@ -1933,7 +1973,7 @@ public void testListLogEntriesAsyncWithOptions() throws ExecutionException, Inte
     AsyncPage<LogEntry> page =
         logging
             .listLogEntriesAsync(
-                EntryListOption.filter("logName:syslog"),
+                EntryListOption.filter(filter),
                 EntryListOption.sortOrder(SortingField.TIMESTAMP, Logging.SortingOrder.DESCENDING))
             .get();
     assertEquals(cursor, page.getNextPageToken());
@@ -2011,8 +2051,8 @@ public void run() {
           };
       threads[i].start();
     }
-    for (int i = 0; i < threads.length; i++) {
-      threads[i].join();
+    for (Thread thread : threads) {
+      thread.join();
     }
     assertSame(0, exceptions.get());
   }
diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/TimestampDefaultFilterTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/TimestampDefaultFilterTest.java
