Add safe-logging SafeArg to relevant logging statements

tritium-brave/build.gradle

@@ -3,6 +3,7 @@ apply from: "${rootDir}/gradle/publish-jars.gradle"
 dependencies {
     compile project(':tritium-api')
     compile project(':tritium-core')
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'io.zipkin.brave:brave-core'
     compile 'org.slf4j:slf4j-api'
 

tritium-brave/src/main/java/com/palantir/tritium/brave/BraveLocalTracingInvocationEventHandler.java

@@ -32,7 +32,7 @@
 
 public final class BraveLocalTracingInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(BraveLocalTracingInvocationEventHandler.class);
+    private static final Logger logger = LoggerFactory.getLogger(BraveLocalTracingInvocationEventHandler.class);
 
     private final Brave brave;
     private final String component;
@@ -71,7 +71,7 @@ public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable ca
 
     private void debugIfNullContext(@Nullable InvocationContext context) {
         if (context == null) {
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
+            logger.debug("Encountered null metric context likely due to exception in preInvocation");
         }
     }
 

tritium-caffeine/build.gradle

@@ -10,6 +10,7 @@ dependencies {
     compile 'com.github.ben-manes.caffeine:caffeine'
     compile 'com.google.code.findbugs:annotations'
     compile 'com.google.guava:guava'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'io.dropwizard.metrics:metrics-core'
     compile 'org.slf4j:slf4j-api'
 

tritium-core/build.gradle

@@ -4,6 +4,7 @@ dependencies {
     compile project(':tritium-api')
     compile 'com.google.guava:guava'
     compile 'com.google.code.findbugs:annotations'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'org.slf4j:slf4j-api'
 
     testCompile 'com.google.guava:guava-testlib'

tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java

@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.collect.ImmutableList;
+import com.palantir.logsafe.SafeArg;
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.List;
@@ -29,7 +30,7 @@
 
 public final class CompositeInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(CompositeInvocationEventHandler.class);
+    private static final Logger logger = LoggerFactory.getLogger(CompositeInvocationEventHandler.class);
 
     private final List<InvocationEventHandler<InvocationContext>> handlers;
 
@@ -76,7 +77,7 @@ public void onSuccess(@Nullable InvocationContext invocationContext, @Nullable O
                 }
             }
         } else {
-            LOGGER.debug("onSuccess InvocationContext was not a CompositeInvocationContext: {}", invocationContext);
+            logger.debug("onSuccess InvocationContext was not a CompositeInvocationContext: {}", invocationContext);
         }
     }
 
@@ -93,7 +94,7 @@ public void onFailure(@Nullable InvocationContext invocationContext, @Nonnull Th
                 }
             }
         } else {
-            LOGGER.debug("onFailure InvocationContext was not a CompositeInvocationContext: {}", invocationContext);
+            logger.debug("onFailure InvocationContext was not a CompositeInvocationContext: {}", invocationContext);
         }
     }
 
