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

simonz130 · 2020-10-29T06:35:10Z

Making the behavior of ListLogEntries API consistent with gcloud: if filter wasn't specified we now attach a default filter of last 24 hours. This will prevent calls from hanging when listing all logs from projects with high log volumes.

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #303 ☕️

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 639-641
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
- lines 1488-1488

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

codecov · 2020-10-29T06:40:55Z

Codecov Report

Merging #304 (a0e16a9) into master (983f106) will increase coverage by 0.08%.
The diff coverage is 89.47%.

@@             Coverage Diff              @@
##             master     #304      +/-   ##
============================================
+ Coverage     75.77%   75.85%   +0.08%     
- Complexity      636      640       +4     
============================================
  Files            43       44       +1     
  Lines          3863     3876      +13     
  Branches        280      281       +1     
============================================
+ Hits           2927     2940      +13     
  Misses          772      772              
  Partials        164      164

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/google/cloud/logging/LoggingImpl.java	`89.85% <81.81%> (+0.14%)`	`92.00 <1.00> (+1.00)`
...m/google/cloud/logging/TimestampDefaultFilter.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 983f106...a0e16a9. Read the comment docs.

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 639-641
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
- lines 1488-1488

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 639-641
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
- lines 1488-1488

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 638-640
google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
- lines 1491-1491

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

…ggingImplTest.java Co-authored-by: yoshi-code-bot <[email protected]>

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 638-641

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 638-641

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 638-641

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java
- lines 39-39
- lines 789-792

…istLogsFlackiness

yoshi-code-bot · 2020-11-10T08:48:38Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

+        builder.setFilter(filter);
+      }
+    else {
+      // If filter is not specified, default filter is looking back 24 hours in line with gcloud behavior
+      builder.setFilter(createDefaultTimeRangeFilter());


Suggested change

builder.setFilter(filter);

}

