Adding "callerOf(Class<?>)" to LogSites to allow simpler APIs for log…

…ging utilities.

This avoids people needing to expose "LogSite" as a parameter in logging helper classes and should allow most of the current ~350 call sites using it to be simplified.

This is a retry of the previous change which accidentally broke anyone who was relying on being able to set an INVALID log site to suppress log site determination (that's apparently a thing in Android).

RELNOTES=Adding "callerOf(Class<?>)" to LogSites.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=289240473

api/src/main/java/com/google/common/flogger/LogContext.java

@@ -517,7 +517,7 @@ protected boolean postProcess(@NullableDecl LogSiteKey logSiteKey) {
    * of arguments.
    */
   private boolean shouldLog() {
-    // The log site may have already been injected via "withInjectedLogSite()".
+    // The log site may have already been injected via "withInjectedLogSite()" or similar.
     if (logSite == null) {
       // From the point at which we call inferLogSite() we can skip 1 additional method (the
       // shouldLog() method itself) when looking up the stack to find the log() method.
@@ -571,9 +571,11 @@ private void logImpl(String message, Object... args) {
   @Override
   public final API withInjectedLogSite(LogSite logSite) {
     // First call wins (since auto-injection will typically target the log() method at the end of
-    // the chain and might not check for previous explicit injection).
-    if (this.logSite == null) {
-      this.logSite = checkNotNull(logSite, "log site");
+    // the chain and might not check for previous explicit injection). In particular it MUST be
+    // allowed for a caller to specify the "INVALID" log site, and have that set the field here to
+    // disable log site lookup at this log statement (though passing "null" is a no-op).
+    if (this.logSite == null && logSite != null) {
+      this.logSite = logSite;
     }
     return api();
   }

api/src/main/java/com/google/common/flogger/LogSites.java

@@ -26,16 +26,83 @@
  *
  */
 public final class LogSites {
+  /**
+   * Returns a {@code LogSite} for the caller of the specified class. This can be used in
+   * conjunction with the {@link LoggingApi#withInjectedLogSite(LogSite)} method to implement
+   * logging helper methods. In some platforms, log site determination may be unsupported, and in
+   * those cases this method will always return the {@link LogSite#INVALID} instance.
+   * <p>
+   * For example (in {@code MyLoggingHelper}):
+   * <pre>{@code
+   * public static void logAndSomethingElse(String message, Object... args) {
+   *   logger.atInfo()
+   *       .withInjectedLogSite(callerOf(MyLoggingHelper.class))
+   *       .logVarargs(message, args);
+   * }
+   * }</pre>
+   * <p>
+   * This method should be used for the simple cases where the class in which the logging occurs is
+   * a public logging API. If the log statement is in a different class (not the public logging API)
+   * and the {@code LogSite} instance needs to be passed through several layers, consider using
+   * {@link #logSite()} instead to avoid too much "magic" in your code.
+   * <p>
+   * You should also seek to ensure that any API used with this method "looks like a logging API".
+   * It's no good if a log entry contains a class and method name which doesn't correspond to
+   * anything the user can relate to. In particular, the API should probably always accept the log
+   * message or at least some of its parameters, and should always have methods with "log" in their
+   * names to make the connection clear.
+   * <p>
+   * It is very important to note that this method can be very slow, since determining the log site
+   * can involve stack trace analysis. It is only recommended that it is used for cases where
+   * logging is expected to occur (e.g. {@code INFO} level or above). Implementing a helper method
+   * for {@code FINE} logging is usually unnecessary (it doesn't normally need to follow any
+   * specific "best practice" behavior).
+   * <p>
+   * Note that even when log site determination is supported, it is not defined as to whether two
+   * invocations of this method on the same line of code will produce the same instance, equivalent
+   * instances or distinct instance. Thus you should never invoke this method twice in a single
+   * statement (and you should never need to).
+   * <p>
+   * Note that this method call may be replaced in compiled applications via bytecode manipulation
+   * or other mechanisms to improve performance.
+   *
+   * @param loggingApi the logging API to be identified as the source of log statements (this must
+   *        appear somewhere on the stack above the point at which this method is called).
+   * @return the log site of the caller of the specified logging API, or {@link LogSite#INVALID} if
+   *         the logging API was not found.
+   */
+  public static LogSite callerOf(Class<?> loggingApi) {
+    // Can't skip anything here since someone could pass in LogSite.class.
+    return Platform.getCallerFinder().findLogSite(loggingApi, 0);
+  }
+
   /**
    * Returns a {@code LogSite} for the current line of code. This can be used in conjunction with
    * the {@link LoggingApi#withInjectedLogSite(LogSite)} method to implement logging helper
    * methods. In some platforms, log site determination may be unsupported, and in those cases this
    * method will always return the {@link LogSite#INVALID} instance.
    * <p>
+   * For example (in {@code MyLoggingHelper}):
+   * <pre>{@code
+   * public static void logAndSomethingElse(LogSite logSite, String message, Object... args) {
+   *   logger.atInfo()
+   *       .withInjectedLogSite(logSite)
+   *       .logVarargs(message, args);
+   * }
+   * }</pre>
+   * where callers would do:
+   * <pre>{@code
+   * MyLoggingHelper.logAndSomethingElse(logSite(), "message...");
+   * }</pre>
+   * <p>
+   * Because this method adds an additional parameter and exposes a Flogger specific type to the
+   * calling code, you should consider using {@link #callerOf(Class<?>)} for simple logging
+   * utilities.
+   * <p>
    * It is very important to note that this method can be very slow, since determining the log site
    * can involve stack trace analysis. It is only recommended that it is used for cases where
-   * logging is expected to occur (e.g. {@code WARNING} level or above). Implementing a helper
-   * method for {@code FINE} logging is usually unnecessary (it doesn't normally need to follow any
+   * logging is expected to occur (e.g. {@code INFO} level or above). Implementing a helper method
+   * for {@code FINE} logging is usually unnecessary (it doesn't normally need to follow any
    * specific "best practice" behavior).
    * <p>
    * Note that even when log site determination is supported, it is not defined as to whether two
@@ -45,8 +112,11 @@ public final class LogSites {
    * <p>
    * Note that this method call may be replaced in compiled applications via bytecode manipulation
    * or other mechanisms to improve performance.
+   *
+   * @return the log site of the caller of this method.
    */
   public static LogSite logSite() {
+    // Don't call "callerOf()" to avoid making another stack entry.
     return Platform.getCallerFinder().findLogSite(LogSites.class, 0);
   }
 

api/src/main/java/com/google/common/flogger/LoggingApi.java

@@ -627,7 +627,6 @@ protected final API noOp() {
 
     @Override
     public API withInjectedLogSite(LogSite logSite) {
-      checkNotNull(logSite, "log site");
       return noOp();
     }
 

api/src/test/java/com/google/common/flogger/LogContextTest.java

@@ -544,7 +544,6 @@ public void testStackTraceFormatting() {
     // Print the stack trace via the expected method (ie, printStackTrace()).
     Throwable cause = backend.getLogged(0).getMetadata().findValue(Key.LOG_CAUSE);
     assertThat(cause).hasMessageThat().isEqualTo("SMALL");
-    ;
     StringWriter out = new StringWriter();
     cause.printStackTrace(new PrintWriter(out));
     Iterable<String> actualStackLines = Splitter.on('\n').trimResults().split(out.toString());
@@ -598,9 +597,11 @@ private static StackTraceElement getCallerInfoFollowingLine() {
   public void testExplicitLogSiteInjection() {
     FakeLoggerBackend backend = new FakeLoggerBackend();
     FluentLogger logger = new FluentLogger(backend);
+    // Tests it's the log site instance that controls rate limiting, even over different calls.
+    // We don't expect this to ever happen in real code though.
     for (int i = 0; i <= 6; i++) {
-      logHelper(logger, logSite(), 2, "Foo: " + i);
-      logHelper(logger, logSite(), 3, "Bar: " + i);
+      logHelper(logger, logSite(), 2, "Foo: " + i);  // Log every 2nd (0, 2, 4, 6)
+      logHelper(logger, logSite(), 3, "Bar: " + i);  // Log every 3rd (0, 3, 6)
     }
     // Expect: Foo -> 0, 2, 4, 6 and Bar -> 0, 3, 6 (but not in that order)
     assertThat(backend.getLoggedCount()).isEqualTo(7);
@@ -617,4 +618,21 @@ public void testExplicitLogSiteInjection() {
   private static void logHelper(FluentLogger logger, LogSite logSite, int n, String message) {
     logger.atInfo().withInjectedLogSite(logSite).every(n).log(message);
   }
+
+  // It's important that injecting an INVALID log site acts as a override to suppress log site
+  // calculation rather than being a no-op.
+  @Test
+  public void testExplicitLogSiteSuppression() {
+    FakeLoggerBackend backend = new FakeLoggerBackend();
+    FluentLogger logger = new FluentLogger(backend);
+
+    logger.atInfo().withInjectedLogSite(LogSite.INVALID).log("No log site here");
+    logger.atInfo().withInjectedLogSite(null).log("No-op injection");
+
+    assertThat(backend.getLoggedCount()).isEqualTo(2);
+    backend.assertLogged(0).logSite().isEqualTo(LogSite.INVALID);
+
+    backend.assertLogged(1).logSite().isNotNull();
+    backend.assertLogged(1).logSite().isNotEqualTo(LogSite.INVALID);
+  }
 }

api/src/test/java/com/google/common/flogger/LogSitesTest.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2020 The Flogger Authors.
+ *
+ * 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.common.flogger;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public final class LogSitesTest {
+  @Test
+  public void testLogSite() {
+    assertThat(LogSites.logSite().getMethodName()).isEqualTo("testLogSite");
+  }
+
+  @Test
+  public void testCallerOf() {
+    assertThat(MyLogUtil.getCallerLogSite().getMethodName()).isEqualTo("testCallerOf");
+    assertThat(MyLogUtil.getCallerLogSiteWrapped().getMethodName()).isEqualTo("testCallerOf");
+  }
+
+  @Test
+  public void testCallerOf_notFound() {
+    assertThat(LogSites.callerOf(String.class)).isEqualTo(LogSite.INVALID);
+  }
+
+  private static class MyLogUtil {
+    static LogSite getCallerLogSite() {
+      return LogSites.callerOf(MyLogUtil.class);
+    }
+
+    static LogSite getCallerLogSiteWrapped() {
+      return getCallerLogSite();
+    }
+  }
+}

api/src/test/java/com/google/common/flogger/testing/LogDataSubject.java

@@ -90,4 +90,11 @@ public void wasForced() {
       failWithActual(simpleFact("expected to be forced"));
     }
   }
+
+  /**
+   * Asserts about the log site of the log record.
+   */
+  public Subject logSite() {
+    return check("getLogSite()").that(actual.getLogSite());
+  }
 }