@@ -118,11 +119,15 @@ public String toString() {
 
     private static void preInvocationFailed(InvocationEventHandler<? extends InvocationContext> handler,
             Object instance, Method method, Object[] args, Exception exception) {
-        LOGGER.warn("Exception handling preInvocation({}): "
+        logger.warn("Exception handling preInvocation({}): "
                         + "invocation of {}.{} with arguments {} on {} threw: {}",
                 handler,
-                method.getDeclaringClass().getCanonicalName(), method.getName(), Arrays.toString(args), instance,
-                exception, exception);
+                SafeArg.of("class", method.getDeclaringClass().getCanonicalName()),
+                SafeArg.of("method", method.getName()),
+                Arrays.toString(args),
+                instance,
+                exception,
+                exception);
     }
 
     private void handleSuccess(InvocationEventHandler<?> handler,
@@ -132,7 +137,7 @@ private void handleSuccess(InvocationEventHandler<?> handler,
         try {
             handler.onSuccess(context, result);
         } catch (RuntimeException e) {
-            LOGGER.warn("Exception handling onSuccess({}, {}): {}",
+            logger.warn("Exception handling onSuccess({}, {}): {}",
                     context, result, e, e);
         }
     }
@@ -144,7 +149,7 @@ private void handleFailure(InvocationEventHandler<?> handler,
         try {
             handler.onFailure(context, cause);
         } catch (RuntimeException e) {
-            LOGGER.warn("Exception handling onFailure({}, {}): {}",
+            logger.warn("Exception handling onFailure({}, {}): {}",
                     context, cause, e, e);
         }
     }

tritium-lib/build.gradle

@@ -7,6 +7,7 @@ dependencies {
     compile project(':tritium-slf4j')
     compile 'com.google.code.findbugs:annotations'
     compile 'com.google.guava:guava'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'org.slf4j:slf4j-api'
 
     testCompile project(path: ':tritium-core', configuration: 'testArtifacts')

tritium-lib/src/main/java/com/palantir/tritium/proxy/InvocationEventProxy.java

@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.reflect.AbstractInvocationHandler;
+import com.palantir.logsafe.SafeArg;
 import com.palantir.tritium.api.functions.BooleanSupplier;
 import com.palantir.tritium.event.CompositeInvocationEventHandler;
 import com.palantir.tritium.event.InstrumentationFilter;
@@ -37,14 +38,15 @@
 abstract class InvocationEventProxy<C extends InvocationContext>
         extends AbstractInvocationHandler implements InvocationHandler {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(InvocationEventProxy.class);
+    private static final Logger logger = LoggerFactory.getLogger(InvocationEventProxy.class);
     private static final Object[] NO_ARGS = {};
 
     private final InstrumentationFilter filter;
     private final InvocationEventHandler<?> eventHandler;
 
     /**
      * Always enabled instrumentation handler.
+     *
      * @param handlers event handlers
      */
     protected InvocationEventProxy(List<InvocationEventHandler<InvocationContext>> handlers) {
@@ -81,12 +83,12 @@ private boolean isEnabled(Object instance, Method method, Object[] args) {
             return eventHandler.isEnabled()
                     && filter.shouldInstrument(instance, method, args);
         } catch (Throwable t) {
-            LOGGER.warn("Exception handling isEnabled({}): {}",
-                    toInvocationDebugString(instance, method, args), t, t);
+            logInvocationWarning("isEnabled", instance, method, args, t);
             return false;
         }
     }
 
+
     @Override
     @Nullable
     @SuppressWarnings("checkstyle:illegalthrows")
@@ -102,7 +104,7 @@ protected final Object handleInvocation(Object proxy, Method method, Object[] ar
      * {@link #invoke} delegates to this method upon any method invocation on the instance, except
      * {@link Object#equals}, {@link Object#hashCode} and {@link Object#toString}. The result will be returned as the
      * proxied method's return value.
-     *
+     * <p>
      * <p>
      * Unlike {@link #invoke}, {@code args} will never be null. When the method has no parameter, an empty array is
      * passed in.
@@ -126,8 +128,7 @@ final InvocationContext handlePreInvocation(Object instance, Method method, Obje
         try {
             return eventHandler.preInvocation(instance, method, args);
         } catch (RuntimeException e) {
-            LOGGER.warn("Exception handling preInvocation({}): {}",
-                    toInvocationDebugString(instance, method, args), e, e);
+            logInvocationWarning("preInvocation", instance, method, args, e);
         }
         return null;
     }
@@ -147,30 +148,48 @@ final Object handleOnSuccess(@Nullable InvocationContext context, @Nullable Obje
         try {
             eventHandler.onSuccess(context, result);
         } catch (RuntimeException e) {
-            LOGGER.warn("Exception handling onSuccess({}, {}): {}",
-                    context, result, e, e);
+            logInvocationWarningOnSuccess(context, result, e);
         }
         return result;
     }
 
+    private static void logInvocationWarningOnSuccess(InvocationContext context, Object result, Exception cause) {
+        logger.warn("{} exception handling onSuccess({}, {}): {}",
+                safeSimpleClassName("cause", cause), context, safeSimpleClassName("result", result), cause);
+    }
+
     final Throwable handleOnFailure(@Nullable InvocationContext context, Throwable cause) {
         try {
             eventHandler.onFailure(context, cause);
         } catch (RuntimeException e) {
-            LOGGER.warn("Exception handling onFailure({}, {}): {}",
-                    context, cause, e, e);
+            logInvocationWarningOnFailure(context, cause, e);
         }
         return cause;
     }
 
-    protected final String toInvocationDebugString(Object instance, Method method, @Nullable Object[] args) {
-        return "invocation of "
-                + method.getDeclaringClass() + '.' + method.getName()
-                + " with arguments " + Arrays.toString(nullToEmpty(args))
-                + " on " + instance + " via " + this + " : ";
+    private static void logInvocationWarningOnFailure(InvocationContext context, @Nullable Throwable result,
+            RuntimeException cause) {
+        logger.warn("{} exception handling onFailure({}, {}): {}",
+                safeSimpleClassName("cause", cause), context, safeSimpleClassName("result", result), cause);
+    }
+
+    private static SafeArg<String> safeSimpleClassName(String name, @Nullable Object object) {
+        return SafeArg.of(name, (object == null) ? "null" : object.getClass().getSimpleName());
+    }
+
+    static void logInvocationWarning(String event, Object instance, Method method, Object[] args,
+            Throwable cause) {
+        logger.warn("{} exception handling {} invocation of {}.{} with arguments {} on {}",
+                safeSimpleClassName("throwable", cause),
+                event,
+                SafeArg.of("class", method.getDeclaringClass()),
+                SafeArg.of("method", method.getName()),
+                Arrays.toString(nullToEmpty(args)),
+                instance,
+                cause);
     }
 
-    protected static Object[] nullToEmpty(@Nullable Object[] args) {
+    private static Object[] nullToEmpty(@Nullable Object[] args) {
         return (args == null) ? NO_ARGS : args;
     }
 

tritium-lib/src/test/java/com/palantir/tritium/proxy/InvocationEventProxyTest.java

@@ -142,8 +142,8 @@ public void testToInvocationDebugString() throws Exception {
         InvocationEventProxy<InvocationContext> proxy = createBasicProxy(
                 Collections.<InvocationEventHandler<InvocationContext>>emptyList());
 
-        String debugString = proxy.toInvocationDebugString(this, getToStringMethod(), null);
-        assertThat(debugString, containsString("x"));
+        Throwable cause = new RuntimeException("cause");
+        proxy.logInvocationWarning("test", this, getToStringMethod(), null, cause);
     }
 
     private InvocationEventProxy<InvocationContext> createBasicProxy(

tritium-metrics/build.gradle

@@ -5,6 +5,7 @@ dependencies {
     compile project(':tritium-core')
     compile 'com.google.code.findbugs:annotations'
     compile 'com.google.guava:guava'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'io.dropwizard.metrics:metrics-core'
     compile 'org.mpierce.metrics.reservoir:hdrhistogram-metrics-reservoir'
     compile 'org.hdrhistogram:HdrHistogram'

tritium-metrics/src/main/java/com/palantir/tritium/event/metrics/MetricsInvocationEventHandler.java

@@ -36,7 +36,7 @@
  */
 public final class MetricsInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(MetricsInvocationEventHandler.class);
+    private static final Logger logger = LoggerFactory.getLogger(MetricsInvocationEventHandler.class);
 
     private final MetricRegistry metricRegistry;
     private final String serviceName;
@@ -63,7 +63,7 @@ public InvocationContext preInvocation(Object instance, Method method, Object[]
     @Override
     public void onSuccess(@Nullable InvocationContext context, @Nullable Object result) {
         if (context == null) {
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
+            logger.debug("Encountered null metric context likely due to exception in preInvocation");
             return;
         }
         metricRegistry.timer(getBaseMetricName(context))
@@ -74,7 +74,7 @@ public void onSuccess(@Nullable InvocationContext context, @Nullable Object resu
     public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause) {
         if (context == null) {
             markGlobalFailure();
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation: {}", cause, cause);
+            logger.debug("Encountered null metric context likely due to exception in preInvocation: {}", cause, cause);
             return;
         }
 

tritium-metrics/src/main/java/com/palantir/tritium/metrics/MetricRegistries.java

@@ -40,7 +40,7 @@
  */
 public final class MetricRegistries {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(MetricRegistries.class);
+    private static final Logger logger = LoggerFactory.getLogger(MetricRegistries.class);
 
     /**
      * An ISO 8601 date format for pre-Java 8 compatibility without Joda dependency.
@@ -160,7 +160,7 @@ public static <T extends Metric> T registerSafe(MetricRegistry registry, String
         if (existingMetric == null) {
             return registry.register(name, metric);
         } else {
-            LOGGER.warn("Metric already registered at this name."
+            logger.warn("Metric already registered at this name."
                     + " Name: {}, existing metric: {}", name, existingMetric);
 
             Set<Class<?>> existingMetricInterfaces = ImmutableSet.copyOf(existingMetric.getClass().getInterfaces());

tritium-slf4j/build.gradle

@@ -4,6 +4,7 @@ dependencies {
     compile project(':tritium-api')
     compile project(':tritium-core')
     compile 'com.google.code.findbugs:annotations'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'org.slf4j:slf4j-api'
 
     testCompile project(path: ':tritium-core', configuration: 'testArtifacts')

tritium-slf4j/src/main/java/com/palantir/tritium/event/log/LoggingInvocationEventHandler.java

@@ -20,6 +20,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.palantir.logsafe.SafeArg;
 import com.palantir.tritium.api.functions.BooleanSupplier;
 import com.palantir.tritium.api.functions.LongPredicate;
 import com.palantir.tritium.event.AbstractInvocationEventHandler;
@@ -40,7 +41,7 @@
  */
 public class LoggingInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(LoggingInvocationEventHandler.class);
+    private static final Logger CLASS_LOGGER = LoggerFactory.getLogger(LoggingInvocationEventHandler.class);
     private static final List<String> MESSAGE_PATTERNS = generateMessagePatterns(10);
 
     public static final LongPredicate LOG_ALL_DURATIONS = LongPredicate.TRUE;
@@ -76,7 +77,7 @@ public final InvocationContext preInvocation(Object instance, Method method, Obj
     @Override
     public final void onSuccess(@Nullable InvocationContext context, @Nullable Object result) {
         if (context == null) {
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
+            CLASS_LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
             return;
         }
 
@@ -87,7 +88,7 @@ public final void onSuccess(@Nullable InvocationContext context, @Nullable Objec
     @Override
     public final void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause) {
         if (context == null) {
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
+            CLASS_LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
             return;
         }
 
@@ -191,9 +192,9 @@ static String getMessagePattern(Object[] args) {
 
     static Object[] getLogParams(Method method, Object[] args, long durationNanos, LoggingLevel level) {
         Object[] logParams = new Object[3 + args.length];
-        logParams[0] = method.getDeclaringClass().getSimpleName();
-        logParams[1] = method.getName();
-        logParams[logParams.length - 1] = String.format("%.3f", durationNanos / 1000000.0d);
+        logParams[0] = SafeArg.of("class", method.getDeclaringClass().getSimpleName());
+        logParams[1] = SafeArg.of("method", method.getName());
+        logParams[logParams.length - 1] = SafeArg.of("milliseconds", String.format("%.3f", durationNanos / 1000000.0d));
 
         Class<?>[] argTypes = method.getParameterTypes();
         if (argTypes.length == 0) {
@@ -209,7 +210,7 @@ static Object[] getLogParams(Method method, Object[] args, long durationNanos, L
                 }
             }
 
-            logParams[2 + i] = argMessage;
+            logParams[2 + i] = SafeArg.of("type" + i, argMessage);
         }
 
         return logParams;

tritium-tracing/build.gradle

@@ -6,6 +6,7 @@ dependencies {
 
     compile 'com.google.code.findbugs:annotations'
     compile 'com.palantir.remoting1:tracing'
+    compile 'com.palantir.safe-logging:safe-logging'
     compile 'org.slf4j:slf4j-api'
 
     testCompile project(path: ':tritium-core', configuration: 'testArtifacts')

tritium-tracing/src/main/java/com/palantir/tritium/tracing/TracingInvocationEventHandler.java

@@ -30,7 +30,7 @@
 
 public final class TracingInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(TracingInvocationEventHandler.class);
+    private static final Logger logger = LoggerFactory.getLogger(TracingInvocationEventHandler.class);
 
     private final String component;
 
@@ -66,7 +66,7 @@ public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable ca
 
     private void debugIfNullContext(@Nullable InvocationContext context) {
         if (context == null) {
-            LOGGER.debug("Encountered null metric context likely due to exception in preInvocation");
+            logger.debug("Encountered null metric context likely due to exception in preInvocation");
         }
     }