Use a bucketed metric for the project-specific replication lag
Previously there was a metric-per-project logic for tracking the
replicaton lags between nodes. Use the proper bucket callback
to provide all the values for the metric, avoid the metrics explosion
and also the danger to re-create the same project-specific metric
multiple times.
Change-Id: I3f8e95d296a49083ceca514412ab2b79beb788ed
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatus.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatus.java
index d677df8..0253588 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatus.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatus.java
@@ -20,7 +20,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.events.ProjectDeletedListener;
-import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.CallbackMetric1;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.serialize.JavaCacheSerializer;
@@ -161,26 +161,27 @@
}
}
- void incrementLagMetric(Project.NameKey projectName) {
- logger.atFine().log("Creating replication lag metric for project %s", projectName);
- String sanitizedProjectName = SubscriberMetrics.sanitizeProjectName(projectName.get());
- metricMaker.newCallbackMetric(
- String.format("%s_%s", SubscriberMetrics.REPLICATION_LAG_SEC, sanitizedProjectName),
- Long.class,
- new Description("Replication lag for project (sec)")
- .setGauge()
- .setUnit(Description.Units.SECONDS),
- () -> getReplicationStatus(projectName.get()));
+ @VisibleForTesting
+ Runnable replicationLagMetricPerProject(CallbackMetric1<String, Long> metricCallback) {
+ return () -> {
+ if (replicationStatusPerProject.isEmpty()) {
+ metricCallback.forceCreate("");
+ } else {
+ replicationStatusPerProject.entrySet().stream()
+ .filter(e -> e.getValue() > 0)
+ .forEach(
+ e ->
+ metricCallback.set(
+ SubscriberMetrics.sanitizeProjectName(e.getKey()), e.getValue()));
+ metricCallback.prune();
+ }
+ };
}
@VisibleForTesting
public void doUpdateLag(Project.NameKey projectName, Long lag) {
cache.put(projectName.get(), lag);
replicationStatusPerProject.put(projectName.get(), lag);
-
- if (lag > 0) {
- incrementLagMetric(projectName);
- }
}
@VisibleForTesting
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/SubscriberMetrics.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/SubscriberMetrics.java
index 8eba666..bb971ff 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/SubscriberMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/consumer/SubscriberMetrics.java
@@ -14,12 +14,15 @@
package com.googlesource.gerrit.plugins.multisite.consumer;
+import com.google.gerrit.metrics.CallbackMetric1;
import com.google.gerrit.metrics.Counter1;
import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.ProjectEvent;
import com.google.gerrit.server.events.RefUpdatedEvent;
+import com.google.gerrit.server.logging.Metadata;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.multisite.MultiSiteMetrics;
@@ -39,11 +42,15 @@
"multi_site/subscriber/subscriber_replication_status/sec_behind";
private static final String REPLICATION_LAG_MSEC =
"multi_site/subscriber/subscriber_replication_status/msec_behind";
+ private static final String REPLICATION_LAG_MSEC_PROJECT =
+ "multi_site/subscriber/subscriber_replication_status/msec_behind/per_project";
private final Counter1<String> subscriberSuccessCounter;
private final Counter1<String> subscriberFailureCounter;
private final ReplicationStatus replicationStatus;
private static final Pattern isValidMetricNamePattern = Pattern.compile("[a-zA-Z0-9_-]");
+ private static final Field<String> PROJECT_NAME =
+ Field.ofString("project_name", Metadata.Builder::cacheName).build();
@Inject
public SubscriberMetrics(MetricMaker metricMaker, ReplicationStatus replicationStatus) {
@@ -75,6 +82,16 @@
.setGauge()
.setUnit(Description.Units.MILLISECONDS),
replicationStatus::getMaxLagMillis);
+
+ CallbackMetric1<String, Long> metrics =
+ metricMaker.newCallbackMetric(
+ SubscriberMetrics.REPLICATION_LAG_MSEC_PROJECT,
+ Long.class,
+ new Description("Per-project replication lag (msec)")
+ .setGauge()
+ .setUnit(Description.Units.MILLISECONDS),
+ PROJECT_NAME);
+ metricMaker.newTrigger(metrics, replicationStatus.replicationLagMetricPerProject(metrics));
}
/**
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/CallbackMetricMaker.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/CallbackMetricMaker.java
index daf1cfd..a197c91 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/CallbackMetricMaker.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/CallbackMetricMaker.java
@@ -1,9 +1,9 @@
package com.googlesource.gerrit.plugins.multisite.consumer;
-import com.google.common.base.Supplier;
-import com.google.gerrit.extensions.registration.RegistrationHandle;
+import com.google.gerrit.metrics.Counter1;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.DisabledMetricMaker;
+import com.google.gerrit.metrics.Field;
import org.junit.Ignore;
@Ignore
@@ -15,14 +15,9 @@
}
@Override
- public <V> RegistrationHandle newCallbackMetric(
- String name, Class<V> valueClass, Description desc, Supplier<V> trigger) {
+ public <F1> Counter1<F1> newCounter(String name, Description desc, Field<F1> field1) {
callbackMetricCounter += 1;
- return new RegistrationHandle() {
-
- @Override
- public void remove() {}
- };
+ return super.newCounter(name, desc, field1);
}
public void resetCallbackMetricCounter() {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatusTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatusTest.java
index d49108c..7828062 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatusTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/consumer/ReplicationStatusTest.java
@@ -15,8 +15,11 @@
package com.googlesource.gerrit.plugins.multisite.consumer;
import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.any;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import com.google.common.cache.Cache;
@@ -24,6 +27,8 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.events.ProjectDeletedListener;
+import com.google.gerrit.metrics.CallbackMetric1;
+import com.google.gerrit.metrics.DisabledMetricMaker;
import com.google.gerrit.server.project.ProjectCache;
import com.googlesource.gerrit.plugins.multisite.Configuration;
import com.googlesource.gerrit.plugins.multisite.ProjectVersionLogger;
@@ -43,9 +48,9 @@
@Mock private ProjectVersionLogger verLogger;
@Mock private ProjectCache projectCache;
@Mock private ProjectVersionRefUpdate projectVersionRefUpdate;
+ @Mock private CallbackMetric1<String, Long> perProjectReplicationLagMetricCallback;
private ReplicationStatus objectUnderTest;
private Cache<String, Long> replicationStatusCache;
- private CallbackMetricMaker callbackMetricMaker = new CallbackMetricMaker();
@Before
public void setup() throws Exception {
@@ -53,7 +58,6 @@
.thenReturn(
ImmutableSortedSet.of(Project.nameKey("projectA"), Project.nameKey("projectB")));
replicationStatusCache = CacheBuilder.newBuilder().build();
- callbackMetricMaker.resetCallbackMetricCounter();
objectUnderTest =
new ReplicationStatus(
replicationStatusCache,
@@ -62,7 +66,7 @@
projectCache,
Executors.newScheduledThreadPool(1),
new Configuration(new Config(), new Config()),
- callbackMetricMaker);
+ new DisabledMetricMaker());
}
@Test
@@ -174,10 +178,12 @@
.thenReturn(Optional.of(projectRemoteVersion));
objectUnderTest.updateReplicationLag(Project.nameKey(projectName));
+ objectUnderTest.replicationLagMetricPerProject(perProjectReplicationLagMetricCallback).run();
assertThat(replicationStatusCache.getIfPresent(projectName))
.isEqualTo(projectRemoteVersion - projectLocalVersion);
- assertThat(callbackMetricMaker.getCallbackMetricCounter()).isEqualTo(1);
+ verify(perProjectReplicationLagMetricCallback)
+ .set(eq(projectName), eq(projectRemoteVersion - projectLocalVersion));
}
@Test
@@ -189,7 +195,8 @@
objectUnderTest.updateReplicationLag(Project.nameKey(projectName));
- assertThat(callbackMetricMaker.getCallbackMetricCounter()).isEqualTo(0);
+ assertThat(replicationStatusCache.getIfPresent(projectName)).isNull();
+ verify(perProjectReplicationLagMetricCallback, never()).set(any(), any());
}
@SuppressWarnings("unchecked")
