Adjust cache-chroniclemap to new metric sanitization scheme
Gerrit's I6a5366fa0f1f change improved metrics name sanitization to
avoid collisions.
Introduce CacheNameSanitizer to:
* keep the backward compatibitlity with the oridinary cache names e.g.
`diff_summary` stays `diff_summary`
* all the other cases are sanitized according to metrics sanitization
rules e.g. `very+confusing.cache#name` becomes
`very_0x2B_confusing_0x2E_cache_0x23_name`
Tests were adjusted to new sanitization rules.
Bug: Issue 307357257
Change-Id: I59732013baa1abcbfba1536c7b45d158eb746bc8
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
index 0730380..35a1cc3 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
@@ -94,7 +94,7 @@
private final Counter0 persistFailures;
private Metrics(MetricMaker metricMaker, String name) {
- String sanitizedName = metricMaker.sanitizeMetricName(name);
+ String sanitizedName = CacheNameSanitizer.sanitize(metricMaker, name);
indexSize =
metricMaker.newCallbackMetric(
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java
new file mode 100644
index 0000000..212aae3
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java
@@ -0,0 +1,41 @@
+// Copyright (C) 2023 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.gerrit.metrics.MetricMaker;
+import java.util.regex.Pattern;
+
+class CacheNameSanitizer {
+ private static final Pattern METRIC_NAME_PATTERN =
+ Pattern.compile("[a-zA-Z0-9_-]+([a-zA-Z0-9_-]+)*");
+
+ /**
+ * Detect if <code>cacheName</code> contains only metric name allowed characters (that matches
+ * {@link #METRIC_NAME_PATTERN}). Note that `/` regardless of being allowed in metric names is
+ * omitted as it denotes the sub-metric and cache name should not be treated as metric /
+ * sub-metric. Typical persistent cache name (e.g. `diff_summary`) adheres to it therefore it is
+ * returned without any modification. In all other cases call to {@link
+ * MetricMaker#sanitizeMetricName(String)} is performed so that name is sanitized. This way
+ * sanitization stays backward compatible but also non-typical cases are handled.
+ */
+ static String sanitize(MetricMaker metricMaker, String cacheName) {
+ if (METRIC_NAME_PATTERN.matcher(cacheName).matches()) {
+ return cacheName;
+ }
+ return metricMaker.sanitizeMetricName(cacheName);
+ }
+
+ private CacheNameSanitizer() {}
+}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
index c27be99..8966aa5 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
@@ -30,7 +30,7 @@
ChronicleMapStoreMetrics(String name, MetricMaker metricMaker) {
this.name = name;
- this.sanitizedName = metricMaker.sanitizeMetricName(name);
+ this.sanitizedName = CacheNameSanitizer.sanitize(metricMaker, name);
this.metricMaker = metricMaker;
this.storePutFailures =
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
index 098e8c6..380648c 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
@@ -35,7 +35,7 @@
final Timer0 serializeLatency;
private Metrics(MetricMaker metricMaker, String cacheName) {
- String sanitizedName = metricMaker.sanitizeMetricName(cacheName);
+ String sanitizedName = CacheNameSanitizer.sanitize(metricMaker, cacheName);
deserializeLatency =
metricMaker.newTimer(
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java
new file mode 100644
index 0000000..ec469e4
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2023 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 static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.modules.cache.chroniclemap.CacheNameSanitizer.sanitize;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.metrics.MetricMaker;
+import org.junit.Test;
+
+public class CacheNameSanitizerTest {
+ @Test
+ public void shouldNotSanitizeTypicalCacheName() {
+ String cacheName = "diff_summary";
+
+ assertThat(sanitize(mock(MetricMaker.class), cacheName)).isEqualTo(cacheName);
+ }
+
+ @Test
+ public void shouldNotSanitizeCacheNameWithHyphens() {
+ String cacheName = "cache_name-with-hyphens";
+
+ assertThat(sanitize(mock(MetricMaker.class), cacheName)).isEqualTo(cacheName);
+ }
+
+ @Test
+ public void shouldFallbackToMetricMakerSanitization() {
+ MetricMaker metricMaker = mock(MetricMaker.class);
+ String sanitizedName = "sanitized";
+ when(metricMaker.sanitizeMetricName(anyString())).thenReturn(sanitizedName);
+
+ assertThat(sanitize(metricMaker, "very+confusing.cache#name")).isEqualTo(sanitizedName);
+ }
+}
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 58cc4ec..7bf2f6b 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
@@ -592,7 +592,7 @@
@Test
public void shouldSanitizeUnwantedCharsInMetricNames() throws Exception {
String cacheName = "very+confusing.cache#name";
- String sanitized = "very_confusing_cache_name";
+ String sanitized = "very_0x2B_confusing_0x2E_cache_0x23_name";
String percentageFreeMetricName = "cache/chroniclemap/percentage_free_space_" + sanitized;
String autoResizeMetricName = "cache/chroniclemap/remaining_autoresizes_" + sanitized;
String maxAutoResizeMetricName = "cache/chroniclemap/max_autoresizes_" + sanitized;
