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;