else {

// If filter is not specified, default filter is looking back 24 hours in line with gcloud behavior

builder.setFilter(createDefaultTimeRangeFilter());

builder.setFilter(filter);

} else {

// If filter is not specified, default filter is looking back 24 hours in line with gcloud

// behavior

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

daniel-sanche · 2020-11-10T18:51:57Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

+    calendar.set(Calendar.HOUR_OF_DAY, 0);
+    calendar.set(Calendar.MINUTE, 0);
+    calendar.set(Calendar.SECOND, 0);
+    calendar.set(Calendar.MILLISECOND, 0);


This reads to me like "yesterday at midnight", not "24 hours ago". Is that right?

daniel-sanche · 2020-11-10T18:54:53Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

+  }
+
+  private static Date yesterday() {
+    Calendar calendar = Calendar.getInstance();


What timezone is used here? Is that accounted for?

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

daniel-sanche · 2020-11-10T19:43:24Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

    if (filter != null) {
+      if (!filter.contains("timestamp")) {


you should try filter.lower(). "TIMESTAMP" is also allowed

yoshi-code-bot

Some suggestions could not be made:

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java
- lines 80-81
- lines 1747-1747

yoshi-code-bot · 2020-11-11T04:43:14Z

google-cloud-logging/src/main/java/com/google/cloud/logging/ITimestampDefaultFilter.java

+ * 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


Suggested change

* 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

* Encapsulates implementation of default time filter. This is needed for testing since we can't

* mock static classes with EasyMock

/**

* Creates a default filter with timestamp to query Cloud Logging

*

* @return The filter using timestamp field

*/

String createDefaultTimestampFilter();

yoshi-code-bot · 2020-11-11T04:43:15Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

    if (filter != null) {
+      if (!filter.toLowerCase().contains("timestamp")) {
+        filter = String.format("%s AND %s", filter, defaultTimestampFilterCreator.createDefaultTimestampFilter());


Suggested change

filter = String.format("%s AND %s", filter, defaultTimestampFilterCreator.createDefaultTimestampFilter());

filter =

String.format(

"%s AND %s", filter, defaultTimestampFilterCreator.createDefaultTimestampFilter());

yoshi-code-bot · 2020-11-11T04:43:15Z

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java

+                    listLogEntriesResponse.getEntriesList() == null
                    ? ImmutableList.<LogEntry>of()
                    : Lists.transform(


Suggested change

listLogEntriesResponse.getEntriesList() == null

? ImmutableList.<LogEntry>of()

: Lists.transform(

listLogEntriesResponse.getEntriesList() == null

listLogEntriesResponse.getEntriesList(), LogEntry.FROM_PB_FUNCTION);

listLogEntriesResponse.getNextPageToken().equals("")

yoshi-code-bot · 2020-11-11T04:43:15Z

google-cloud-logging/src/main/java/com/google/cloud/logging/TimestampDefaultFilter.java

+    @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);
+


Suggested change

@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);

@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();

}

yoshi-code-bot · 2020-11-11T04:43:18Z

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

+    LoggingImpl.defaultTimestampFilterCreator = new ITimestampDefaultFilter() {
+      @Override
+      public String createDefaultTimestampFilter() {
+        return "timestamp>=\"2020-10-30T15:39:09Z\"";
+      }
+    };


Suggested change

LoggingImpl.defaultTimestampFilterCreator = new ITimestampDefaultFilter() {

@Override

public String createDefaultTimestampFilter() {

return "timestamp>=\"2020-10-30T15:39:09Z\"";

}

};

LoggingImpl.defaultTimestampFilterCreator =

new ITimestampDefaultFilter() {

@Override

public String createDefaultTimestampFilter() {

return "timestamp>=\"2020-10-30T15:39:09Z\"";

}

};

yoshi-code-bot · 2020-11-11T04:43:19Z

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

@@ -1744,11 +1756,17 @@ public void testListLogEntriesNextPage() throws ExecutionException, InterruptedE
    String cursor1 = "cursor";
    EasyMock.replay(rpcFactoryMock);
    logging = options.getService();
+
+    String defaultTimeFilter = LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();


Suggested change

String defaultTimeFilter = LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();

String defaultTimeFilter =

LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();

yoshi-code-bot · 2020-11-11T04:43:19Z

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

@@ -1809,7 +1831,8 @@ public void testListLogEntriesWithOptions() {
        ListLogEntriesRequest.newBuilder()
            .addResourceNames(PROJECT_PB)
            .setOrderBy("timestamp desc")
-            .setFilter("logName:syslog")
+            .setFilter(
+                String.format("logName:syslog AND %s", LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter()))


Suggested change

String.format("logName:syslog AND %s", LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter()))

String.format(

"logName:syslog AND %s",

LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter()))

yoshi-code-bot · 2020-11-11T04:43:21Z

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

@@ -1915,11 +1948,12 @@ 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());


Suggested change

String filter = String.format("logName:syslog AND %s", LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter());

String filter =

String.format(

"logName:syslog AND %s",

LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter());

yoshi-code-bot · 2020-11-11T04:43:21Z

google-cloud-logging/src/test/java/com/google/cloud/logging/TimestampDefaultFilterTest.java

+import org.junit.Test;
+
+import javax.management.timer.Timer;
+import java.text.DateFormat;
+import java.text.SimpleDateFormat;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class TimestampDefaultFilterTest {
+
+    @Test
+    public void DefaultTimestampFilterTest() {
+        ITimestampDefaultFilter filter = new TimestampDefaultFilter();
+
+        TimeZone timeZone = TimeZone.getTimeZone("UTC");
+        Calendar calendar = Calendar.getInstance(timeZone);
+        calendar.add(Calendar.DATE, -1);
+        Date expected = calendar.getTime();
+
+        // Timestamp filter exists
+        String defaultFilter = filter.createDefaultTimestampFilter();
+        assertTrue(defaultFilter.contains("timestamp>="));
+
+        // Time is last 24 hours


Suggested change

import org.junit.Test;

import javax.management.timer.Timer;

import java.text.DateFormat;

import java.text.SimpleDateFormat;

import java.util.Calendar;

import java.util.Date;

import java.util.TimeZone;

import static org.junit.Assert.assertTrue;

import static org.junit.Assert.fail;

public class TimestampDefaultFilterTest {

@Test

public void DefaultTimestampFilterTest() {

ITimestampDefaultFilter filter = new TimestampDefaultFilter();

TimeZone timeZone = TimeZone.getTimeZone("UTC");

Calendar calendar = Calendar.getInstance(timeZone);

calendar.add(Calendar.DATE, -1);

Date expected = calendar.getTime();

// Timestamp filter exists

String defaultFilter = filter.createDefaultTimestampFilter();

assertTrue(defaultFilter.contains("timestamp>="));

// Time is last 24 hours

import static org.junit.Assert.assertTrue;

import static org.junit.Assert.fail;

import javax.management.timer.Timer;

import org.junit.Test;

@Test

public void DefaultTimestampFilterTest() {

ITimestampDefaultFilter filter = new TimestampDefaultFilter();

TimeZone timeZone = TimeZone.getTimeZone("UTC");

Calendar calendar = Calendar.getInstance(timeZone);

calendar.add(Calendar.DATE, -1);

Date expected = calendar.getTime();

// Timestamp filter exists

String defaultFilter = filter.createDefaultTimestampFilter();

assertTrue(defaultFilter.contains("timestamp>="));

// Time is last 24 hours

try {

DateFormat rfcDateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'");

rfcDateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));

Date actual = rfcDateFormat.parse(defaultFilter.substring(12, defaultFilter.length() - 1));

assertTrue(Math.abs(expected.getTime() - actual.getTime()) < Timer.ONE_MINUTE);

} catch (java.text.ParseException ex) {

fail(); // Just fail if exception is thrown

}

daniel-sanche · 2020-11-11T19:35:10Z

google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingImplTest.java

    ListLogEntriesRequest request =
        ListLogEntriesRequest.newBuilder()
            .addResourceNames(PROJECT_PB)
            .setOrderBy("timestamp desc")
-            .setFilter("logName:syslog")
+            .setFilter(filter)


I'm not sure if I'm reading these tests right, but they all seem to include a timestamp as part of the filter. I'd expect to see tests with no filter added to make sure it's added automatically, and tests with "timestamp" or "TIMESTAMP" in the string to make sure those are behaving as expected

simonz130 requested a review from chingor13 October 29, 2020 06:35

simonz130 requested a review from a team as a code owner October 29, 2020 06:35

simonz130 requested a review from a team October 29, 2020 06:35

simonz130 requested a review from a team as a code owner October 29, 2020 06:35

simonz130 self-assigned this Oct 29, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 29, 2020

yoshi-code-bot reviewed Oct 29, 2020

View reviewed changes

fix: make ListLogEntries use 24hr filter

552997c

simonz130 force-pushed the fixListLogsFlackiness branch from d69cdde to 552997c Compare October 29, 2020 06:46

yoshi-code-bot reviewed Oct 29, 2020

View reviewed changes

fix: lint

fa981c4

yoshi-code-bot reviewed Oct 29, 2020

View reviewed changes

Simon Zeltser and others added 2 commits November 2, 2020 09:28

Update google-cloud-logging/src/test/java/com/google/cloud/logging/Lo…

78b7564

…ggingImplTest.java Co-authored-by: yoshi-code-bot <[email protected]>

Update google-cloud-logging/src/test/java/com/google/cloud/logging/Lo…

71a33d1

…ggingImplTest.java Co-authored-by: yoshi-code-bot <[email protected]>

yoshi-code-bot reviewed Nov 2, 2020

View reviewed changes

Merge branch 'master' into fixListLogsFlackiness

cf9f0db

yoshi-code-bot reviewed Nov 9, 2020

View reviewed changes

stephaniewang526 approved these changes Nov 9, 2020

View reviewed changes

simonz130 added 2 commits November 9, 2020 23:47

Merge branch 'master' of github.com:googleapis/java-logging into fixL…

7a46890

…istLogsFlackiness

fix: fixing lint

3cfa258

yoshi-code-bot reviewed Nov 10, 2020

View reviewed changes

fix: lint2

896666d

simonz130 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 10, 2020

freelerobot reviewed Nov 10, 2020

View reviewed changes

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingImpl.java Show resolved Hide resolved

daniel-sanche reviewed Nov 10, 2020

View reviewed changes

fix: autoadd timestamp if not there

5abdb07

yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 10, 2020

daniel-sanche reviewed Nov 10, 2020

View reviewed changes

simonz130 added 2 commits November 10, 2020 20:38

feat!: listLogEntries now appends default 24h filter

641acb9

fix: added lowercase check

ddfd50d

yoshi-code-bot reviewed Nov 11, 2020

View reviewed changes

chore: lint

a0e16a9

daniel-sanche reviewed Nov 11, 2020

View reviewed changes

daniel-sanche approved these changes Nov 11, 2020

View reviewed changes

simonz130 merged commit c2f40df into googleapis:master Nov 11, 2020

simonz130 deleted the fixListLogsFlackiness branch November 11, 2020 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

simonz130 commented Oct 29, 2020

yoshi-code-bot left a comment

codecov bot commented Oct 29, 2020 •

edited

Loading

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot left a comment

yoshi-code-bot Nov 10, 2020

daniel-sanche Nov 10, 2020

daniel-sanche Nov 10, 2020

daniel-sanche Nov 10, 2020

yoshi-code-bot left a comment

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

yoshi-code-bot Nov 11, 2020

daniel-sanche Nov 11, 2020

	String defaultTimeFilter = LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();
	String defaultTimeFilter =
	LoggingImpl.defaultTimestampFilterCreator.createDefaultTimestampFilter();

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

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

Conversation

simonz130 commented Oct 29, 2020

yoshi-code-bot left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshi-code-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2020 •

edited

Loading