From 69554f28a9fc2d3b05dc84c0591cb3f214a37ef1 Mon Sep 17 00:00:00 2001 From: losalex Date: Mon, 31 Oct 2022 16:59:36 -0700 Subject: [PATCH 1/3] fix: Make partialSuccess to be true by default --- .../com/google/cloud/logging/LoggingImpl.java | 10 ++++++-- .../google/cloud/logging/LoggingImplTest.java | 24 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) 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 8c813feb1..d98655b3e 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 @@ -859,6 +859,8 @@ public void write(Iterable logEntries, WriteOption... options) { try { final Map writeOptions = optionMap(options); final Boolean loggingOptionsPopulateFlag = getOptions().getAutoPopulateMetadata(); + final Boolean writeOptionPartialSuccessFlag = + WriteOption.OptionType.PARTIAL_SUCCESS.get(writeOptions); final Boolean writeOptionPopulateFlag = WriteOption.OptionType.AUTO_POPULATE_METADATA.get(writeOptions); Tuple> pair = @@ -872,9 +874,13 @@ public void write(Iterable logEntries, WriteOption... options) { logEntries = populateMetadata(logEntries, sharedResourceMetadata, this.getClass().getName()); } - // Add partialSuccess option always for request containing instrumentation data + // Add partialSuccess = true option always for request which does not have it set explicitly. + // For request containing instrumentation data always override it with true. writeLogEntries( - logEntries, pair.x() ? Instrumentation.addPartialSuccessOption(options) : options); + logEntries, + pair.x() || writeOptionPartialSuccessFlag == null + ? Instrumentation.addPartialSuccessOption(options) + : options); if (flushSeverity != null) { for (LogEntry logEntry : logEntries) { // flush pending writes if log severity at or above flush severity 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 index 9f230d60b..7504299ed 100644 --- 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 @@ -1851,6 +1851,7 @@ public void testWriteLogEntries() { .addAllEntries( Iterables.transform( ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response)); @@ -1869,6 +1870,7 @@ public void testWriteLogEntriesDoesNotEnableFlushByDefault() { ImmutableList.of( LOG_ENTRY1, LOG_ENTRY2.toBuilder().setSeverity(Severity.EMERGENCY).build()), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); ApiFuture apiFuture = SettableApiFuture.create(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(apiFuture); @@ -1886,6 +1888,7 @@ public void testWriteLogEntriesWithSeverityFlushEnabled() { .addAllEntries( Iterables.transform( ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response)); @@ -1934,6 +1937,7 @@ public void testWriteLogEntriesAsync() { ImmutableList.of( LOG_ENTRY1, LOG_ENTRY2, LOG_ENTRY_NO_DESTINATION, LOG_ENTRY_EMPTY), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response)); @@ -1955,6 +1959,7 @@ public void testWriteLogEntriesAsyncWithOptions() { .addAllEntries( Iterables.transform( ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response)); @@ -2229,6 +2234,7 @@ public void testFlush() throws InterruptedException { .addAllEntries( Iterables.transform( ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); EasyMock.expect(loggingRpcMock.write(request)).andReturn(mockRpcResponse); EasyMock.replay(loggingRpcMock); @@ -2264,6 +2270,7 @@ public void testFlushStress() throws InterruptedException { WriteLogEntriesRequest.newBuilder() .addAllEntries( Iterables.transform(ImmutableList.of(LOG_ENTRY1), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) .build(); Thread[] threads = new Thread[100]; @@ -2304,6 +2311,22 @@ public void testDiagnosticInfoWithPartialSuccess() { testDiagnosticInfoGeneration(true); } + @Test + public void testPartialSuccessNotOverridenIfPresent() { + WriteLogEntriesRequest request = + WriteLogEntriesRequest.newBuilder() + .addAllEntries( + Iterables.transform( + ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(false) + .build(); + WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); + EasyMock.expect(loggingRpcMock.write(request)).andReturn(ApiFutures.immediateFuture(response)); + EasyMock.replay(rpcFactoryMock, loggingRpcMock); + logging = options.getService(); + logging.write(ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2), WriteOption.partialSuccess(false)); + } + private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) { Instrumentation.setInstrumentationStatus(false); LogEntry jsonEntry = @@ -2362,6 +2385,7 @@ private void testWriteLogEntriesWithDestination( LOG_ENTRY_NO_DESTINATION, LOG_ENTRY_EMPTY), LogEntry.toPbFunction(projectId))) + .setPartialSuccess(true) .build(); WriteLogEntriesResponse response = WriteLogEntriesResponse.getDefaultInstance(); EasyMock.expect(loggingRpcMock.write(expectedWriteLogEntriesRequest)) From 8deb4b4768ba3eb01e9545d0a8ffc9cc9ac3f289 Mon Sep 17 00:00:00 2001 From: losalex Date: Tue, 1 Nov 2022 17:03:01 -0700 Subject: [PATCH 2/3] Address PR comments --- .../google/cloud/logging/Instrumentation.java | 24 ------------- .../com/google/cloud/logging/LoggingImpl.java | 36 +++++++++++++++---- 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java index f26d4003c..7e291cefc 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java @@ -19,17 +19,13 @@ import com.google.api.client.util.Strings; import com.google.api.gax.core.GaxProperties; import com.google.cloud.Tuple; -import com.google.cloud.logging.Logging.WriteOption; import com.google.cloud.logging.Payload.JsonPayload; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.protobuf.ListValue; import com.google.protobuf.Struct; import com.google.protobuf.Value; import java.util.ArrayList; -import java.util.Collections; import java.util.List; -import org.jspecify.nullness.Nullable; public final class Instrumentation { public static final String DIAGNOSTIC_INFO_KEY = "logging.googleapis.com/diagnostic"; @@ -91,26 +87,6 @@ public static Tuple> populateInstrumentationInfo( return Tuple.of(true, entries); } - /** - * Adds a partialSuccess flag option to array of WriteOption - * - * @param options {WriteOption[]} The options array to be extended - * @return The new array of oprions containing WriteOption.OptionType.PARTIAL_SUCCESS flag set to - * true - */ - public static WriteOption @Nullable [] addPartialSuccessOption(WriteOption[] options) { - if (options == null) { - return options; - } - List writeOptions = new ArrayList<>(); - Collections.addAll(writeOptions, options); - // Make sure we remove all partial success flags if any exist - writeOptions.removeIf( - option -> option.getOptionType() == WriteOption.OptionType.PARTIAL_SUCCESS); - writeOptions.add(WriteOption.partialSuccess(true)); - return Iterables.toArray(writeOptions, WriteOption.class); - } - /** * The helper method to generate a log entry with diagnostic instrumentation data. * 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 d98655b3e..ea69f2529 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 @@ -91,6 +91,7 @@ import com.google.protobuf.util.Durations; import java.text.ParseException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; @@ -874,13 +875,14 @@ public void write(Iterable logEntries, WriteOption... options) { logEntries = populateMetadata(logEntries, sharedResourceMetadata, this.getClass().getName()); } - // Add partialSuccess = true option always for request which does not have it set explicitly. - // For request containing instrumentation data always override it with true. - writeLogEntries( - logEntries, - pair.x() || writeOptionPartialSuccessFlag == null - ? Instrumentation.addPartialSuccessOption(options) - : options); + // Add partialSuccess = true option always for request which does not have + // it set explicitly in options. + // For request containing instrumentation data (e.g. when pair.x() == true), + // always set or override partialSuccess with true. + if (pair.x() || writeOptionPartialSuccessFlag == null) { + options = addPartialSuccessOption(options); + } + writeLogEntries(logEntries, options); if (flushSeverity != null) { for (LogEntry logEntry : logEntries) { // flush pending writes if log severity at or above flush severity @@ -1135,4 +1137,24 @@ public void close() throws Exception { int getNumPendingWrites() { return pendingWrites.size(); } + + /** + * Adds a partialSuccess flag option to array of WriteOption + * + * @param options {WriteOption[]} The options array to be extended + * @return The new array of oprions containing WriteOption.OptionType.PARTIAL_SUCCESS flag set to + * true + */ + static WriteOption @Nullable [] addPartialSuccessOption(WriteOption[] options) { + if (options == null) { + return options; + } + List writeOptions = new ArrayList<>(); + Collections.addAll(writeOptions, options); + // Make sure we remove all partial success flags if any exist + writeOptions.removeIf( + option -> option.getOptionType() == WriteOption.OptionType.PARTIAL_SUCCESS); + writeOptions.add(WriteOption.partialSuccess(true)); + return Iterables.toArray(writeOptions, WriteOption.class); + } } From 29056c230fbac3e41f559d60527898a5ace80593 Mon Sep 17 00:00:00 2001 From: losalex Date: Tue, 1 Nov 2022 17:10:01 -0700 Subject: [PATCH 3/3] Move back Instrumentation.addPartialSuccessOption --- .../google/cloud/logging/Instrumentation.java | 24 +++++++++++++++++++ .../com/google/cloud/logging/LoggingImpl.java | 23 +----------------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java index 7e291cefc..f26d4003c 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java @@ -19,13 +19,17 @@ import com.google.api.client.util.Strings; import com.google.api.gax.core.GaxProperties; import com.google.cloud.Tuple; +import com.google.cloud.logging.Logging.WriteOption; import com.google.cloud.logging.Payload.JsonPayload; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.protobuf.ListValue; import com.google.protobuf.Struct; import com.google.protobuf.Value; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import org.jspecify.nullness.Nullable; public final class Instrumentation { public static final String DIAGNOSTIC_INFO_KEY = "logging.googleapis.com/diagnostic"; @@ -87,6 +91,26 @@ public static Tuple> populateInstrumentationInfo( return Tuple.of(true, entries); } + /** + * Adds a partialSuccess flag option to array of WriteOption + * + * @param options {WriteOption[]} The options array to be extended + * @return The new array of oprions containing WriteOption.OptionType.PARTIAL_SUCCESS flag set to + * true + */ + public static WriteOption @Nullable [] addPartialSuccessOption(WriteOption[] options) { + if (options == null) { + return options; + } + List writeOptions = new ArrayList<>(); + Collections.addAll(writeOptions, options); + // Make sure we remove all partial success flags if any exist + writeOptions.removeIf( + option -> option.getOptionType() == WriteOption.OptionType.PARTIAL_SUCCESS); + writeOptions.add(WriteOption.partialSuccess(true)); + return Iterables.toArray(writeOptions, WriteOption.class); + } + /** * The helper method to generate a log entry with diagnostic instrumentation data. * 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 ea69f2529..48a5f0f85 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 @@ -91,7 +91,6 @@ import com.google.protobuf.util.Durations; import java.text.ParseException; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; @@ -880,7 +879,7 @@ public void write(Iterable logEntries, WriteOption... options) { // For request containing instrumentation data (e.g. when pair.x() == true), // always set or override partialSuccess with true. if (pair.x() || writeOptionPartialSuccessFlag == null) { - options = addPartialSuccessOption(options); + options = Instrumentation.addPartialSuccessOption(options); } writeLogEntries(logEntries, options); if (flushSeverity != null) { @@ -1137,24 +1136,4 @@ public void close() throws Exception { int getNumPendingWrites() { return pendingWrites.size(); } - - /** - * Adds a partialSuccess flag option to array of WriteOption - * - * @param options {WriteOption[]} The options array to be extended - * @return The new array of oprions containing WriteOption.OptionType.PARTIAL_SUCCESS flag set to - * true - */ - static WriteOption @Nullable [] addPartialSuccessOption(WriteOption[] options) { - if (options == null) { - return options; - } - List writeOptions = new ArrayList<>(); - Collections.addAll(writeOptions, options); - // Make sure we remove all partial success flags if any exist - writeOptions.removeIf( - option -> option.getOptionType() == WriteOption.OptionType.PARTIAL_SUCCESS); - writeOptions.add(WriteOption.partialSuccess(true)); - return Iterables.toArray(writeOptions, WriteOption.class); - } }