From be250c30bfbec0da0983b335c7bd65c542b9a52d Mon Sep 17 00:00:00 2001 From: losalex Date: Fri, 17 Jun 2022 14:32:23 -0700 Subject: [PATCH 1/9] feat: Add support for library instrumentation --- .../google/cloud/logging/Instrumentation.java | 183 ++++++++++++++++++ .../com/google/cloud/logging/Logging.java | 12 +- .../google/cloud/logging/LoggingHandler.java | 15 +- .../com/google/cloud/logging/LoggingImpl.java | 3 +- .../cloud/logging/InstrumentationTest.java | 141 ++++++++++++++ .../com/google/cloud/logging/LoggingTest.java | 5 + 6 files changed, 355 insertions(+), 4 deletions(-) create mode 100644 google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java create mode 100644 google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java 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 new file mode 100644 index 000000000..a312b04a2 --- /dev/null +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java @@ -0,0 +1,183 @@ +/* + * Copyright 2022 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 com.google.api.client.util.Strings; +import com.google.api.gax.core.GaxProperties; +import com.google.cloud.Tuple; +import com.google.cloud.logging.Payload.JsonPayload; +import com.google.cloud.logging.Payload.Type; +import com.google.common.collect.ImmutableMap; +import com.google.protobuf.ListValue; +import com.google.protobuf.Struct; +import com.google.protobuf.Value; +import java.util.ArrayList; +import java.util.List; + +public class Instrumentation { + public static final String DIAGNOSTIC_INFO_KEY = "logging.googleapis.com/diagnostic"; + public static final String INSTRUMENTATION_SOURCE_KEY = "instrumentation_source"; + public static final String INSTRUMENTATION_NAME_KEY = "name"; + public static final String INSTRUMENTATION_VERSION_KEY = "version"; + public static final String JAVA_LIBRARY_NAME_PREFIX = "java"; + public static final String DEFAULT_INSTRUMENTATION_VERSION = "UNKNOWN"; + public static final int MAX_DIAGNOSTIC_VALUE_LENGTH = 14; + public static final int MAX_DIAGNOSTIC_ENTIES = 3; + private static boolean instrumentationAdded = false; + + public static Tuple> populateInstrumentationInfo( + Iterable logEntries) { + boolean isWritten = setInstrumentationStatus(true); + List entries = new ArrayList<>(); + boolean isInfoAdded = false; + for (LogEntry logEntry : logEntries) { + // Check if LogEntry has a proper payload and also contains a diagnostic entry + if (logEntry.getPayload().getType() == Type.JSON + && logEntry + .getPayload() + .getData() + .containsFields(DIAGNOSTIC_INFO_KEY)) { + try { + ListValue infoList = + logEntry + .getPayload() + .getData() + .getFieldsOrThrow(DIAGNOSTIC_INFO_KEY) + .getStructValue() + .getFieldsOrThrow(INSTRUMENTATION_SOURCE_KEY) + .getListValue(); + entries.add(createDiagnosticEntry(null, null, infoList)); + isInfoAdded = isWritten = true; + } catch (Exception ex) { + System.err.println("ERROR: unexpected exception in populateInstrumentationInfo: " + ex); + } + } else { + entries.add(logEntry); + } + } + if (!isWritten) { + entries.add(createDiagnosticEntry(null, null, null)); + isInfoAdded = true; + } + return Tuple.of(isInfoAdded, entries); + } + + /** + * The helper method to generate a log entry with diagnostic instrumentation data. + * + * @param libraryName {string} The name of the logging library to be reported. Should be prefixed + * with 'java'. Will be truncated if longer than 14 characters. + * @param libraryVersion {string} The version of the logging library to be reported. Will be + * truncated if longer than 14 characters. + * @returns {LogEntry} The entry with diagnostic instrumentation data. + */ + public static LogEntry createDiagnosticEntry(String libraryName, String libraryVersion) { + return createDiagnosticEntry(libraryName, libraryVersion, null); + } + + private static LogEntry createDiagnosticEntry( + String libraryName, String libraryVersion, ListValue existingLibraryList) { + Struct instrumentation = + Struct.newBuilder() + .putAllFields( + ImmutableMap.of( + INSTRUMENTATION_SOURCE_KEY, + Value.newBuilder() + .setListValue( + generateLibrariesList(libraryName, libraryVersion, existingLibraryList)) + .build())) + .build(); + LogEntry entry = + LogEntry.of( + JsonPayload.of( + Struct.newBuilder() + .putAllFields( + ImmutableMap.of( + DIAGNOSTIC_INFO_KEY, + Value.newBuilder().setStructValue(instrumentation).build())) + .build())); + return entry; + } + + private static ListValue generateLibrariesList( + String libraryName, String libraryVersion, ListValue existingLibraryList) { + if (Strings.isNullOrEmpty(libraryName) || !libraryName.startsWith(JAVA_LIBRARY_NAME_PREFIX)) + libraryName = JAVA_LIBRARY_NAME_PREFIX; + if (Strings.isNullOrEmpty(libraryVersion)) { + libraryVersion = GaxProperties.getLibraryVersion(Instrumentation.class.getClass()); + if (Strings.isNullOrEmpty(libraryVersion)) libraryVersion = DEFAULT_INSTRUMENTATION_VERSION; + } + Struct libraryInfo = createInfoStruct(libraryName, libraryVersion); + ListValue.Builder libraryList = ListValue.newBuilder(); + // Append first the library info for this library + libraryList.addValues(Value.newBuilder().setStructValue(libraryInfo).build()); + if (existingLibraryList != null) { + for (Value val : existingLibraryList.getValuesList()) { + if (val.hasStructValue()) { + try { + String name = + val.getStructValue().getFieldsOrThrow(INSTRUMENTATION_NAME_KEY).getStringValue(); + if (Strings.isNullOrEmpty(name) || !name.startsWith(JAVA_LIBRARY_NAME_PREFIX)) continue; + String version = + val.getStructValue().getFieldsOrThrow(INSTRUMENTATION_VERSION_KEY).getStringValue(); + if (Strings.isNullOrEmpty(version)) continue; + libraryList.addValues( + Value.newBuilder().setStructValue(createInfoStruct(name, version)).build()); + if (libraryList.getValuesCount() == MAX_DIAGNOSTIC_ENTIES) break; + } catch (Exception ex) { + System.err.println("ERROR: unexpected exception in generateLibrariesList: " + ex); + } + } + } + } + return libraryList.build(); + } + + private static Struct createInfoStruct(String libraryName, String libraryVersion) { + return Struct.newBuilder() + .putAllFields( + ImmutableMap.of( + INSTRUMENTATION_NAME_KEY, + Value.newBuilder().setStringValue(truncateValue(libraryName)).build(), + INSTRUMENTATION_VERSION_KEY, + Value.newBuilder().setStringValue(truncateValue(libraryVersion)).build())) + .build(); + } + + /** + * The helper method used to set a status of a flag which indicates if instrumentation info + * already written or not. + * + * @param value {boolean} The value to be set. + * @returns The value of the flag before it is set. + */ + public static boolean setInstrumentationStatus(boolean value) { + if (instrumentationAdded == value) return instrumentationAdded; + return setAndGetInstrumentationStatus(value); + } + + private static synchronized boolean setAndGetInstrumentationStatus(boolean value) { + boolean current = instrumentationAdded; + instrumentationAdded = value; + return current; + } + + private static String truncateValue(String value) { + if (Strings.isNullOrEmpty(value) || value.length() < MAX_DIAGNOSTIC_VALUE_LENGTH) return value; + return value.substring(0, MAX_DIAGNOSTIC_VALUE_LENGTH) + "*"; + } +} 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 a765c73e0..832c61137 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 @@ -71,7 +71,8 @@ enum OptionType implements Option.OptionType { RESOURCE, LABELS, LOG_DESTINATION, - AUTO_POPULATE_METADATA; + AUTO_POPULATE_METADATA, + PARTIAL_SUCCESS; @SuppressWarnings("unchecked") T get(Map options) { @@ -123,6 +124,15 @@ public static WriteOption destination(LogDestinationName destination) { public static WriteOption autoPopulateMetadata(boolean autoPopulateMetadata) { return new WriteOption(OptionType.AUTO_POPULATE_METADATA, autoPopulateMetadata); } + + /** + * Returns an option to set partialSuccess flag. See {@link + * https://cloud.google.com/logging/docs/reference/v2/rest/v2/entries/write#body.request_body.FIELDS.partial_success} + * for more details. + */ + public static WriteOption partialSuccess(boolean partialSuccess) { + return new WriteOption(OptionType.PARTIAL_SUCCESS, partialSuccess); + } } /** Fields according to which log entries can be sorted. */ 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 92b9f8794..964d1274a 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 @@ -19,6 +19,7 @@ import static com.google.common.base.MoreObjects.firstNonNull; import com.google.cloud.MonitoredResource; +import com.google.cloud.Tuple; import com.google.cloud.logging.Logging.WriteOption; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -312,7 +313,9 @@ public void publish(LogRecord record) { } if (logEntry != null) { try { - Iterable logEntries = ImmutableList.of(logEntry); + Tuple> pair = + Instrumentation.populateInstrumentationInfo(ImmutableList.of(logEntry)); + Iterable logEntries = pair.y(); if (autoPopulateMetadata) { logEntries = logging.populateMetadata( @@ -321,7 +324,15 @@ public void publish(LogRecord record) { if (redirectToStdout) { logEntries.forEach(log -> System.out.println(log.toStructuredJsonString())); } else { - logging.write(logEntries, defaultWriteOptions); + // Add partialSuccess option always for request containing instrumentation data + if (pair.x()) { + List writeOptions = new ArrayList(); + writeOptions.addAll(Arrays.asList(defaultWriteOptions)); + writeOptions.add(WriteOption.partialSuccess(true)); + logging.write(logEntries, Iterables.toArray(writeOptions, WriteOption.class)); + } else { + logging.write(logEntries, defaultWriteOptions); + } } } catch (Exception ex) { getErrorManager().error(null, ex, ErrorManager.WRITE_FAILURE); 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 c75658c11..9b5558a0d 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 @@ -23,6 +23,7 @@ import static com.google.cloud.logging.Logging.WriteOption.OptionType.LABELS; import static com.google.cloud.logging.Logging.WriteOption.OptionType.LOG_DESTINATION; import static com.google.cloud.logging.Logging.WriteOption.OptionType.LOG_NAME; +import static com.google.cloud.logging.Logging.WriteOption.OptionType.PARTIAL_SUCCESS; import static com.google.cloud.logging.Logging.WriteOption.OptionType.RESOURCE; import static com.google.common.base.Preconditions.checkNotNull; @@ -92,7 +93,6 @@ import java.util.concurrent.TimeoutException; class LoggingImpl extends BaseService implements Logging { - protected static final String RESOURCE_NAME_FORMAT = "projects/%s/traces/%s"; private static final int FLUSH_WAIT_TIMEOUT_SECONDS = 6; private final LoggingRpc rpc; @@ -774,6 +774,7 @@ private static WriteLogEntriesRequest writeLogEntriesRequest( builder.putAllLabels(labels); } + builder.setPartialSuccess(Boolean.TRUE.equals(PARTIAL_SUCCESS.get(options))); builder.addAllEntries(Iterables.transform(logEntries, LogEntry.toPbFunction(projectId))); return builder.build(); } diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java new file mode 100644 index 000000000..28afaa930 --- /dev/null +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java @@ -0,0 +1,141 @@ +/* + * Copyright 2022 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 com.google.api.client.util.Lists; +import com.google.cloud.Tuple; +import com.google.cloud.logging.Payload.JsonPayload; +import com.google.cloud.logging.Payload.StringPayload; +import com.google.cloud.logging.Payload.Type; +import com.google.common.collect.ImmutableList; +import com.google.protobuf.ListValue; +import com.google.protobuf.Value; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import org.junit.Assert; +import org.junit.Test; + +public class InstrumentationTest { + private static final StringPayload STRING_PAYLOAD = StringPayload.of("payload"); + private static final LogEntry STRING_ENTRY = LogEntry.newBuilder(STRING_PAYLOAD).build(); + private static final String JAVA_OTHER_NAME = "java-other"; + private static final String JAVA_INVALID_NAME = "no-java-name"; + private static final String JAVA_OTHER_VERSION = "1.0.0"; + + @Test + public void testInstrumentationGenerated() { + Instrumentation.setInstrumentationStatus(false); + verifyEntries( + Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)), + 1, + 2, + new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)), + new HashSet<>(Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION))); + } + + @Test + public void testNoInstrumentationGenerated() { + Instrumentation.setInstrumentationStatus(true); + Tuple> pair = + Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)); + ArrayList entries = Lists.newArrayList(pair.y()); + Assert.assertFalse(pair.x()); + Assert.assertEquals(entries.size(), 1); + Assert.assertTrue(entries.get(0).getPayload().getType() == Type.STRING); + } + + @Test + public void testInstrumentationUpdated() { + Instrumentation.setInstrumentationStatus(false); + LogEntry json_entry = + LogEntry.newBuilder(generateInstrumentationPayload(JAVA_OTHER_NAME, JAVA_OTHER_VERSION)) + .build(); + verifyEntries( + Instrumentation.populateInstrumentationInfo(ImmutableList.of(json_entry)), + 0, + 1, + new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX, JAVA_OTHER_NAME)), + new HashSet<>( + Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION, JAVA_OTHER_VERSION))); + } + + @Test + public void testInvalidInstrumentationRemoved() { + Instrumentation.setInstrumentationStatus(false); + LogEntry json_entry = + LogEntry.newBuilder(generateInstrumentationPayload(JAVA_INVALID_NAME, JAVA_OTHER_VERSION)) + .build(); + verifyEntries( + Instrumentation.populateInstrumentationInfo(ImmutableList.of(json_entry)), + 0, + 1, + new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)), + new HashSet<>(Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION))); + } + + public static JsonPayload generateInstrumentationPayload( + String libraryName, String libraryVersion) { + Map json_data = new HashMap<>(); + Map instrumentation_data = new HashMap<>(); + Map info = new HashMap<>(); + info.put(Instrumentation.INSTRUMENTATION_NAME_KEY, libraryName); + info.put(Instrumentation.INSTRUMENTATION_VERSION_KEY, libraryVersion); + List list = ImmutableList.of(info); + instrumentation_data.put(Instrumentation.INSTRUMENTATION_SOURCE_KEY, list); + json_data.put(Instrumentation.DIAGNOSTIC_INFO_KEY, instrumentation_data); + return JsonPayload.of(json_data); + } + + private static void verifyEntries( + Tuple> pair, + int index, + int expected, + HashSet names, + HashSet versions) { + ArrayList entries = Lists.newArrayList(pair.y()); + Assert.assertTrue(pair.x()); + Assert.assertEquals(entries.size(), expected); + Assert.assertTrue(entries.get(index).getPayload().getType() == Type.JSON); + ListValue infoList = + entries + .get(index) + .getPayload() + .getData() + .getFieldsOrThrow(Instrumentation.DIAGNOSTIC_INFO_KEY) + .getStructValue() + .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_SOURCE_KEY) + .getListValue(); + for (Value val : infoList.getValuesList()) { + Assert.assertTrue( + names.remove( + val.getStructValue() + .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_NAME_KEY) + .getStringValue())); + Assert.assertTrue( + versions.remove( + val.getStructValue() + .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_VERSION_KEY) + .getStringValue())); + } + Assert.assertEquals(names.size(), 0); + Assert.assertEquals(versions.size(), 0); + } +} diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingTest.java index fab23f972..ef87bb13d 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingTest.java @@ -44,6 +44,7 @@ public class LoggingTest { private static final String ORGANIZATION_NAME = "organization"; private static final String BILLING_NAME = "billing"; private static final Boolean DONT_AUTO_POPULATE_METADATA = false; + private static final Boolean DO_PARTIAL_SUCCESS = false; @Test public void testListOption() { @@ -114,6 +115,10 @@ public void testWriteOption() { writeOption = WriteOption.autoPopulateMetadata(DONT_AUTO_POPULATE_METADATA); assertEquals(DONT_AUTO_POPULATE_METADATA, writeOption.getValue()); assertEquals(WriteOption.OptionType.AUTO_POPULATE_METADATA, writeOption.getOptionType()); + + writeOption = WriteOption.partialSuccess(DO_PARTIAL_SUCCESS); + assertEquals(DO_PARTIAL_SUCCESS, writeOption.getValue()); + assertEquals(WriteOption.OptionType.PARTIAL_SUCCESS, writeOption.getOptionType()); } @Test From 09b308f75e4193f50797f394418235f34f69179b Mon Sep 17 00:00:00 2001 From: losalex Date: Fri, 17 Jun 2022 14:40:29 -0700 Subject: [PATCH 2/9] Print instrumentation record only once --- .../com/google/cloud/logging/Instrumentation.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 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 a312b04a2..bc4b5df1b 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 @@ -42,11 +42,13 @@ public class Instrumentation { public static Tuple> populateInstrumentationInfo( Iterable logEntries) { boolean isWritten = setInstrumentationStatus(true); + if (isWritten) return Tuple.of(false, logEntries); List entries = new ArrayList<>(); - boolean isInfoAdded = false; + for (LogEntry logEntry : logEntries) { // Check if LogEntry has a proper payload and also contains a diagnostic entry - if (logEntry.getPayload().getType() == Type.JSON + if (!isWritten + && logEntry.getPayload().getType() == Type.JSON && logEntry .getPayload() .getData() @@ -61,7 +63,7 @@ public static Tuple> populateInstrumentationInfo( .getFieldsOrThrow(INSTRUMENTATION_SOURCE_KEY) .getListValue(); entries.add(createDiagnosticEntry(null, null, infoList)); - isInfoAdded = isWritten = true; + isWritten = true; } catch (Exception ex) { System.err.println("ERROR: unexpected exception in populateInstrumentationInfo: " + ex); } @@ -71,9 +73,8 @@ public static Tuple> populateInstrumentationInfo( } if (!isWritten) { entries.add(createDiagnosticEntry(null, null, null)); - isInfoAdded = true; } - return Tuple.of(isInfoAdded, entries); + return Tuple.of(true, entries); } /** From 2a597ba594a8d6f3e0e36e5c2a48b3ec348e45ba Mon Sep 17 00:00:00 2001 From: losalex Date: Tue, 21 Jun 2022 11:28:51 -0700 Subject: [PATCH 3/9] Fix test --- .../cloud/logging/LoggingHandlerTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) 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 dbe3f7f5f..b307a8ba2 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 @@ -201,6 +201,7 @@ public void enhanceLogEntry(LogEntry.Builder builder) { @Before public void setUp() { + Instrumentation.setInstrumentationStatus(true); logging = EasyMock.createMock(Logging.class); options = EasyMock.createMock(LoggingOptions.class); expect(options.getProjectId()).andStubReturn(PROJECT); @@ -628,6 +629,29 @@ public void testRedirectToStdoutDisabled() { System.setOut(null); } + @Test + public void testDiagnosticInfo() { + Instrumentation.setInstrumentationStatus(false); + LogEntry json_entry = + LogEntry.newBuilder( + InstrumentationTest.generateInstrumentationPayload( + Instrumentation.JAVA_LIBRARY_NAME_PREFIX, + Instrumentation.DEFAULT_INSTRUMENTATION_VERSION)) + .build(); + logging.write( + ImmutableList.of(FINEST_ENTRY, json_entry), + WriteOption.logName(LOG_NAME), + WriteOption.resource(DEFAULT_RESOURCE), + WriteOption.labels(BASE_SEVERITY_MAP), + WriteOption.partialSuccess(true)); + expectLastCall().once(); + replay(options, logging); + LoggingHandler handler = new LoggingHandler(LOG_NAME, options, DEFAULT_RESOURCE); + handler.setLevel(Level.ALL); + handler.setFormatter(new TestFormatter()); + handler.publish(newLogRecord(Level.FINEST, MESSAGE)); + } + private void testPublishCustomResourceWithDestination( LogEntry entry, LogDestinationName destination) { MonitoredResource resource = MonitoredResource.of("custom", ImmutableMap.of()); From 553ce1f6eb3e214de27fa9a50881c38d5670ad21 Mon Sep 17 00:00:00 2001 From: losalex Date: Tue, 21 Jun 2022 18:22:23 -0700 Subject: [PATCH 4/9] Fix tests --- .../google/cloud/logging/Instrumentation.java | 16 ++++++++++++++-- .../cloud/logging/InstrumentationTest.java | 10 +++++++--- .../google/cloud/logging/LoggingHandlerTest.java | 2 +- 3 files changed, 22 insertions(+), 6 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 bc4b5df1b..faff1f683 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 @@ -119,8 +119,7 @@ private static ListValue generateLibrariesList( if (Strings.isNullOrEmpty(libraryName) || !libraryName.startsWith(JAVA_LIBRARY_NAME_PREFIX)) libraryName = JAVA_LIBRARY_NAME_PREFIX; if (Strings.isNullOrEmpty(libraryVersion)) { - libraryVersion = GaxProperties.getLibraryVersion(Instrumentation.class.getClass()); - if (Strings.isNullOrEmpty(libraryVersion)) libraryVersion = DEFAULT_INSTRUMENTATION_VERSION; + libraryVersion = getLibraryVersion(Instrumentation.class.getClass()); } Struct libraryInfo = createInfoStruct(libraryName, libraryVersion); ListValue.Builder libraryList = ListValue.newBuilder(); @@ -171,6 +170,19 @@ public static boolean setInstrumentationStatus(boolean value) { return setAndGetInstrumentationStatus(value); } + /** + * Returns a library version associated with given class + * + * @param libraryClass {Class} The class to be used to determine a library version + * @return The version number string for given class or "UNKNOWN" if class library version cannot + * be detected + */ + public static String getLibraryVersion(Class libraryClass) { + String libraryVersion = GaxProperties.getLibraryVersion(libraryClass); + if (Strings.isNullOrEmpty(libraryVersion)) libraryVersion = DEFAULT_INSTRUMENTATION_VERSION; + return libraryVersion; + } + private static synchronized boolean setAndGetInstrumentationStatus(boolean value) { boolean current = instrumentationAdded; instrumentationAdded = value; diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java index 28afaa930..5838fa80f 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java @@ -48,7 +48,8 @@ public void testInstrumentationGenerated() { 1, 2, new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)), - new HashSet<>(Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION))); + new HashSet<>( + Arrays.asList(Instrumentation.getLibraryVersion(Instrumentation.class.getClass())))); } @Test @@ -74,7 +75,9 @@ public void testInstrumentationUpdated() { 1, new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX, JAVA_OTHER_NAME)), new HashSet<>( - Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION, JAVA_OTHER_VERSION))); + Arrays.asList( + Instrumentation.getLibraryVersion(Instrumentation.class.getClass()), + JAVA_OTHER_VERSION))); } @Test @@ -88,7 +91,8 @@ public void testInvalidInstrumentationRemoved() { 0, 1, new HashSet<>(Arrays.asList(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)), - new HashSet<>(Arrays.asList(Instrumentation.DEFAULT_INSTRUMENTATION_VERSION))); + new HashSet<>( + Arrays.asList(Instrumentation.getLibraryVersion(Instrumentation.class.getClass())))); } public static JsonPayload generateInstrumentationPayload( 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 b307a8ba2..209c3e23d 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 @@ -636,7 +636,7 @@ public void testDiagnosticInfo() { LogEntry.newBuilder( InstrumentationTest.generateInstrumentationPayload( Instrumentation.JAVA_LIBRARY_NAME_PREFIX, - Instrumentation.DEFAULT_INSTRUMENTATION_VERSION)) + Instrumentation.getLibraryVersion(Instrumentation.class.getClass()))) .build(); logging.write( ImmutableList.of(FINEST_ENTRY, json_entry), From f8388ac7d4dda85d61b463d80424b4a63b881f44 Mon Sep 17 00:00:00 2001 From: losalex Date: Tue, 21 Jun 2022 19:11:01 -0700 Subject: [PATCH 5/9] Fix integration test --- .../cloud/logging/it/ITJulLoggerTest.java | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java index 9f4853628..0b55c2e06 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java @@ -26,6 +26,7 @@ import com.google.cloud.MonitoredResource; import com.google.cloud.logging.BaseSystemTest; +import com.google.cloud.logging.Instrumentation; import com.google.cloud.logging.LogEntry; import com.google.cloud.logging.LoggingHandler; import com.google.cloud.logging.LoggingOptions; @@ -34,6 +35,8 @@ import com.google.cloud.logging.Synchronicity; import com.google.common.collect.ImmutableMap; import com.google.logging.v2.LogName; +import com.google.protobuf.ListValue; +import com.google.protobuf.Value; import java.util.Iterator; import java.util.logging.Level; import java.util.logging.Logger; @@ -82,7 +85,24 @@ public void testLoggingHandler() throws InterruptedException { assertThat(entry.getOperation()).isNull(); assertThat(entry.getInsertId()).isNotNull(); assertThat(entry.getTimestamp()).isNotNull(); - assertThat(iterator.hasNext()).isFalse(); + assertThat(iterator.hasNext()).isTrue(); + entry = iterator.next(); + ListValue infoList = + entry + .getPayload() + .getData() + .getFieldsOrThrow(Instrumentation.DIAGNOSTIC_INFO_KEY) + .getStructValue() + .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_SOURCE_KEY) + .getListValue(); + for (Value val : infoList.getValuesList()) { + assertThat( + val.getStructValue() + .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_NAME_KEY) + .getStringValue() + .startsWith(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)) + .isTrue(); + } logger.removeHandler(handler); } From f0759d6a17c64c29e5593fe885bb54f61e8851dd Mon Sep 17 00:00:00 2001 From: losalex Date: Thu, 23 Jun 2022 16:10:27 -0700 Subject: [PATCH 6/9] Address PR comments, fix tests --- .../google/cloud/logging/Instrumentation.java | 43 +++++++++++++++---- .../google/cloud/logging/LoggingHandler.java | 18 +++----- .../com/google/cloud/logging/LoggingImpl.java | 9 +++- .../cloud/logging/LoggingHandlerTest.java | 23 ---------- .../google/cloud/logging/LoggingImplTest.java | 36 ++++++++++++++++ 5 files changed, 82 insertions(+), 47 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 faff1f683..0ce775c44 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,16 @@ 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.cloud.logging.Payload.Type; 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.Arrays; import java.util.List; public class Instrumentation { @@ -38,7 +41,15 @@ public class Instrumentation { public static final int MAX_DIAGNOSTIC_VALUE_LENGTH = 14; public static final int MAX_DIAGNOSTIC_ENTIES = 3; private static boolean instrumentationAdded = false; + private static Object instrumentationLock = new Object(); + /** + * Populates entries with instrumentation info which is added in separate log entry + * + * @param logEntries {Iterable} The list of entries to be populated + * @return {Tuple>} containg a flag if instrumentation info was added + * or not and a modified list of log entries + */ public static Tuple> populateInstrumentationInfo( Iterable logEntries) { boolean isWritten = setInstrumentationStatus(true); @@ -77,6 +88,23 @@ 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[] addPartialSuccessOption(WriteOption[] options) { + List writeOptions = new ArrayList(); + writeOptions.addAll(Arrays.asList(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. * @@ -139,7 +167,6 @@ private static ListValue generateLibrariesList( Value.newBuilder().setStructValue(createInfoStruct(name, version)).build()); if (libraryList.getValuesCount() == MAX_DIAGNOSTIC_ENTIES) break; } catch (Exception ex) { - System.err.println("ERROR: unexpected exception in generateLibrariesList: " + ex); } } } @@ -163,11 +190,15 @@ private static Struct createInfoStruct(String libraryName, String libraryVersion * already written or not. * * @param value {boolean} The value to be set. - * @returns The value of the flag before it is set. + * @returns The value of the flag before it was set. */ public static boolean setInstrumentationStatus(boolean value) { if (instrumentationAdded == value) return instrumentationAdded; - return setAndGetInstrumentationStatus(value); + synchronized (instrumentationLock) { + boolean current = instrumentationAdded; + instrumentationAdded = value; + return current; + } } /** @@ -183,12 +214,6 @@ public static String getLibraryVersion(Class libraryClass) { return libraryVersion; } - private static synchronized boolean setAndGetInstrumentationStatus(boolean value) { - boolean current = instrumentationAdded; - instrumentationAdded = value; - return current; - } - private static String truncateValue(String value) { if (Strings.isNullOrEmpty(value) || value.length() < MAX_DIAGNOSTIC_VALUE_LENGTH) return value; return value.substring(0, MAX_DIAGNOSTIC_VALUE_LENGTH) + "*"; 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 964d1274a..ffa4c6273 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 @@ -19,7 +19,6 @@ import static com.google.common.base.MoreObjects.firstNonNull; import com.google.cloud.MonitoredResource; -import com.google.cloud.Tuple; import com.google.cloud.logging.Logging.WriteOption; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -313,9 +312,10 @@ public void publish(LogRecord record) { } if (logEntry != null) { try { - Tuple> pair = - Instrumentation.populateInstrumentationInfo(ImmutableList.of(logEntry)); - Iterable logEntries = pair.y(); + Iterable logEntries = + redirectToStdout + ? Instrumentation.populateInstrumentationInfo(ImmutableList.of(logEntry)).y() + : ImmutableList.of(logEntry); if (autoPopulateMetadata) { logEntries = logging.populateMetadata( @@ -324,15 +324,7 @@ public void publish(LogRecord record) { if (redirectToStdout) { logEntries.forEach(log -> System.out.println(log.toStructuredJsonString())); } else { - // Add partialSuccess option always for request containing instrumentation data - if (pair.x()) { - List writeOptions = new ArrayList(); - writeOptions.addAll(Arrays.asList(defaultWriteOptions)); - writeOptions.add(WriteOption.partialSuccess(true)); - logging.write(logEntries, Iterables.toArray(writeOptions, WriteOption.class)); - } else { - logging.write(logEntries, defaultWriteOptions); - } + logging.write(logEntries, defaultWriteOptions); } } catch (Exception ex) { getErrorManager().error(null, ex, ErrorManager.WRITE_FAILURE); 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 9b5558a0d..a2078e079 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 @@ -40,6 +40,7 @@ import com.google.cloud.MonitoredResource; import com.google.cloud.MonitoredResourceDescriptor; import com.google.cloud.PageImpl; +import com.google.cloud.Tuple; import com.google.cloud.logging.spi.v2.LoggingRpc; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; @@ -852,6 +853,9 @@ public void write(Iterable logEntries, WriteOption... options) { final Boolean logingOptionsPopulateFlag = getOptions().getAutoPopulateMetadata(); final Boolean writeOptionPopulateFlga = WriteOption.OptionType.AUTO_POPULATE_METADATA.get(writeOptions); + Tuple> pair = + Instrumentation.populateInstrumentationInfo(logEntries); + logEntries = pair.y(); if (writeOptionPopulateFlga == Boolean.TRUE || (writeOptionPopulateFlga == null && logingOptionsPopulateFlag == Boolean.TRUE)) { @@ -859,8 +863,9 @@ public void write(Iterable logEntries, WriteOption... options) { logEntries = populateMetadata(logEntries, sharedResourceMetadata, this.getClass().getName()); } - - writeLogEntries(logEntries, options); + // Add partialSuccess option always for request containing instrumentation data + writeLogEntries( + logEntries, pair.x() ? 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/LoggingHandlerTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java index 209c3e23d..d55a3e786 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 @@ -629,29 +629,6 @@ public void testRedirectToStdoutDisabled() { System.setOut(null); } - @Test - public void testDiagnosticInfo() { - Instrumentation.setInstrumentationStatus(false); - LogEntry json_entry = - LogEntry.newBuilder( - InstrumentationTest.generateInstrumentationPayload( - Instrumentation.JAVA_LIBRARY_NAME_PREFIX, - Instrumentation.getLibraryVersion(Instrumentation.class.getClass()))) - .build(); - logging.write( - ImmutableList.of(FINEST_ENTRY, json_entry), - WriteOption.logName(LOG_NAME), - WriteOption.resource(DEFAULT_RESOURCE), - WriteOption.labels(BASE_SEVERITY_MAP), - WriteOption.partialSuccess(true)); - expectLastCall().once(); - replay(options, logging); - LoggingHandler handler = new LoggingHandler(LOG_NAME, options, DEFAULT_RESOURCE); - handler.setLevel(Level.ALL); - handler.setFormatter(new TestFormatter()); - handler.publish(newLogRecord(Level.FINEST, MESSAGE)); - } - private void testPublishCustomResourceWithDestination( LogEntry entry, LogDestinationName destination) { MonitoredResource resource = MonitoredResource.of("custom", ImmutableMap.of()); 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 68774e720..a7f6f4b98 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 @@ -252,6 +252,7 @@ private void configureListLogsTests( @Before public void setUp() { + Instrumentation.setInstrumentationStatus(true); rpcFactoryMock = EasyMock.createStrictMock(LoggingRpcFactory.class); loggingRpcMock = EasyMock.createStrictMock(LoggingRpc.class); EasyMock.expect(rpcFactoryMock.create(EasyMock.anyObject(LoggingOptions.class))) @@ -2288,6 +2289,41 @@ public void run() { assertSame(0, exceptions.get()); } + @Test + public void testDiagnosticInfoWithNoPartialSuccess() { + testDiagnosticInfoGeneration(false); + } + + @Test + public void testDiagnosticInfoWithPartialSuccess() { + testDiagnosticInfoGeneration(true); + } + + private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) { + Instrumentation.setInstrumentationStatus(false); + LogEntry json_entry = + LogEntry.newBuilder( + InstrumentationTest.generateInstrumentationPayload( + Instrumentation.JAVA_LIBRARY_NAME_PREFIX, + Instrumentation.getLibraryVersion(Instrumentation.class.getClass()))) + .build(); + WriteLogEntriesRequest request = + WriteLogEntriesRequest.newBuilder() + .addAllEntries( + Iterables.transform( + ImmutableList.of(LOG_ENTRY1, LOG_ENTRY2, json_entry), + LogEntry.toPbFunction(PROJECT))) + .setPartialSuccess(true) + .build(); + WriteLogEntriesResponse response = WriteLogEntriesResponse.newBuilder().build(); + 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(addPartialSuccessOption)); + } + private void testDeleteByDestination( String logId, String logName, LogDestinationName destination, boolean useAsyncDelete) throws ExecutionException, InterruptedException { From 7072d7da64edd625c1e49b9a4d52ca1fab88ac95 Mon Sep 17 00:00:00 2001 From: losalex Date: Fri, 24 Jun 2022 17:54:20 -0700 Subject: [PATCH 7/9] Address comments --- .../google/cloud/logging/Instrumentation.java | 25 ++++++++++++------- .../cloud/logging/InstrumentationTest.java | 8 +++--- .../cloud/logging/LoggingHandlerTest.java | 2 +- .../google/cloud/logging/LoggingImplTest.java | 4 +-- 4 files changed, 23 insertions(+), 16 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 0ce775c44..c4f9cf42c 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 @@ -52,7 +52,7 @@ public class Instrumentation { */ public static Tuple> populateInstrumentationInfo( Iterable logEntries) { - boolean isWritten = setInstrumentationStatus(true); + boolean isWritten = setInstrumentationStatus(); if (isWritten) return Tuple.of(false, logEntries); List entries = new ArrayList<>(); @@ -96,6 +96,7 @@ public static Tuple> populateInstrumentationInfo( * true */ public static WriteOption[] addPartialSuccessOption(WriteOption[] options) { + if (options == null) return options; List writeOptions = new ArrayList(); writeOptions.addAll(Arrays.asList(options)); // Make sure we remove all partial success flags if any exist @@ -186,21 +187,27 @@ private static Struct createInfoStruct(String libraryName, String libraryVersion } /** - * The helper method used to set a status of a flag which indicates if instrumentation info - * already written or not. + * The helper method used to set to true the flag which indicates if instrumentation info already + * written or not. * - * @param value {boolean} The value to be set. * @returns The value of the flag before it was set. */ - public static boolean setInstrumentationStatus(boolean value) { - if (instrumentationAdded == value) return instrumentationAdded; + public static boolean setInstrumentationStatus() { + if (instrumentationAdded) return instrumentationAdded; synchronized (instrumentationLock) { - boolean current = instrumentationAdded; - instrumentationAdded = value; - return current; + instrumentationAdded = true; + return false; } } + /** + * The package-private method to reset an instrumentation status to false to be used for testing + * purposes + */ + static void resetInstrumentationStatus() { + instrumentationAdded = false; + } + /** * Returns a library version associated with given class * diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java index 5838fa80f..02a4f20ee 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java @@ -42,7 +42,7 @@ public class InstrumentationTest { @Test public void testInstrumentationGenerated() { - Instrumentation.setInstrumentationStatus(false); + Instrumentation.resetInstrumentationStatus(); verifyEntries( Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)), 1, @@ -54,7 +54,7 @@ public void testInstrumentationGenerated() { @Test public void testNoInstrumentationGenerated() { - Instrumentation.setInstrumentationStatus(true); + Instrumentation.setInstrumentationStatus(); Tuple> pair = Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)); ArrayList entries = Lists.newArrayList(pair.y()); @@ -65,7 +65,7 @@ public void testNoInstrumentationGenerated() { @Test public void testInstrumentationUpdated() { - Instrumentation.setInstrumentationStatus(false); + Instrumentation.resetInstrumentationStatus(); LogEntry json_entry = LogEntry.newBuilder(generateInstrumentationPayload(JAVA_OTHER_NAME, JAVA_OTHER_VERSION)) .build(); @@ -82,7 +82,7 @@ public void testInstrumentationUpdated() { @Test public void testInvalidInstrumentationRemoved() { - Instrumentation.setInstrumentationStatus(false); + Instrumentation.resetInstrumentationStatus(); LogEntry json_entry = LogEntry.newBuilder(generateInstrumentationPayload(JAVA_INVALID_NAME, JAVA_OTHER_VERSION)) .build(); 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 d55a3e786..78ae3928d 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 @@ -201,7 +201,7 @@ public void enhanceLogEntry(LogEntry.Builder builder) { @Before public void setUp() { - Instrumentation.setInstrumentationStatus(true); + Instrumentation.setInstrumentationStatus(); logging = EasyMock.createMock(Logging.class); options = EasyMock.createMock(LoggingOptions.class); expect(options.getProjectId()).andStubReturn(PROJECT); 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 a7f6f4b98..80565f4d4 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 @@ -252,7 +252,7 @@ private void configureListLogsTests( @Before public void setUp() { - Instrumentation.setInstrumentationStatus(true); + Instrumentation.setInstrumentationStatus(); rpcFactoryMock = EasyMock.createStrictMock(LoggingRpcFactory.class); loggingRpcMock = EasyMock.createStrictMock(LoggingRpc.class); EasyMock.expect(rpcFactoryMock.create(EasyMock.anyObject(LoggingOptions.class))) @@ -2300,7 +2300,7 @@ public void testDiagnosticInfoWithPartialSuccess() { } private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) { - Instrumentation.setInstrumentationStatus(false); + Instrumentation.resetInstrumentationStatus(); LogEntry json_entry = LogEntry.newBuilder( InstrumentationTest.generateInstrumentationPayload( From 90565fc8c31b3a09bfd7613dc7d4772e0f933318 Mon Sep 17 00:00:00 2001 From: losalex Date: Fri, 24 Jun 2022 22:34:27 -0700 Subject: [PATCH 8/9] Fix tests and methods --- .../google/cloud/logging/Instrumentation.java | 23 +++++++------------ .../cloud/logging/InstrumentationTest.java | 8 +++---- .../cloud/logging/LoggingHandlerTest.java | 2 +- .../google/cloud/logging/LoggingImplTest.java | 4 ++-- .../cloud/logging/it/ITJulLoggerTest.java | 21 ----------------- 5 files changed, 15 insertions(+), 43 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 c4f9cf42c..f9d9a2bb4 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 @@ -52,7 +52,7 @@ public class Instrumentation { */ public static Tuple> populateInstrumentationInfo( Iterable logEntries) { - boolean isWritten = setInstrumentationStatus(); + boolean isWritten = setInstrumentationStatus(true); if (isWritten) return Tuple.of(false, logEntries); List entries = new ArrayList<>(); @@ -187,27 +187,20 @@ private static Struct createInfoStruct(String libraryName, String libraryVersion } /** - * The helper method used to set to true the flag which indicates if instrumentation info already - * written or not. + * The package-private helper method used to set the flag which indicates if instrumentation info + * already written or not. * * @returns The value of the flag before it was set. */ - public static boolean setInstrumentationStatus() { - if (instrumentationAdded) return instrumentationAdded; + static boolean setInstrumentationStatus(boolean value) { + if (instrumentationAdded == value) return instrumentationAdded; synchronized (instrumentationLock) { - instrumentationAdded = true; - return false; + boolean current = instrumentationAdded; + instrumentationAdded = value; + return current; } } - /** - * The package-private method to reset an instrumentation status to false to be used for testing - * purposes - */ - static void resetInstrumentationStatus() { - instrumentationAdded = false; - } - /** * Returns a library version associated with given class * diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java index 02a4f20ee..5838fa80f 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/InstrumentationTest.java @@ -42,7 +42,7 @@ public class InstrumentationTest { @Test public void testInstrumentationGenerated() { - Instrumentation.resetInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(false); verifyEntries( Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)), 1, @@ -54,7 +54,7 @@ public void testInstrumentationGenerated() { @Test public void testNoInstrumentationGenerated() { - Instrumentation.setInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(true); Tuple> pair = Instrumentation.populateInstrumentationInfo(ImmutableList.of(STRING_ENTRY)); ArrayList entries = Lists.newArrayList(pair.y()); @@ -65,7 +65,7 @@ public void testNoInstrumentationGenerated() { @Test public void testInstrumentationUpdated() { - Instrumentation.resetInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(false); LogEntry json_entry = LogEntry.newBuilder(generateInstrumentationPayload(JAVA_OTHER_NAME, JAVA_OTHER_VERSION)) .build(); @@ -82,7 +82,7 @@ public void testInstrumentationUpdated() { @Test public void testInvalidInstrumentationRemoved() { - Instrumentation.resetInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(false); LogEntry json_entry = LogEntry.newBuilder(generateInstrumentationPayload(JAVA_INVALID_NAME, JAVA_OTHER_VERSION)) .build(); 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 78ae3928d..d55a3e786 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 @@ -201,7 +201,7 @@ public void enhanceLogEntry(LogEntry.Builder builder) { @Before public void setUp() { - Instrumentation.setInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(true); logging = EasyMock.createMock(Logging.class); options = EasyMock.createMock(LoggingOptions.class); expect(options.getProjectId()).andStubReturn(PROJECT); 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 80565f4d4..a7f6f4b98 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 @@ -252,7 +252,7 @@ private void configureListLogsTests( @Before public void setUp() { - Instrumentation.setInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(true); rpcFactoryMock = EasyMock.createStrictMock(LoggingRpcFactory.class); loggingRpcMock = EasyMock.createStrictMock(LoggingRpc.class); EasyMock.expect(rpcFactoryMock.create(EasyMock.anyObject(LoggingOptions.class))) @@ -2300,7 +2300,7 @@ public void testDiagnosticInfoWithPartialSuccess() { } private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) { - Instrumentation.resetInstrumentationStatus(); + Instrumentation.setInstrumentationStatus(false); LogEntry json_entry = LogEntry.newBuilder( InstrumentationTest.generateInstrumentationPayload( diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java index 0b55c2e06..80f0e8a50 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/it/ITJulLoggerTest.java @@ -26,7 +26,6 @@ import com.google.cloud.MonitoredResource; import com.google.cloud.logging.BaseSystemTest; -import com.google.cloud.logging.Instrumentation; import com.google.cloud.logging.LogEntry; import com.google.cloud.logging.LoggingHandler; import com.google.cloud.logging.LoggingOptions; @@ -35,8 +34,6 @@ import com.google.cloud.logging.Synchronicity; import com.google.common.collect.ImmutableMap; import com.google.logging.v2.LogName; -import com.google.protobuf.ListValue; -import com.google.protobuf.Value; import java.util.Iterator; import java.util.logging.Level; import java.util.logging.Logger; @@ -85,24 +82,6 @@ public void testLoggingHandler() throws InterruptedException { assertThat(entry.getOperation()).isNull(); assertThat(entry.getInsertId()).isNotNull(); assertThat(entry.getTimestamp()).isNotNull(); - assertThat(iterator.hasNext()).isTrue(); - entry = iterator.next(); - ListValue infoList = - entry - .getPayload() - .getData() - .getFieldsOrThrow(Instrumentation.DIAGNOSTIC_INFO_KEY) - .getStructValue() - .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_SOURCE_KEY) - .getListValue(); - for (Value val : infoList.getValuesList()) { - assertThat( - val.getStructValue() - .getFieldsOrThrow(Instrumentation.INSTRUMENTATION_NAME_KEY) - .getStringValue() - .startsWith(Instrumentation.JAVA_LIBRARY_NAME_PREFIX)) - .isTrue(); - } logger.removeHandler(handler); } From dc3956ceef970174133a0dca9770ae5bed231508 Mon Sep 17 00:00:00 2001 From: losalex Date: Sat, 25 Jun 2022 00:18:26 -0700 Subject: [PATCH 9/9] Fix test failures and add default logName for instrumentation entry --- .../google/cloud/logging/Instrumentation.java | 19 +++++++++++-------- .../google/cloud/logging/LoggingImplTest.java | 1 + 2 files changed, 12 insertions(+), 8 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 f9d9a2bb4..8471d882e 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 @@ -38,6 +38,7 @@ public class Instrumentation { public static final String INSTRUMENTATION_VERSION_KEY = "version"; public static final String JAVA_LIBRARY_NAME_PREFIX = "java"; public static final String DEFAULT_INSTRUMENTATION_VERSION = "UNKNOWN"; + public static final String INSTRUMENTATION_LOG_NAME = "diagnostic-log"; public static final int MAX_DIAGNOSTIC_VALUE_LENGTH = 14; public static final int MAX_DIAGNOSTIC_ENTIES = 3; private static boolean instrumentationAdded = false; @@ -132,14 +133,16 @@ private static LogEntry createDiagnosticEntry( .build())) .build(); LogEntry entry = - LogEntry.of( - JsonPayload.of( - Struct.newBuilder() - .putAllFields( - ImmutableMap.of( - DIAGNOSTIC_INFO_KEY, - Value.newBuilder().setStructValue(instrumentation).build())) - .build())); + LogEntry.newBuilder( + JsonPayload.of( + Struct.newBuilder() + .putAllFields( + ImmutableMap.of( + DIAGNOSTIC_INFO_KEY, + Value.newBuilder().setStructValue(instrumentation).build())) + .build())) + .setLogName(INSTRUMENTATION_LOG_NAME) + .build(); return entry; } 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 a7f6f4b98..421f057e7 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 @@ -2306,6 +2306,7 @@ private void testDiagnosticInfoGeneration(boolean addPartialSuccessOption) { InstrumentationTest.generateInstrumentationPayload( Instrumentation.JAVA_LIBRARY_NAME_PREFIX, Instrumentation.getLibraryVersion(Instrumentation.class.getClass()))) + .setLogName(Instrumentation.INSTRUMENTATION_LOG_NAME) .build(); WriteLogEntriesRequest request = WriteLogEntriesRequest.newBuilder()