From aa0c176418534c1f07054ab10d66006fd88f6c39 Mon Sep 17 00:00:00 2001 From: losalex <90795544+losalex@users.noreply.github.com> Date: Wed, 30 Nov 2022 16:40:16 -0800 Subject: [PATCH] fix: Need a way to disable flushing (#1206) * fix: Need a way to disable flushing * fix test name * add test for flush level warning * add a comment explaining Severity.UNRECOGNIZED usage for flush level * fix comment * Remove redundant definition * Adjust to latest PR review * Address PR comments * Add test to validate that Severity.NONE cannot be used * add null treatment for flushSeverity * Remove redundand null check --- .../com/google/cloud/logging/LogEntry.java | 3 ++ .../com/google/cloud/logging/Logging.java | 4 +- .../google/cloud/logging/LoggingHandler.java | 36 +++++++---------- .../com/google/cloud/logging/LoggingImpl.java | 11 ++++-- .../com/google/cloud/logging/Severity.java | 3 ++ .../google/cloud/logging/LogEntryTest.java | 10 +++++ .../cloud/logging/LoggingHandlerTest.java | 39 ++++++++++++++++++- 7 files changed, 78 insertions(+), 28 deletions(-) diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java index 45b8b9a30..5c5727d27 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java @@ -715,6 +715,9 @@ public String toStructuredJsonString() { if (payload.getType() == Payload.Type.PROTO) { throw new UnsupportedOperationException("LogEntry with protobuf payload cannot be converted"); } + if (severity == Severity.NONE) { + throw new IllegalArgumentException("Severity.NONE cannot be used for LogEntry"); + } final StringBuilder builder = new StringBuilder("{"); final StructuredLogFormatter formatter = new StructuredLogFormatter(builder); diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java index 014d71139..9d89e0eee 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Logging.java @@ -303,8 +303,8 @@ public static TailOption project(String project) { /** * Sets flush severity for asynchronous logging writes. It is disabled by default, enabled when - * this method is called with not null value. Logs will be immediately written out for entries at - * or higher than flush severity. + * this method is called with any {@link Severity} value other than {@link Severity.NONE}. Logs + * will be immediately written out for entries at or higher than flush severity. * *

Enabling this can cause the leaking and hanging threads, see BUG(2796) BUG(3880). However * you can explicitly call {@link #flush}. diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java index 07f9fcd9e..a0b176968 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java @@ -498,29 +498,21 @@ private static Severity severityFor(Level level) { if (level instanceof LoggingLevel) { return ((LoggingLevel) level).getSeverity(); } - - switch (level.intValue()) { - // FINEST - // FINER - // FINE - case 300: - case 400: - case 500: - return Severity.DEBUG; - // CONFIG - // INFO - case 700: - case 800: - return Severity.INFO; - // WARNING - case 900: - return Severity.WARNING; - // SEVERE - case 1000: - return Severity.ERROR; - default: - return Severity.DEFAULT; + // Choose the severity based on Level range. + // The assumption is that Level values below maintain same numeric value + int value = level.intValue(); + if (value <= Level.FINE.intValue()) { + return Severity.DEBUG; + } else if (value <= Level.INFO.intValue()) { + return Severity.INFO; + } else if (value <= Level.WARNING.intValue()) { + return Severity.WARNING; + } else if (value <= Level.SEVERE.intValue()) { + return Severity.ERROR; + } else if (value == Level.OFF.intValue()) { + return Severity.NONE; } + return Severity.DEFAULT; } private static boolean isTrueOrNull(Boolean b) { 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 index 48a5f0f85..e6bc7a1ec 100644 --- 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 @@ -107,7 +107,7 @@ class LoggingImpl extends BaseService implements Logging { private final Map> pendingWrites = new ConcurrentHashMap<>(); private volatile Synchronicity writeSynchronicity = Synchronicity.ASYNC; - private volatile Severity flushSeverity = null; + private volatile Severity flushSeverity = Severity.NONE; private boolean closed; private static Boolean emptyToBooleanFunction(Empty input) { @@ -138,7 +138,12 @@ public void setWriteSynchronicity(Synchronicity writeSynchronicity) { @Override public void setFlushSeverity(Severity flushSeverity) { - this.flushSeverity = flushSeverity; + // For backward compatibility we treat null as Severity.NONE + if (flushSeverity == null) { + this.flushSeverity = Severity.NONE; + } else { + this.flushSeverity = flushSeverity; + } } @Override @@ -882,7 +887,7 @@ public void write(Iterable logEntries, WriteOption... options) { options = Instrumentation.addPartialSuccessOption(options); } writeLogEntries(logEntries, options); - if (flushSeverity != null) { + if (flushSeverity != Severity.NONE) { for (LogEntry logEntry : logEntries) { // flush pending writes if log severity at or above flush severity if (logEntry.getSeverity().compareTo(flushSeverity) >= 0) { diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/Severity.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/Severity.java index ed3967b58..96218a4c8 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/Severity.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Severity.java @@ -26,6 +26,9 @@ */ public enum Severity { + /** The None severity level. Should not be used with log entries. */ + NONE(LogSeverity.UNRECOGNIZED), + /** The log entry has no assigned severity level. */ DEFAULT(LogSeverity.DEFAULT), diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java index 66b7850a2..341b75934 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/LogEntryTest.java @@ -394,4 +394,14 @@ public void testStructureLogPresentations() { public void testStructureLogPresentationWithProtobufPayload() { assertThrows(UnsupportedOperationException.class, () -> PROTO_ENTRY.toStructuredJsonString()); } + + @Test + public void testStructureLogInvalidSeverity() { + assertThrows( + IllegalArgumentException.class, + () -> PROTO_ENTRY.toBuilder().setSeverity(Severity.NONE).build().toPb(PROJECT)); + assertThrows( + IllegalArgumentException.class, + () -> STRING_ENTRY.toBuilder().setSeverity(Severity.NONE).build().toStructuredJsonString()); + } } diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java index 2b6af1053..c6267e22e 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java @@ -231,6 +231,12 @@ protected void after() { } } + static class CustomLevel extends Level { + CustomLevel() { + super("CUSTOM", 510); + } + } + @Rule public final OutputStreamPatcher outputStreamPatcher = new OutputStreamPatcher(); @Before @@ -241,7 +247,7 @@ public void setUp() { expect(options.getProjectId()).andStubReturn(PROJECT); expect(options.getService()).andStubReturn(logging); expect(options.getAutoPopulateMetadata()).andStubReturn(Boolean.FALSE); - logging.setFlushSeverity(EasyMock.anyObject(Severity.class)); + logging.setFlushSeverity(Severity.ERROR); expectLastCall().anyTimes(); logging.setWriteSynchronicity(EasyMock.anyObject(Synchronicity.class)); expectLastCall().anyTimes(); @@ -537,6 +543,37 @@ public void testFlushLevel() { handler.publish(newLogRecord(Level.WARNING, MESSAGE)); } + @Test + public void testFlushLevelOff() { + logging.setFlushSeverity(Severity.NONE); + expectLastCall().once(); + replay(options, logging); + LoggingHandler handler = new LoggingHandler(LOG_NAME, options, DEFAULT_RESOURCE); + handler.setFlushLevel(Level.OFF); + assertEquals(Level.OFF, handler.getFlushLevel()); + } + + @Test + public void testFlushLevelOn() { + logging.setFlushSeverity(Severity.WARNING); + expectLastCall().once(); + replay(options, logging); + LoggingHandler handler = new LoggingHandler(LOG_NAME, options, DEFAULT_RESOURCE); + handler.setFlushLevel(Level.WARNING); + assertEquals(Level.WARNING, handler.getFlushLevel()); + } + + @Test + public void testCustomFlushLevelOn() { + CustomLevel level = new CustomLevel(); + logging.setFlushSeverity(Severity.INFO); + expectLastCall().once(); + replay(options, logging); + LoggingHandler handler = new LoggingHandler(LOG_NAME, options, DEFAULT_RESOURCE); + handler.setFlushLevel(level); + assertEquals(level, handler.getFlushLevel()); + } + @Test public void testSyncWrite() { reset(logging);