Logging refactor of JUL + monitored resource construction #1847

jabubake · 2017-04-03T16:07:57Z

Auto detection of select resources to get JUL working across GCP environments and refactoring of code to help support more adapters like Slf4j.
Testing across GCP in progress.
See Issue #1654

TODO : tests

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

+        value = MetadataConfig.getInstanceId();
+        break;
+      case "cluster_name":
+        value = MetadataConfig.getClusterName();


removing flush size config : needs to be better exposed in loggingoptions for control

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

@@ -174,6 +174,7 @@ public LoggingHandler(String log, LoggingOptions options, MonitoredResource moni
      this.flushLevel = config.getFlushLevel();
      String logName = firstNonNull(log, config.getLogName());

+      LoggingOptions loggingOptions = (options != null) ? options : LoggingOptions.getDefaultInstance();


garrettjonesgoogle

Just some quick initial comments.

google-cloud-core/src/main/java/com/google/cloud/MetadataConfig.java

+        }
+        return null;
+    }
+}


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

+ */
+
+package com.google.cloud.logging;
+/**


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

+import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class LoggingHelper {


michaelbausor

Hi @jabubake! I've just done an initial review - one thing is it looks like it would be good to run the Google Java formatter to clean up some of the indentation issues.

(cc @pongad who asked me to review)

google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java

-    // return null if can't determine
-    return null;
+    // return project id from metadata config
+     return MetadataConfig.getProjectId();


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

+  }
+
+  String getLogName() {
+    return getProperty( logFileNameTag, "java.log");


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

+  }
+}
+
+


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

-  private final List<Enhancer> enhancers;
+  private LoggingHelper loggingHelper;
+  private List<Enhancer> enhancers;
+  private  ErrorHandler errorHandler;


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

+
+      LoggingOptions loggingOptions = (options != null) ? options : LoggingOptions.getDefaultInstance();
+      if (options == null) {
+        options = LoggingOptions.getDefaultInstance();


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

+      this.flushLevel = config.getFlushLevel();
+      String logName = firstNonNull(log, config.getLogName());
+
+      LoggingOptions loggingOptions = (options != null) ? options : LoggingOptions.getDefaultInstance();


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

+    try {
+      logEntryBuilder = logEntryBuilderFor(record);
+    } catch (Exception ex) {
+      errorHandler.handleFormatError(ex);


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

+      loggingHelper.close();
+  }
+
+  public synchronized void setFlushLevel(Level level) {


renaming helper -> service

jabubake · 2017-04-05T19:29:32Z

@michaelbausor @garrettjonesgoogle ran the Google java formatter.
I renamed the class to LoggingService : its a wrapper class, if you have a preferred name suggestion, let me know.

pongad · 2017-04-06T06:59:48Z

Codacy seems to be really mad at you, but the flushing code looks fine (at least not any worse than it was before 😆 ). Lots of unit test failures though, are you setting default properties correctly?

meltsufin · 2017-04-06T14:33:13Z

@gregw PTAL

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

+      case "instance_id":
+        value = MetadataConfig.getInstanceId();
+        break;
+      case "cluster_name":


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

+        break;
+      case "gce_instance":
+        labels = new String[] {"instance_id", "zone"};
+        break;


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

+ */
+public class TraceEnhancer implements Enhancer {
+
+  private static final ThreadLocal<String> traceId = new ThreadLocal<>();


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

+package com.google.cloud.logging;
+
+/** A Log Enhancer. Used to enhance the {@link LogEntry} */
+public interface Enhancer {


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

+import com.google.cloud.MonitoredResource;
+import com.google.cloud.logging.Logging.WriteOption;
+import com.google.common.util.concurrent.Uninterruptibles;
+import java.util.*;


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

+      return "gae_app_standard";
+    }
+    if (MetadataConfig.getInstanceId() != null) {
+      return "gce_instance";


garrettjonesgoogle

I finally got a chance to complete a deep review.

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

 import com.google.common.base.Strings;
-import java.util.List;
+
+import java.util.*;


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

+      } catch (Exception e) {
+        // Ignore exceptions, they are propagated to the error manager.
+      }
+    }


