QueryProcessor: Create Singleton metrics instances for subclasses

When change.QueryProcessor was refactored to support
AccountQueryProcessor in change Iaa76a69ff9, the Singleton Metrics class
became a per-instance class. This does not appear to be the intent for
classes creating metrics in general and specifically is a problem when
plugins get a query processor instance from a provider because then the
PluginMetricsMaker is used to create the new metric. That
PluginMetricsMaker keeps a reference to any created metrics instances,
which means a plugin that runs queries would forever accumulate memory.

Fix that issue by creating Singleton subclasses of
QueryProcessor.Metrics for each of QueryProcessor's subclasses. This way
the provided *QueryProcessor instances will always get the appropriate
Metrics instance without continually creating new ones.

Change-Id: I7cf5471e29a4357df65af67864f1517133db1ecc
Release-Notes: Fixed potential OOM due to *QueryProcessor classes creating new metrics whenever an instance was provided

5 files changed, 47 insertions, 11 deletions

diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index 98e49f749f..b7584c6f7c 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -61,7 +61,7 @@ public abstract class QueryProcessor<T> {
   protected static class Metrics {
     final Timer1<String> executionTime;
 
-    Metrics(MetricMaker metricMaker) {
+    protected Metrics(MetricMaker metricMaker) {
       executionTime =
           metricMaker.newTimer(
               "query/query_latency",
@@ -95,14 +95,14 @@ public abstract class QueryProcessor<T> {
   private Set<String> requestedFields;
 
   protected QueryProcessor(
-      MetricMaker metricMaker,
+      Metrics metrics,
       SchemaDefinitions<T> schemaDef,
       IndexConfig indexConfig,
       IndexCollection<?, T, ? extends Index<?, T>> indexes,
       IndexRewriter<T> rewriter,
       String limitField,
       IntSupplier userQueryLimit) {
-    this.metrics = new Metrics(metricMaker);
+    this.metrics = metrics;
     this.schemaDef = schemaDef;
     this.indexConfig = indexConfig;
     this.indexes = indexes;
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
index eef913e13a..d812eefa49 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
@@ -34,6 +34,7 @@ import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
 import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.Singleton;
 
 /**
  * Query processor for the account index.
@@ -46,6 +47,14 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
   private final Sequences sequences;
   private final IndexConfig indexConfig;
 
+  @Singleton
+  protected static class AccountQueryMetrics extends QueryProcessor.Metrics {
+    @Inject
+    protected AccountQueryMetrics(MetricMaker metricMaker) {
+      super(metricMaker);
+    }
+  }
+
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
     checkState(
@@ -57,14 +66,14 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
   protected AccountQueryProcessor(
       Provider<CurrentUser> userProvider,
       AccountLimits.Factory limitsFactory,
-      MetricMaker metricMaker,
+      AccountQueryMetrics accountQueryMetrics,
       IndexConfig indexConfig,
       AccountIndexCollection indexes,
       AccountIndexRewriter rewriter,
       AccountControl.Factory accountControlFactory,
       Sequences sequences) {
     super(
-        metricMaker,
+        accountQueryMetrics,
         AccountSchemaDefinitions.INSTANCE,
         indexConfig,
         indexes,
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index b7dc127051..3097224246 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -43,6 +43,7 @@ import com.google.gerrit.server.index.change.IndexedChangeQuery;
 import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.Singleton;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -66,6 +67,14 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
   private final Sequences sequences;
   private final IndexConfig indexConfig;
 
+  @Singleton
+  protected static class ChangeQueryMetrics extends QueryProcessor.Metrics {
+    @Inject
+    protected ChangeQueryMetrics(MetricMaker metricMaker) {
+      super(metricMaker);
+    }
+  }
+
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
     checkState(
@@ -77,7 +86,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
   ChangeQueryProcessor(
       Provider<CurrentUser> userProvider,
       AccountLimits.Factory limitsFactory,
-      MetricMaker metricMaker,
+      ChangeQueryMetrics changeQueryMetrics,
       IndexConfig indexConfig,
       ChangeIndexCollection indexes,
       ChangeIndexRewriter rewriter,
@@ -85,7 +94,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
       ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory,
       DynamicSet<ChangePluginDefinedInfoFactory> changePluginDefinedInfoFactories) {
     super(
-        metricMaker,
+        changeQueryMetrics,
         ChangeSchemaDefinitions.INSTANCE,
         indexConfig,
         indexes,
diff --git a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
index 344a978e93..74c8d397d4 100644
--- a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
@@ -34,6 +34,7 @@ import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
 import com.google.gerrit.server.notedb.Sequences;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.Singleton;
 
 /**
  * Query processor for the group index.
@@ -47,6 +48,14 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
   private final Sequences sequences;
   private final IndexConfig indexConfig;
 
+  @Singleton
+  protected static class GroupQueryMetrics extends QueryProcessor.Metrics {
+    @Inject
+    protected GroupQueryMetrics(MetricMaker metricMaker) {
+      super(metricMaker);
+    }
+  }
+
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
     checkState(
@@ -58,14 +67,14 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> {
   protected GroupQueryProcessor(
       Provider<CurrentUser> userProvider,
       AccountLimits.Factory limitsFactory,
-      MetricMaker metricMaker,
+      GroupQueryMetrics groupQueryMetrics,
       IndexConfig indexConfig,
       GroupIndexCollection indexes,
       GroupIndexRewriter rewriter,
       GroupControl.GenericFactory groupControlFactory,
       Sequences sequences) {
     super(
-        metricMaker,
+        groupQueryMetrics,
         GroupSchemaDefinitions.INSTANCE,
         indexConfig,
         indexes,
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
index 3877c2546c..ddc7ccc9af 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
@@ -34,6 +34,7 @@ import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.Singleton;
 
 /**
  * Query processor for the project index.
@@ -49,6 +50,14 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
   private final ProjectCache projectCache;
   private final IndexConfig indexConfig;
 
+  @Singleton
+  protected static class ProjectQueryMetrics extends QueryProcessor.Metrics {
+    @Inject
+    protected ProjectQueryMetrics(MetricMaker metricMaker) {
+      super(metricMaker);
+    }
+  }
+
   static {
     // It is assumed that basic rewrites do not touch visibleto predicates.
     checkState(
@@ -60,14 +69,14 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> {
   protected ProjectQueryProcessor(
       Provider<CurrentUser> userProvider,
       AccountLimits.Factory limitsFactory,
-      MetricMaker metricMaker,
+      ProjectQueryMetrics projectQueryMetrics,
       IndexConfig indexConfig,
       ProjectIndexCollection indexes,
       ProjectIndexRewriter rewriter,
       PermissionBackend permissionBackend,
       ProjectCache projectCache) {
     super(
-        metricMaker,
+        projectQueryMetrics,
         ProjectSchemaDefinitions.INSTANCE,
         indexConfig,
         indexes,