palantir/safe-logging

Interfaces and utilities for safe log messages

License

This repository is made available under the Apache 2.0 License.

Safe-Logging

Interfaces and utilities for safe log messages.

Usage

Add dependency to gradle:

dependencies {
    implementation 'com.palantir.safe-logging:logger'
}

Update logger instances to use SafeLogger and annotate log parameters with named SafeArg and UnsafeArg as appropriate. For example:

Previously

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

private static final Logger log = LoggerFactory.getLogger(MyClass.class);
...
log.info("Twisted the {} knob {} times", knobName, count);

Now

import com.palantir.logsafe.logger.SafeLogger;
import com.palantir.logsafe.logger.SafeLoggerFactory;

private static final SafeLogger log = SafeLoggerFactory.get(MyClass.class);
...
log.info("Twisted the {} knob {} times", UnsafeArg.of("knobName", knobName), SafeArg.of("count", count));
// Even cleaner without slf4j-style interpolation markers:
log.info("Twisted the knob", UnsafeArg.of("knob", knobName), SafeArg.of("twists", count));

Preconditions

Guava Preconditions equivalent which produces exceptions conforming to the SafeLoggable standard.

Usage

Add dependency to gradle:

dependencies {
    implementation 'com.palantir.safe-logging:preconditions'
    // optional test utilities
    testImplementation 'com.palantir.safe-logging:preconditions-assertj'
}

Annotate Preconditions error messages with named SafeArg and UnsafeArg as appropriate. For example:

// previously
import com.google.common.base.Preconditions;
...
Preconditions.checkArgument(uname.size() > MAX_LEN, "%s username longer than max %s", uname, MAX_LEN);

// now
import com.palantir.logsafe.Preconditions;
...
Preconditions.checkArgument(uname.size() > MAX_LEN, "username longer than max",
        UnsafeArg.of("uname", uname), SafeArg.of("max", MAX_LEN));

safe-logging's Issues

Release 1.0

This seems to have stabilized with few/no meaningful changes since summer. Given it's widely used, time to 1.0

Provide alternate UnsafeArg factory that takes Supplier<T>

We may want to log an object with a sensitive field that should not be logged even under unsafe parameters (e.g. user credentials).

log.info("message", UnsafeArg.of("name", unsafeValue))

Rather than relying on the toString() implementation to remove the offending value, users should instead be able to write:

