Add metric to count errors that occurred during plugin invocations

For plugins that are invoked through a plugin context errors may be
caught and logged. Add a metric that counts how often this happens so
that plugins that have issues can be easily detected.

Change-Id: I03c75305c0b33f1c974b789d644b5daed12bc8b4
Signed-off-by: Edwin Kempin <ekempin@google.com>

Documentation/metrics.txt

@@ -160,6 +160,10 @@ suggestion.
 
 * `sequence/next_id_latency`: Latency of requesting IDs from repo sequences.
 
+=== Plugin
+
+* `plugin/error_count`: Number of plugin errors.
+
 === Replication Plugin
 
 * `plugins/replication/replication_latency`: Time spent pushing to remote

java/com/google/gerrit/server/plugincontext/PluginContext.java

@@ -16,13 +16,21 @@ package com.google.gerrit.server.plugincontext;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.base.Strings;
 import com.google.common.base.Throwables;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.Extension;
+import com.google.gerrit.metrics.Counter3;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.DisabledMetricMaker;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.server.logging.TraceContext;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
 
 /**
  * Context for invoking plugin extensions.
@@ -98,6 +106,32 @@ public class PluginContext<T> {
     R call(T extension) throws X;
   }
 
+  @Singleton
+  public static class PluginMetrics {
+    public static final PluginMetrics DISABLED_INSTANCE =
+        new PluginMetrics(new DisabledMetricMaker());
+
+    final Counter3<String, String, String> errorCount;
+
+    @Inject
+    PluginMetrics(MetricMaker metricMaker) {
+      this.errorCount =
+          metricMaker.newCounter(
+              "plugin/error_count",
+              new Description("Number of plugin errors").setCumulative().setUnit("errors"),
+              Field.ofString("plugin_name"),
+              Field.ofString("class_name"),
+              Field.ofString("export_name"));
+    }
+
+    void incrementErrorCount(Extension<?> extension) {
+      errorCount.increment(
+          extension.getPluginName(),
+          extension.get().getClass().getName(),
+          Strings.nullToEmpty(extension.getExportName()));
+    }
+  }
+
   /**
    * Opens a new trace context for invoking a plugin extension.
    *
@@ -128,11 +162,14 @@ public class PluginContext<T> {
    *
    * <p>The consumer gets the extension implementation provided that should be invoked.
    *
+   * @param pluginMetrics the plugin metrics
    * @param extension extension that is being invoked
    * @param extensionImplConsumer the consumer that invokes the extension
    */
   static <T> void runLogExceptions(
-      Extension<T> extension, ExtensionImplConsumer<T> extensionImplConsumer) {
+      PluginMetrics pluginMetrics,
+      Extension<T> extension,
+      ExtensionImplConsumer<T> extensionImplConsumer) {
     T extensionImpl = extension.get();
     if (extensionImpl == null) {
       return;
@@ -141,6 +178,7 @@ public class PluginContext<T> {
     try (TraceContext traceContext = newTrace(extension)) {
       extensionImplConsumer.run(extensionImpl);
     } catch (Throwable e) {
+      pluginMetrics.incrementErrorCount(extension);
       logger.atWarning().withCause(e).log(
           "Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
     }
@@ -152,11 +190,14 @@ public class PluginContext<T> {
    * <p>The consumer get the {@link Extension} provided that should be invoked. The extension
    * provides access to the plugin name and the export name.
    *
+   * @param pluginMetrics the plugin metrics
    * @param extension extension that is being invoked
    * @param extensionConsumer the consumer that invokes the extension
    */
   static <T> void runLogExceptions(
-      Extension<T> extension, ExtensionConsumer<Extension<T>> extensionConsumer) {
+      PluginMetrics pluginMetrics,
+      Extension<T> extension,
+      ExtensionConsumer<Extension<T>> extensionConsumer) {
     T extensionImpl = extension.get();
     if (extensionImpl == null) {
       return;
@@ -165,6 +206,7 @@ public class PluginContext<T> {
     try (TraceContext traceContext = newTrace(extension)) {
       extensionConsumer.run(extension);
     } catch (Throwable e) {
+      pluginMetrics.incrementErrorCount(extension);
       logger.atWarning().withCause(e).log(
           "Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
     }
@@ -177,12 +219,14 @@ public class PluginContext<T> {
    *
    * <p>The consumer gets the extension implementation provided that should be invoked.
    *
+   * @param pluginMetrics the plugin metrics
    * @param extension extension that is being invoked
    * @param extensionImplConsumer the consumer that invokes the extension
    * @param exceptionClass type of the exceptions that should be thrown
    * @throws X expected exception from the plugin extension
    */
   static <T, X extends Exception> void runLogExceptions(
+      PluginMetrics pluginMetrics,
       Extension<T> extension,
       ExtensionImplConsumer<T> extensionImplConsumer,
       Class<X> exceptionClass)
@@ -197,6 +241,7 @@ public class PluginContext<T> {
     } catch (Throwable e) {
       Throwables.throwIfInstanceOf(e, exceptionClass);
       Throwables.throwIfUnchecked(e);
+      pluginMetrics.incrementErrorCount(extension);
       logger.atWarning().withCause(e).log(
           "Failure in %s of plugin invoke%s", extensionImpl.getClass(), extension.getPluginName());
     }
@@ -210,12 +255,14 @@ public class PluginContext<T> {
    * <p>The consumer get the {@link Extension} provided that should be invoked. The extension
    * provides access to the plugin name and the export name.
    *
+   * @param pluginMetrics the plugin metrics
    * @param extension extension that is being invoked
    * @param extensionConsumer the consumer that invokes the extension
    * @param exceptionClass type of the exceptions that should be thrown
    * @throws X expected exception from the plugin extension
    */
   static <T, X extends Exception> void runLogExceptions(
+      PluginMetrics pluginMetrics,
       Extension<T> extension,
       ExtensionConsumer<Extension<T>> extensionConsumer,
       Class<X> exceptionClass)
@@ -230,6 +277,7 @@ public class PluginContext<T> {
     } catch (Throwable e) {
       Throwables.throwIfInstanceOf(e, exceptionClass);
       Throwables.throwIfUnchecked(e);
+      pluginMetrics.incrementErrorCount(extension);
       logger.atWarning().withCause(e).log(
           "Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
     }

java/com/google/gerrit/server/plugincontext/PluginItemContext.java

@@ -23,6 +23,7 @@ import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.server.plugincontext.PluginContext.CheckedExtensionImplFunction;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplConsumer;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplFunction;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 import com.google.inject.Inject;
 
 /**
@@ -82,11 +83,13 @@ import com.google.inject.Inject;
  */
 public class PluginItemContext<T> {
   @Nullable private final DynamicItem<T> dynamicItem;
+  private final PluginMetrics pluginMetrics;
 
   @VisibleForTesting
   @Inject
-  public PluginItemContext(DynamicItem<T> dynamicItem) {
+  public PluginItemContext(DynamicItem<T> dynamicItem, PluginMetrics pluginMetrics) {
     this.dynamicItem = dynamicItem;
+    this.pluginMetrics = pluginMetrics;
   }
 
   /**
@@ -115,7 +118,7 @@ public class PluginItemContext<T> {
     if (extension == null) {
       return;
     }
-    PluginContext.runLogExceptions(extension, extensionImplConsumer);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionImplConsumer);
   }
 
   /**
@@ -136,7 +139,7 @@ public class PluginItemContext<T> {
     if (extension == null) {
       return;
     }
-    PluginContext.runLogExceptions(extension, extensionImplConsumer, exceptionClass);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionImplConsumer, exceptionClass);
   }
 
   /**

java/com/google/gerrit/server/plugincontext/PluginMapContext.java

@@ -19,6 +19,7 @@ import com.google.common.collect.Iterators;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionConsumer;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 import com.google.inject.Inject;
 import java.util.Iterator;
 import java.util.SortedSet;
@@ -92,11 +93,13 @@ import java.util.SortedSet;
  */
 public class PluginMapContext<T> implements Iterable<PluginMapEntryContext<T>> {
   private final DynamicMap<T> dynamicMap;
+  private final PluginMetrics pluginMetrics;
 
   @VisibleForTesting
   @Inject
-  public PluginMapContext(DynamicMap<T> dynamicMap) {
+  public PluginMapContext(DynamicMap<T> dynamicMap, PluginMetrics pluginMetrics) {
     this.dynamicMap = dynamicMap;
+    this.pluginMetrics = pluginMetrics;
   }
 
   /**
@@ -111,7 +114,8 @@ public class PluginMapContext<T> implements Iterable<PluginMapEntryContext<T>> {
    */
   @Override
   public Iterator<PluginMapEntryContext<T>> iterator() {
-    return Iterators.transform(dynamicMap.iterator(), PluginMapEntryContext<T>::new);
+    return Iterators.transform(
+        dynamicMap.iterator(), e -> new PluginMapEntryContext<>(e, pluginMetrics));
   }
 
   /**
@@ -147,7 +151,7 @@ public class PluginMapContext<T> implements Iterable<PluginMapEntryContext<T>> {
    * @param extensionConsumer consumer that invokes the extension
    */
   public void runEach(ExtensionConsumer<Extension<T>> extensionConsumer) {
-    dynamicMap.forEach(p -> PluginContext.runLogExceptions(p, extensionConsumer));
+    dynamicMap.forEach(p -> PluginContext.runLogExceptions(pluginMetrics, p, extensionConsumer));
   }
 
   /**
@@ -166,7 +170,7 @@ public class PluginMapContext<T> implements Iterable<PluginMapEntryContext<T>> {
   public <X extends Exception> void runEach(
       ExtensionConsumer<Extension<T>> extensionConsumer, Class<X> exceptionClass) throws X {
     for (Extension<T> extension : dynamicMap) {
-      PluginContext.runLogExceptions(extension, extensionConsumer, exceptionClass);
+      PluginContext.runLogExceptions(pluginMetrics, extension, extensionConsumer, exceptionClass);
     }
   }
 }

java/com/google/gerrit/server/plugincontext/PluginMapEntryContext.java

@@ -21,6 +21,7 @@ import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.server.plugincontext.PluginContext.CheckedExtensionFunction;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionConsumer;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionFunction;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 
 /**
  * Context to invoke an extension from {@link DynamicMap}.
@@ -84,11 +85,13 @@ import com.google.gerrit.server.plugincontext.PluginContext.ExtensionFunction;
  */
 public class PluginMapEntryContext<T> {
   private final Extension<T> extension;
+  private final PluginMetrics pluginMetrics;
 
-  PluginMapEntryContext(Extension<T> extension) {
+  PluginMapEntryContext(Extension<T> extension, PluginMetrics pluginMetrics) {
     checkNotNull(extension);
     checkNotNull(extension.getExportName(), "export name must be set for plugin map entries");
     this.extension = extension;
+    this.pluginMetrics = pluginMetrics;
   }
 
   /**
@@ -118,7 +121,7 @@ public class PluginMapEntryContext<T> {
    * @param extensionConsumer consumer that invokes the extension
    */
   public void run(ExtensionConsumer<Extension<T>> extensionConsumer) {
-    PluginContext.runLogExceptions(extension, extensionConsumer);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionConsumer);
   }
 
   /**
@@ -133,7 +136,7 @@ public class PluginMapEntryContext<T> {
    */
   public <X extends Exception> void run(
       ExtensionConsumer<Extension<T>> extensionConsumer, Class<X> exceptionClass) throws X {
-    PluginContext.runLogExceptions(extension, extensionConsumer, exceptionClass);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionConsumer, exceptionClass);
   }
 
   /**

java/com/google/gerrit/server/plugincontext/PluginSetContext.java

@@ -19,6 +19,7 @@ import com.google.common.collect.Iterators;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplConsumer;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 import com.google.inject.Inject;
 import java.util.Iterator;
 import java.util.SortedSet;
@@ -84,11 +85,13 @@ import java.util.SortedSet;
  */
 public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
   private final DynamicSet<T> dynamicSet;
+  private final PluginMetrics pluginMetrics;
 
   @VisibleForTesting
   @Inject
-  public PluginSetContext(DynamicSet<T> dynamicSet) {
+  public PluginSetContext(DynamicSet<T> dynamicSet, PluginMetrics pluginMetrics) {
     this.dynamicSet = dynamicSet;
+    this.pluginMetrics = pluginMetrics;
   }
 
   /**
@@ -103,7 +106,8 @@ public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
    */
   @Override
   public Iterator<PluginSetEntryContext<T>> iterator() {
-    return Iterators.transform(dynamicSet.entries().iterator(), PluginSetEntryContext<T>::new);
+    return Iterators.transform(
+        dynamicSet.entries().iterator(), e -> new PluginSetEntryContext<>(e, pluginMetrics));
   }
 
   /**
@@ -138,7 +142,9 @@ public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
    * @param extensionImplConsumer consumer that invokes the extension
    */
   public void runEach(ExtensionImplConsumer<T> extensionImplConsumer) {
-    dynamicSet.entries().forEach(p -> PluginContext.runLogExceptions(p, extensionImplConsumer));
+    dynamicSet
+        .entries()
+        .forEach(p -> PluginContext.runLogExceptions(pluginMetrics, p, extensionImplConsumer));
   }
 
   /**
@@ -156,7 +162,8 @@ public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
   public <X extends Exception> void runEach(
       ExtensionImplConsumer<T> extensionImplConsumer, Class<X> exceptionClass) throws X {
     for (Extension<T> extension : dynamicSet.entries()) {
-      PluginContext.runLogExceptions(extension, extensionImplConsumer, exceptionClass);
+      PluginContext.runLogExceptions(
+          pluginMetrics, extension, extensionImplConsumer, exceptionClass);
     }
   }
 }

java/com/google/gerrit/server/plugincontext/PluginSetEntryContext.java

@@ -21,6 +21,7 @@ import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.server.plugincontext.PluginContext.CheckedExtensionImplFunction;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplConsumer;
 import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplFunction;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 
 /**
  * Context to invoke an extension from {@link DynamicSet}.
@@ -81,9 +82,11 @@ import com.google.gerrit.server.plugincontext.PluginContext.ExtensionImplFunctio
  */
 public class PluginSetEntryContext<T> {
   private final Extension<T> extension;
+  private final PluginMetrics pluginMetrics;
 
-  PluginSetEntryContext(Extension<T> extension) {
+  PluginSetEntryContext(Extension<T> extension, PluginMetrics pluginMetrics) {
     this.extension = checkNotNull(extension);
+    this.pluginMetrics = pluginMetrics;
   }
 
   /**
@@ -117,7 +120,7 @@ public class PluginSetEntryContext<T> {
    * @param extensionImplConsumer consumer that invokes the extension
    */
   public void run(ExtensionImplConsumer<T> extensionImplConsumer) {
-    PluginContext.runLogExceptions(extension, extensionImplConsumer);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionImplConsumer);
   }
 
   /**
@@ -131,7 +134,7 @@ public class PluginSetEntryContext<T> {
    */
   public <X extends Exception> void run(
       ExtensionImplConsumer<T> extensionImplConsumer, Class<X> exceptionClass) throws X {
-    PluginContext.runLogExceptions(extension, extensionImplConsumer, exceptionClass);
+    PluginContext.runLogExceptions(pluginMetrics, extension, extensionImplConsumer, exceptionClass);
   }
 
   /**

javatests/com/google/gerrit/server/account/UniversalGroupBackendTest.java

@@ -35,6 +35,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 import com.google.gerrit.server.plugincontext.PluginSetContext;
 import com.google.gerrit.testing.GerritBaseTests;
 import java.util.Set;
@@ -57,7 +58,9 @@ public class UniversalGroupBackendTest extends GerritBaseTests {
     replay(user);
     backends = new DynamicSet<>();
     backends.add("gerrit", new SystemGroupBackend(new Config()));
-    backend = new UniversalGroupBackend(new PluginSetContext<>(backends));
+    backend =
+        new UniversalGroupBackend(
+            new PluginSetContext<>(backends, PluginMetrics.DISABLED_INSTANCE));
   }
 
   @Test
@@ -125,7 +128,9 @@ public class UniversalGroupBackendTest extends GerritBaseTests {
 
     backends = new DynamicSet<>();
     backends.add("gerrit", backend);
-    backend = new UniversalGroupBackend(new PluginSetContext<>(backends));
+    backend =
+        new UniversalGroupBackend(
+            new PluginSetContext<>(backends, PluginMetrics.DISABLED_INSTANCE));
 
     GroupMembership checker = backend.membershipsOf(member);
     assertFalse(checker.contains(REGISTERED_USERS));