Introduce metric for caches that fall back to default config
Cache chroniclemap relies heavily on configuration (especially on
average key and values sizes) so that its performance can be optimal.
What is more starting with `stable-3.4` there are 7 core caches that
are missing their configuration in Defaults and use fall-back
configuration. It is strongly adviced to auto-tune them (see
'tuning.md') and store dedicated configuration in 'gerrit.config'.
Issuing a warning in such case is not a perfect solution as Gerrit core
caches are initiated very early during the server start hence messages
are not always visible in the log. For this reason the following metric
was introduced:
* 'cache/chroniclemap/caches_without_configuration' that could be used
as an alarm source as it contains the total number of caches that
miss the dedicated configuration in 'gerrit.config' and use defaults
Notes:
* adding cache configuration to `gerrit.config` is not considered as
default configuration
* additionally, the 'auto-adjust-caches' command was extended with
'adjust-caches-on-defaults' option (see the tuning.md for details)
that it can be used to tune all caches that fall back to defaults
Bug: Issue 15865
Change-Id: I8b0835c6d8a8ae96832371b79afd2a11f69f09b9
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
index e7e1415..a07d8c4 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
@@ -27,6 +27,7 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
+import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -52,8 +53,10 @@
private final ChronicleMapCacheConfig.Factory configFactory;
private final Path cacheDir;
private final AdministerCachePermission adminCachePermission;
+ private final CachesWithoutChronicleMapConfigMetric metric;
private boolean dryRun;
+ private boolean adjustCachesOnDefaults;
private Optional<Long> optionalMaxEntries = Optional.empty();
private Set<String> cacheNames = new HashSet<>();
@@ -63,11 +66,13 @@
SitePaths site,
DynamicMap<Cache<?, ?>> cacheMap,
ChronicleMapCacheConfig.Factory configFactory,
- AdministerCachePermission adminCachePermission) {
+ AdministerCachePermission adminCachePermission,
+ CachesWithoutChronicleMapConfigMetric metric) {
this.cacheMap = cacheMap;
this.configFactory = configFactory;
this.cacheDir = getCacheDir(site, cfg.getString("cache", null, "directory"));
this.adminCachePermission = adminCachePermission;
+ this.metric = metric;
}
public boolean isDryRun() {
@@ -78,6 +83,14 @@
this.dryRun = dryRun;
}
+ public boolean isAdjustCachesOnDefaults() {
+ return adjustCachesOnDefaults;
+ }
+
+ public void setAdjustCachesOnDefaults(boolean adjustCachesOnDefaults) {
+ this.adjustCachesOnDefaults = adjustCachesOnDefaults;
+ }
+
public Optional<Long> getOptionalMaxEntries() {
return optionalMaxEntries;
}
@@ -262,6 +275,13 @@
@SuppressWarnings({"unchecked", "rawtypes"})
private Map<String, ChronicleMapCacheImpl<Object, Object>> getChronicleMapCaches() {
+ if (isAdjustCachesOnDefaults()) {
+ if (metric.cachesOnDefaults().isEmpty()) {
+ return Collections.emptyMap();
+ }
+ cacheNames.addAll(metric.cachesOnDefaults());
+ }
+
return cacheMap.plugins().stream()
.map(cacheMap::byPlugin)
.flatMap(
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesCommand.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesCommand.java
index 580bd98..9e7efd3 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesCommand.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesCommand.java
@@ -49,6 +49,14 @@
autoAdjustCachesEngine.setOptionalMaxEntries(Optional.of(maxEntries));
}
+ @Option(
+ name = "--adjust-caches-on-defaults",
+ aliases = {"-a"},
+ usage = "Adjust caches that fall back to default configuration.")
+ public void setAdjustCachesOnDefaults(boolean adjustCachesOnDefaults) {
+ autoAdjustCachesEngine.setAdjustCachesOnDefaults(adjustCachesOnDefaults);
+ }
+
@Argument(
index = 0,
required = false,
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesServlet.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesServlet.java
index 49f7a97..91950ec 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesServlet.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesServlet.java
@@ -57,6 +57,11 @@
.or(() -> Optional.ofNullable(req.getParameter("d")))
.isPresent());
+ autoAdjustCachesEngine.setAdjustCachesOnDefaults(
+ Optional.ofNullable(req.getParameter("adjust-caches-on-defaults"))
+ .or(() -> Optional.ofNullable(req.getParameter("a")))
+ .isPresent());
+
autoAdjustCachesEngine.setOptionalMaxEntries(
Optional.ofNullable(req.getParameter("max-entries"))
.or(() -> Optional.ofNullable(req.getParameter("m")))
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CachesWithoutChronicleMapConfigMetric.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CachesWithoutChronicleMapConfigMetric.java
new file mode 100644
index 0000000..0443fc6
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CachesWithoutChronicleMapConfigMetric.java
@@ -0,0 +1,57 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// 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.googlesource.gerrit.modules.cache.chroniclemap;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+@Singleton
+class CachesWithoutChronicleMapConfigMetric {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final Set<String> uniqueCacheNames;
+ private final Counter0 numberOfCachesWithoutConfig;
+
+ @Inject
+ CachesWithoutChronicleMapConfigMetric(MetricMaker metricMaker) {
+ this.uniqueCacheNames = Collections.synchronizedSet(new HashSet<>());
+
+ String metricName = "cache/chroniclemap/caches_without_configuration";
+ this.numberOfCachesWithoutConfig =
+ metricMaker.newCounter(
+ metricName,
+ new Description(
+ "The number of caches that have no chronicle map configuration provided in 'gerrit.config' and use defaults.")
+ .setUnit("caches"));
+ }
+
+ void incrementForCache(String name) {
+ if (uniqueCacheNames.add(name)) {
+ numberOfCachesWithoutConfig.increment();
+ logger.atWarning().log("Fall back to default configuration for '%s' cache", name);
+ }
+ }
+
+ Set<String> cachesOnDefaults() {
+ return uniqueCacheNames;
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
index 429d2cc..96d7ac3 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
@@ -22,6 +22,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.File;
@@ -70,6 +71,7 @@
@AssistedInject
ChronicleMapCacheConfig(
@GerritServerConfig Config cfg,
+ CachesWithoutChronicleMapConfigMetric cachesWithoutConfigMetric,
@Assisted("ConfigKey") String configKey,
@Assisted File cacheFile,
@Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
@@ -77,6 +79,7 @@
this(
cfg,
+ cachesWithoutConfigMetric,
configKey,
cacheFile,
expireAfterWrite,
@@ -97,6 +100,7 @@
@AssistedInject
ChronicleMapCacheConfig(
@GerritServerConfig Config cfg,
+ CachesWithoutChronicleMapConfigMetric cachesWithoutConfigMetric,
@Assisted("ConfigKey") String configKey,
@Assisted File cacheFile,
@Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
@@ -153,6 +157,8 @@
}
this.persistIndexEvery = Duration.ofSeconds(persistIndexEverySeconds);
this.persistIndexEveryNthPrune = persistIndexEverySeconds / PRUNE_DELAY;
+
+ emitMetricForMissingConfig(cfg, cachesWithoutConfigMetric, configKey);
}
public int getPercentageFreeSpaceEvictionThreshold() {
@@ -207,6 +213,28 @@
return configKey;
}
+ private static void emitMetricForMissingConfig(
+ Config cfg,
+ CachesWithoutChronicleMapConfigMetric cachesWithoutConfigMetric,
+ String configKey) {
+ if (!cfg.getSubsections("cache").stream()
+ .filter(cache -> cache.equalsIgnoreCase(configKey))
+ .filter(
+ cache ->
+ isConfigured(cfg, cache, "maxEntries")
+ || isConfigured(cfg, cache, "avgKeySize")
+ || isConfigured(cfg, cache, "avgValueSize")
+ || isConfigured(cfg, cache, "maxBloatFactor"))
+ .findAny()
+ .isPresent()) {
+ cachesWithoutConfigMetric.incrementForCache(configKey);
+ }
+ }
+
+ private static boolean isConfigured(Config cfg, String configKey, String name) {
+ return cfg.getString("cache", configKey, name) != null;
+ }
+
private static File resolveIndexFile(File persistedCacheFile) {
String cacheFileName = persistedCacheFile.getName();
String indexFileName = String.format("%s.index", FilenameUtils.getBaseName(cacheFileName));
@@ -218,6 +246,7 @@
return duration != null ? duration.getSeconds() : 0;
}
+ @Singleton
static class Defaults {
public static final long DEFAULT_MAX_ENTRIES = 1000;
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheModule.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheModule.java
index 99fe5c1..a9b3856 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheModule.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheModule.java
@@ -26,5 +26,6 @@
factory(ChronicleMapCacheConfig.Factory.class);
bind(PersistentCacheFactory.class).to(ChronicleMapCacheFactory.class);
listener().to(ChronicleMapCacheFactory.class);
+ bind(CachesWithoutChronicleMapConfigMetric.class).asEagerSingleton();
}
}
diff --git a/src/main/resources/Documentation/metrics.md b/src/main/resources/Documentation/metrics.md
index e1ff273..0dca368 100644
--- a/src/main/resources/Documentation/metrics.md
+++ b/src/main/resources/Documentation/metrics.md
@@ -52,3 +52,7 @@
* cache/chroniclemap/keys_index_persist_failures_<cache-name>
: The number of errors caught when persist cache index to file operation was performed
+
+* cache/chroniclemap/caches_without_configuration
+ : The number of caches that have no chronicle map configuration provided in `gerrit.config`
+ and fall back to defaults
diff --git a/src/main/resources/Documentation/tuning.md b/src/main/resources/Documentation/tuning.md
index 77078a4..7abb4d0 100644
--- a/src/main/resources/Documentation/tuning.md
+++ b/src/main/resources/Documentation/tuning.md
@@ -148,7 +148,7 @@
* You can now run an the tuning command:
```bash
-ssh -p 29418 admin@<gerrit-server> cache-chroniclemap auto-adjust-caches [--dry-run] [cache-name]
+ssh -p 29418 admin@<gerrit-server> cache-chroniclemap auto-adjust-caches [--dry-run] [--adjust-caches-on-defaults] [cache-name]
```
* You can also use the REST-API:
@@ -180,6 +180,18 @@
to increase the size of a particular cache only you should be using this
together with the `cache-name` parameter.
+* `--adjust-caches-on-defaults` or `-a` (SSH), `?adjust-caches-on-defaults` or `?a` (REST-API)
+ optional parameter
+
+The alternative to `cache-name` parameter that allows to tune all caches that are
+not configured in `gerrit.config` and thus are relying on defaults.
+Gerrit's admin can monitor `cache/chroniclemap/caches_without_configuration` metric
+(as described in
+[Detect caches that rely on defaults](#detect-caches-that-rely-on-defaults)
+section) to detect caches that rely on those defaults.
+Note that running `adjust-caches-on-defaults` in case when all caches are explicitly
+configured doesn't result in any tuning being performed.
+
* `cache-name` (SSH), `?CACHE_NAME=cache-name` (REST-API) optional restriction of the caches
to analyze and auto-tune. The parameter can be repeated multiple times for analyzing
multiple caches. By default, analyze and adjust all persistent caches.
@@ -316,4 +328,19 @@
4. restart gerrit-2
Once you have tested gerrit-2 and you are happy with the results you can perform
-steps *1.* to *4.* for `gerrit-1`.
\ No newline at end of file
+steps *1.* to *4.* for `gerrit-1`.
+
+## Detect caches that rely on defaults
+
+Gerrit increments `cache/chroniclemap/caches_without_configuration`
+metric when persistent cache that has no configuration in `gerrit.config` IOW
+(relies on fallback defaults) is instantiated.
+
+*Notes:*
+* Additionally a warning is issued to the `error_log` but Gerrit's
+ core caches are initiated earlier than logging subsystem hence seeing them
+ requires starting the server in `run` mode.
+* One can use `auto-adjust-caches` command with `adjust-caches-on-defaults` option
+ (see the [auto-adjust-chronicle-map-caches](#auto-adjust-chronicle-map-caches)
+ section for details) in order to provide explicit defaults for caches in
+ question.
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
index 71cb86e..39e832a 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
@@ -268,6 +268,23 @@
.hasSize(1);
}
+ @Test
+ public void shouldAdjustCachesOnDefaultsWhenSelected() throws Exception {
+ assertThat(
+ Joiner.on('\n').join(tunedFileNamesSet((n) -> n.contains(TEST_CACHE_FILENAME_TUNED))))
+ .isEmpty();
+
+ testCache.get(TEST_CACHE_KEY_100_CHARS);
+ String tuneResult = adminSshSession.exec(SSH_CMD + " --adjust-caches-on-defaults");
+ adminSshSession.assertSuccess();
+
+ assertThat(configResult(tuneResult, CONFIG_HEADER).getSubsections("cache"))
+ .contains(TEST_CACHE_NAME);
+ assertThat(
+ Joiner.on('\n').join(tunedFileNamesSet((n) -> n.contains(TEST_CACHE_FILENAME_TUNED))))
+ .isNotEmpty();
+ }
+
private Config configResult(String result, @Nullable String configHeader)
throws ConfigInvalidException {
Config configResult = new Config();
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
index 7f705f3..d030732 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
@@ -21,6 +21,9 @@
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.Defaults.DEFAULT_PERCENTAGE_FREE_SPACE_EVICTION_THRESHOLD;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.Defaults.DEFAULT_PERSIST_INDEX_EVERY;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheFactory.PRUNE_DELAY;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
import com.google.gerrit.server.config.SitePaths;
import java.io.File;
@@ -33,7 +36,11 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+@RunWith(MockitoJUnitRunner.class)
public class ChronicleMapCacheConfigTest {
private final String cacheDirectory = ".";
@@ -47,6 +54,8 @@
private SitePaths sitePaths;
private StoredConfig gerritConfig;
+ @Mock CachesWithoutChronicleMapConfigMetric cachesWithoutConfigMetricMock;
+
@Before
public void setUp() throws Exception {
sitePaths = new SitePaths(temporaryFolder.newFolder().toPath());
@@ -216,6 +225,21 @@
assertThat(configUnderTest.getPersistIndexEveryNthPrune()).isEqualTo(2L);
}
+ @Test
+ public void shouldIncrementCacheMetricWhenCacheHasNoDedicatedConfiguration() {
+ configUnderTest(gerritConfig);
+
+ verify(cachesWithoutConfigMetricMock, atLeastOnce()).incrementForCache(cacheKey);
+ }
+
+ @Test
+ public void shouldNotIncrementCacheMetricWhenCacheHasAtLeastSingleParameterConfigured() {
+ gerritConfig.setLong("cache", cacheKey, "maxEntries", 1L);
+ configUnderTest(gerritConfig);
+
+ verify(cachesWithoutConfigMetricMock, never()).incrementForCache(cacheName);
+ }
+
private ChronicleMapCacheConfig configUnderTest(StoredConfig gerritConfig) {
File cacheFile =
ChronicleMapCacheFactory.fileName(
@@ -226,6 +250,11 @@
.toFile();
return new ChronicleMapCacheConfig(
- gerritConfig, cacheKey, cacheFile, expireAfterWrite, refreshAfterWrite);
+ gerritConfig,
+ cachesWithoutConfigMetricMock,
+ cacheKey,
+ cacheFile,
+ expireAfterWrite,
+ refreshAfterWrite);
}
}
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
index 72f23e5..b943c07 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.Mockito.mock;
import com.codahale.metrics.Counter;
import com.codahale.metrics.Gauge;
@@ -611,6 +612,7 @@
ChronicleMapCacheConfig config =
new ChronicleMapCacheConfig(
gerritConfig,
+ mock(CachesWithoutChronicleMapConfigMetric.class),
cacheDef.configKey(),
persistentFile,
expireAfterWrite != null ? expireAfterWrite : Duration.ZERO,