google-cloud-core/src/main/java/com/google/cloud/MetadataConfig.java

+
+public class MetadataConfig {
+
+  private static final String metadataUrl = "http://metadata/computeMetadata/v1/";


google-cloud-core/src/main/java/com/google/cloud/MetadataConfig.java

+    return getAttribute("instance/", attributeName);
+  }
+
+  private static String getAttribute(String prefix, String attributeName) {


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

+  private static final String formatterTag = "formatter";
+  private static final String synchronicityTag = "synchronicity";
+  private static final String resourceTypeTag = "resourceType";
+  private static final String enchancersTag = "enhancers";


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

+import com.google.cloud.ServiceOptions;
+import com.google.common.base.Strings;
+
+import java.util.*;


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

+
+    addToMap(map, Resource.gce_instance, Label.instance_id, Label.zone);
+
+    resourceTypeWithLabels = Collections.unmodifiableMap(map);


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

-            });
-        break;
+    for (LoggingEnhancer enhancer : enhancers) {
+      enhancer.enhanceLogEntry(builder);


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

+import java.util.List;
+import java.util.Set;
+
+public class LoggingService {


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

+
+  public synchronized void setFlushSeverity(Severity severity) {
+    flushSeverity = severity;
+  }


garrettjonesgoogle

Some last cleanup items.

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

-    version_id,
-    zone
-  }
+    appId("app_id"),


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

+/**
+ * Monitored resource construction utilities to detect resource type and add labels.
+ */
+abstract class MonitoredResourceUtil {


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

+    return Resource.global;
+  }
+
+  protected static List<LoggingEnhancer> getResourceEnhancers() {


google-cloud-core/src/main/java/com/google/cloud/MetadataConfig.java

+import java.net.HttpURLConnection;
+import java.net.URL;
+
+public class MetadataConfig {


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

+
+package com.google.cloud.logging;
+
+public interface LoggingErrorHandler {


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

+import java.util.Set;
+
+/** Configurable logging service with level-based flush and error handling */
+public class LoggingService {


adding javadoc visibilty update on MonitoredResourceUtil

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

+    switch (resourceType) {
+      case GaeAppFlex:
+        enhancers = new ArrayList<>();
+        enhancers.add(new TraceLoggingEnhancer());


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

+
+      List<LoggingEnhancer> loggingEnhancers = MonitoredResourceUtil.getResourceEnhancers();
+      if (loggingEnhancers != null) {
+        this.enhancers.addAll(loggingEnhancers);


jabubake · 2017-04-17T17:14:25Z

@garrettjonesgoogle , @pongad : I've moved the flush functionality to LoggingImpl. PTAL.

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

+      writeLogEntries(logEntries, options);
+      for (LogEntry logEntry : logEntries) {
+        // flush pending writes if log severity at or above flush severity
+        if (logEntry.getSeverity().compareTo(flushSeverity) >= 0) {


garrettjonesgoogle

LGTM after my open question to @pongad is answered, and other people's feedback is addressed

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

+package com.google.cloud.logging;
+
+/*
+ */


pongad

The volatile makes sense to me. LGTM.

jabubake · 2017-04-18T02:47:44Z

@gregw, I've addressed your comments, PTAL.

meltsufin · 2017-04-18T20:40:05Z

@jabubake You might want to take a look at grpc-context as an alternative thread local to carry the trace id.

jabubake · 2017-04-18T22:29:57Z

@meltsufin : will look into it. @garrettjonesgoogle recommends we do that it in a separate PR.

coveralls · 2017-04-18T23:59:38Z

Coverage decreased (-0.08%) to 80.858% when pulling cca8fc4 on jabubake:ja_logging_refactor into 455f739 on GoogleCloudPlatform:master.

garrettjonesgoogle · 2017-04-19T00:00:47Z

LGTM

meltsufin · 2017-04-19T13:42:43Z

@jabubake and @garrettjonesgoogle thanks pulling this together! When can we expect a push to Maven?

garrettjonesgoogle · 2017-04-19T13:57:08Z

We should have a release by the end of the week.

jabubake added 10 commits February 23, 2017 14:03

adding slf4j logging adapter

b4138ca

TODO : tests

fixes, cleanup

2111278

logging refactor wip

bf9ee31

logging refactoring

46cde33

Merge remote-tracking branch 'upstream/master' into logging_refactor

25cef54

sync fork

8ec9a3f

fork sync

9d79afd

handling null options

b7da6f3

Merge remote-tracking branch 'upstream/master' into ja_logging_refactor

e3e41aa

TODO : tests

unit test fixes

dc2bc97

jabubake added the api: logging Issues related to the Cloud Logging API. label Apr 3, 2017

jabubake requested a review from pongad April 3, 2017 16:07

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 3, 2017

duchuyctlk reviewed Apr 3, 2017

View reviewed changes

jabubake added 2 commits April 3, 2017 10:21

adding missing break

db1d11a

removing in-memory buffering

efbb5c9

removing flush size config : needs to be better exposed in loggingoptions for control

duchuyctlk reviewed Apr 4, 2017

View reviewed changes

garrettjonesgoogle reviewed Apr 4, 2017

View reviewed changes

michaelbausor reviewed Apr 4, 2017

View reviewed changes

jabubake added 2 commits April 5, 2017 12:24

formatting updates

ba5efba

renaming helper -> service

formatting fixes

9a239cf

reverting formatting on files not related to update

d62d94e

garrettjonesgoogle reviewed Apr 6, 2017

View reviewed changes

jabubake added 3 commits April 6, 2017 17:22

code review fixes

7647a11

format update

3d5b71d

code cleanup MonitoredResourceUtil

573395e

garrettjonesgoogle reviewed Apr 11, 2017

View reviewed changes

jabubake added 3 commits April 12, 2017 18:51

Merge remote-tracking branch 'upstream/master' into ja_logging_refactor

cf183c5

refactoring code to google-cloud-java guidelines

4b8cf8f

marking fields only set in constructor to final

3627ce4

garrettjonesgoogle reviewed Apr 13, 2017

View reviewed changes

jabubake added 2 commits April 13, 2017 17:04

removing unused code

073735c

adding javadoc visibilty update on MonitoredResourceUtil

formatting update

f3c1aa0

gregw reviewed Apr 14, 2017

View reviewed changes

moving flush to Logging

82fac2b

removing unused var

9cd77f0

garrettjonesgoogle reviewed Apr 17, 2017

View reviewed changes

moving flush severity, sychnronicity to volatile

f2d15ba

garrettjonesgoogle reviewed Apr 17, 2017

View reviewed changes

google-cloud-logging/src/main/java/com/google/cloud/logging/TraceLoggingEnhancer.java Outdated

package com.google.cloud.logging;

/*

*/

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pongad reviewed Apr 18, 2017

View reviewed changes

adding javadoc

ae68387

jabubake added 3 commits April 18, 2017 09:30

updating logging snippets, writeAsync => write

d0d11f0

Merge remote-tracking branch 'upstream/master' into ja_logging_refactor

b029633

fixing unit test

0b2e670

jabubake added 2 commits April 18, 2017 16:03

adding defaultSeverity argument

ab0e527

removing flaky unit test in reading properties file

cca8fc4

jabubake merged commit 101e56e into googleapis:master Apr 19, 2017

jabubake deleted the ja_logging_refactor branch April 19, 2017 00:02


		public class MetadataConfig {

		private static final String metadataUrl = "http://metadata/computeMetadata/v1/";


		addToMap(map, Resource.gce_instance, Label.instance_id, Label.zone);

		resourceTypeWithLabels = Collections.unmodifiableMap(map);


		package com.google.cloud.logging;

		public interface LoggingErrorHandler {

Logging refactor of JUL + monitored resource construction #1847

Logging refactor of JUL + monitored resource construction #1847

Conversation

jabubake commented Apr 3, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

michaelbausor left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jabubake commented Apr 5, 2017

pongad commented Apr 6, 2017

meltsufin commented Apr 6, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jabubake commented Apr 17, 2017

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

pongad left a comment

Choose a reason for hiding this comment