log.info("message", UnsafeArg.of("name", () -> removeUserCredentials(unsafeValue))

This mostly applies to UnsafeArg, but maybe we could also add to SafeArg for consistency. The more probable scenario with SafeArg is a field that is too large/expensive to log.

Why does SafeLoggable not provide SafeArgs?

SafeLoggable returns Args, but should return SafeArgs?

https://github.com/palantir/safe-logging/blob/develop/safe-logging/src/main/java/com/palantir/logsafe/SafeLoggable.java#L28

Motivation/context in README

It would be very helpful to add a section on what it means for logging arguments to be safe or not, and what logging frameworks (and which?) then does with this information. This especially considering that checking for usage of this library is on by default in https://github.com/palantir/gradle-baseline

Usage examples

How should these annotations be used? Do they work only for JAX-RS endpoints, or are these also the annotations that should be placed for log4j calls?

Please provide usage examples in the README for how to use this library.

Arg is only Serializable if T is serializable

https://github.com/palantir/safe-logging/blob/develop/safe-logging/src/main/java/com/palantir/logsafe/Arg.java#L23

Safe Exception Implementations should provide arguments in message

ServiceException, for example, provides an unsafe exception message (getMessage()), and a safe log message (getLogMessage()). By providing both we can avoid losing data when libraries which depend on safe-logging are used in environments without sls-logging appenders.

Current message when the following is logged using non-sls mechanisms new SafeIllegalArgumentException("Bad Value", SafeArg.of("value", 3))

Bad Value

Expected:

Bad Value: {value=3}

Not all packages published to jcenter

Not all packages in safe-logging are published to jcenter. For example, for version 1.10.1, the safe-logging package is there [1], but preconditions isn't [2]. It is on palantir bintray [3], but that is not available by default in gradle. Could all libraries be published to jcenter?

[1] https://jcenter.bintray.com/com/palantir/safe-logging/safe-logging/1.11.0/
[2] https://jcenter.bintray.com/com/palantir/safe-logging/
[3] https://palantir.bintray.com/releases/com/palantir/safe-logging/

Implement AssertJ representation

I've submitted two PRs, not sure which approach is preferred. In both cases, an exception message for matching against the wrong type of Arg goes from:

java.lang.AssertionError: 
Expecting:
 <[test, version]>
to contain:
 <[test, version]>
but could not find:
 <[test]>

to

java.lang.AssertionError: 
Expecting:
 <[SafeArg[id=test], SafeArg[version=version]]>
to contain:
 <[UnsafeArg[id=test], SafeArg[version=version]]>
but could not find:
 <[UnsafeArg[id=test]]>

Happy to add tests to whichever approach is considered better.

Logging of multiple parameters safely

What happened?

I have a 3-parameter tuple that is used as a composite data store key. I end up writing code that looks like this:

log.error("bad thing happened on {} {} {}",
UnsafeArg.of("key", primaryKey),
UnsafeArg.of("branch", branch),
SafeArg.of("semanticVersion", SafeLog.of(semanticVersion)));

This 3-parameter tuple of [key, branch, semanticVersion] is common across logging statements and safe exceptions because my application needs all 3 parameters to make sense of what is going on.

The construction of logging and exceptions with these three parameters over and over again is tedious and error prone.

I attempted to wrap this logic with something like this:

@SuppressWarnings("rawtypes")
    public static Arg[] argsFromPersistedObjectKeyArgs(PersistedObjectKey key) {
        Arg[] args = new Arg[3];
        args[0] = UnsafeArg.of(KEY_LOG_PARAM, key.key());
        args[1] = SafeArg.of(BRANCH_LOG_PARAM, key.branch());
        args[2] = SafeArg.of(SEMANTIC_VERSION_LOG_PARAM, key.semanticVersion());
        return args;
    }

and use it like so:

log.warn("table found no key for ({})", (Object[]) SafeLoggerUtils.argsFromPersistedObjectKeyArgs(key));

but:

1. I have to use an (Object[]) prefix to avoid ambiguous method calls
2. I end up throwing Baseline Error Prone Checks for the usage cast to (Object[])

warning: [Slf4jLogsafeArgs] slf4j log statement does not use logsafe parameters for arguments [1]
(see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)

To remove the error prone check I have to annotate my methods with Slf4jLogsafeArgs suppressed warnings.

What did you want to happen?

I would like the logging and exception methods to handle non-Args-only objects constructed with an interface that supports both safe and unsafe logging. This may already exist but no one has been able to point me to it so far.

Consider removing versions.props

This library shouldn't have dependencies, so could consider removing all the resolution logic from build.gradle

Ability to mark exception messages as safe

For debuggability I need the ability to mark exception messages as safe or unsafe (right now they're all unsafe by default) so they can be picked up and used by various tools.

As an example of where this would be useful, the contents of this message in http-remoting will always be safe (e.g. 500 Internal Server Error), so I'd like to mark it as safe in some way so it can be logged appropriately.

One possible implementation is SafeRuntimeException, SafeIllegalArgumentException, etc classes

Why is @Safe Jersey-specific?

Javadoc docs about @Safe being jersey-specific. But it could theoretically be used in arbitrary methods, right?

https://github.com/palantir/safe-logging/blob/develop/safe-logging/src/main/java/com/palantir/logsafe/Safe.java#L30

New SafeLogger.forEnclosingClass()

What happened?

Currently, classes access a SafeLogger like this:

public final class MyClass {
    private static final SafeLogger log = SafeLoggerFactory.get(MyClass.class);
   ...
}

This makes it possible to pass a different parameter to the factory's get method than the class it's in, like this antipattern:

public final class MyClass {
    private static final SafeLogger log = SafeLoggerFactory.get(OtherClass.class);  // <-- OtherClass ???
   ...
}

What did you want to happen?

Instead, we could use a pattern that removes the need for this parameter entirely:

public final class MyClass {
    private static final SafeLogger log = SafeLogger.forEnclosingClass();
   ...
}

This is inspired by GoogleLogger.forEnclosingClass() here, which is widely used by Google.

Using the forEnclosingClass() pattern instead of taking the parameter will make the SafeLogger API less error-prone and harder to use incorrectly.