From 9a2843ccb4b7bbcbd64497104b8ce326996244d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Cegie=C5=82ka?= Date: Mon, 13 Sep 2021 14:27:10 +0100 Subject: [PATCH 1/6] Add metrics for cache instance count --- .../main/java/com/palantir/atlasdb/AtlasDbMetricNames.java | 1 + .../palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java | 6 ++++++ .../palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java | 1 + 3 files changed, 8 insertions(+) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java index df1d3c3fded..3e5be09dd4b 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java @@ -106,4 +106,5 @@ private CellFilterMetrics() { public static final String LW_CACHE_RATIO_USED = "lockWatchCacheRatioUsed"; public static final String LW_EVENT_CACHE_FALLBACK_COUNT = "lockWatchEventCacheFallbackCount"; public static final String LW_VALUE_CACHE_FALLBACK_COUNT = "lockWatchValueCacheFallbackCount"; + public static final String LW_CACHE_INSTANCE_COUNT = "lockWatchCacheInstanceCount"; } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java index b2704975265..f2f88ab745d 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java @@ -17,6 +17,7 @@ package com.palantir.atlasdb.keyvalue.api.cache; import com.codahale.metrics.Counter; +import com.codahale.metrics.Gauge; import com.palantir.atlasdb.AtlasDbMetricNames; import com.palantir.atlasdb.util.CurrentValueMetric; import com.palantir.atlasdb.util.MetricsManager; @@ -112,4 +113,9 @@ public void setMaximumCacheSize(long maximumCacheSize) { AtlasDbMetricNames.LW_CACHE_RATIO_USED, () -> () -> cacheSize.getCount() / (double) maximumCacheSize); } + + public void setCacheInstanceCountGauge(Gauge getCacheMapCount) { + metricsManager.registerOrGetGauge( + CacheMetrics.class, AtlasDbMetricNames.LW_CACHE_INSTANCE_COUNT, () -> getCacheMapCount); + } } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java index 026df257061..fad94923137 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java @@ -51,6 +51,7 @@ final class CacheStoreImpl implements CacheStore { this.maxCacheCount = maxCacheCount; this.cacheMap = new ConcurrentHashMap<>(); this.validationProbability = validationProbability; + metrics.setCacheInstanceCountGauge(cacheMap::size); } @Override From 5868e59c4bd6301ffdcb026aa57a2268d9f4486c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Cegie=C5=82ka?= Date: Mon, 13 Sep 2021 14:39:21 +0100 Subject: [PATCH 2/6] Test metric --- .../atlasdb/keyvalue/api/cache/CacheStoreImplTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java index 8fe6d168506..0d9dd0aad83 100644 --- a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java +++ b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java @@ -19,7 +19,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import com.codahale.metrics.Gauge; import com.google.common.collect.ImmutableSet; import com.palantir.atlasdb.keyvalue.api.watch.Sequence; import com.palantir.atlasdb.keyvalue.api.watch.StartTimestamp; @@ -27,6 +29,7 @@ import io.vavr.collection.HashMap; import io.vavr.collection.HashSet; import org.junit.Test; +import org.mockito.ArgumentCaptor; public final class CacheStoreImplTest { private static final StartTimestamp TIMESTAMP_1 = StartTimestamp.of(1L); @@ -51,6 +54,7 @@ public void updatesToSnapshotStoreReflectedInCacheStore() { assertThat(cacheStore.getCache(TIMESTAMP_2)).isExactlyInstanceOf(ValidatingTransactionScopedCache.class); } + @SuppressWarnings("unchecked") @Test public void multipleCallsToGetReturnsTheSameCache() { SnapshotStore snapshotStore = new SnapshotStoreImpl(); @@ -66,6 +70,10 @@ public void multipleCallsToGetReturnsTheSameCache() { TransactionScopedCache cache2 = cacheStore.getCache(TIMESTAMP_2); assertThat(cacheStore.getCache(TIMESTAMP_1)).isEqualTo(cache1).isNotEqualTo(cache2); + + ArgumentCaptor> cacheInstanceCount = ArgumentCaptor.forClass(Gauge.class); + verify(metrics).setCacheInstanceCountGauge(cacheInstanceCount.capture()); + assertThat(cacheInstanceCount.getValue()).extracting(Gauge::getValue).isEqualTo(2); } @Test From 0a98cb7f7f822400fa1cd652a3498ceeb0c47d28 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 13 Sep 2021 13:48:12 +0000 Subject: [PATCH 3/6] Add generated changelog entries --- changelog/@unreleased/pr-5631.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-5631.v2.yml diff --git a/changelog/@unreleased/pr-5631.v2.yml b/changelog/@unreleased/pr-5631.v2.yml new file mode 100644 index 00000000000..30c3565cad5 --- /dev/null +++ b/changelog/@unreleased/pr-5631.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Add metrics for cache instance count + links: + - https://github.com/palantir/atlasdb/pull/5631 From 070a67fc8bc6adba64be3b4016f02d8b7e1c22d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Cegie=C5=82ka?= Date: Mon, 13 Sep 2021 15:01:49 +0100 Subject: [PATCH 4/6] Test metric value in all tests --- .../keyvalue/api/cache/CacheStoreImplTest.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java index 0d9dd0aad83..ee92ec48a81 100644 --- a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java +++ b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java @@ -52,9 +52,9 @@ public void updatesToSnapshotStoreReflectedInCacheStore() { ValueCacheSnapshotImpl.of(HashMap.empty(), HashSet.empty(), ImmutableSet.of())); cacheStore.createCache(TIMESTAMP_2); assertThat(cacheStore.getCache(TIMESTAMP_2)).isExactlyInstanceOf(ValidatingTransactionScopedCache.class); + assertThat(getTransactionCacheInstanceCount()).isEqualTo(1); } - @SuppressWarnings("unchecked") @Test public void multipleCallsToGetReturnsTheSameCache() { SnapshotStore snapshotStore = new SnapshotStoreImpl(); @@ -70,10 +70,7 @@ public void multipleCallsToGetReturnsTheSameCache() { TransactionScopedCache cache2 = cacheStore.getCache(TIMESTAMP_2); assertThat(cacheStore.getCache(TIMESTAMP_1)).isEqualTo(cache1).isNotEqualTo(cache2); - - ArgumentCaptor> cacheInstanceCount = ArgumentCaptor.forClass(Gauge.class); - verify(metrics).setCacheInstanceCountGauge(cacheInstanceCount.capture()); - assertThat(cacheInstanceCount.getValue()).extracting(Gauge::getValue).isEqualTo(2); + assertThat(getTransactionCacheInstanceCount()).isEqualTo(2); } @Test @@ -93,6 +90,7 @@ public void cachesExceedingMaximumCountThrows() { .isExactlyInstanceOf(TransactionFailedRetriableException.class) .hasMessage("Exceeded maximum concurrent caches; transaction can be retried, but with caching " + "disabled"); + assertThat(getTransactionCacheInstanceCount()).isEqualTo(2); } @Test @@ -107,6 +105,7 @@ public void getCacheDoesNotPersistAnything() { assertThat(cacheStore.getCache(TIMESTAMP_1)).isExactlyInstanceOf(NoOpTransactionScopedCache.class); cacheStore.createCache(TIMESTAMP_1); assertThat(cacheStore.getCache(TIMESTAMP_1)).isExactlyInstanceOf(ValidatingTransactionScopedCache.class); + assertThat(getTransactionCacheInstanceCount()).isEqualTo(1); } @Test @@ -118,5 +117,13 @@ public void noOpCachesAreNotStored() { cacheStore.createCache(TIMESTAMP_2); assertThat(cacheStore.getCache(TIMESTAMP_1)).isExactlyInstanceOf(NoOpTransactionScopedCache.class); assertThat(cacheStore.getCache(TIMESTAMP_2)).isExactlyInstanceOf(NoOpTransactionScopedCache.class); + assertThat(getTransactionCacheInstanceCount()).isEqualTo(0); + } + + @SuppressWarnings("unchecked") + private int getTransactionCacheInstanceCount() { + ArgumentCaptor> cacheInstanceCount = ArgumentCaptor.forClass(Gauge.class); + verify(metrics).setCacheInstanceCountGauge(cacheInstanceCount.capture()); + return cacheInstanceCount.getValue().getValue(); } } From 34fd5230113ba94c915952628c464509219013e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Cegie=C5=82ka?= Date: Mon, 13 Sep 2021 15:03:41 +0100 Subject: [PATCH 5/6] Style rename --- .../main/java/com/palantir/atlasdb/AtlasDbMetricNames.java | 2 +- .../com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java | 4 ++-- .../palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java | 2 +- .../atlasdb/keyvalue/api/cache/CacheStoreImplTest.java | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java index 3e5be09dd4b..432c75cfd0d 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/AtlasDbMetricNames.java @@ -106,5 +106,5 @@ private CellFilterMetrics() { public static final String LW_CACHE_RATIO_USED = "lockWatchCacheRatioUsed"; public static final String LW_EVENT_CACHE_FALLBACK_COUNT = "lockWatchEventCacheFallbackCount"; public static final String LW_VALUE_CACHE_FALLBACK_COUNT = "lockWatchValueCacheFallbackCount"; - public static final String LW_CACHE_INSTANCE_COUNT = "lockWatchCacheInstanceCount"; + public static final String LW_TRANSACTION_CACHE_INSTANCE_COUNT = "lockWatchTransactionCacheInstanceCount"; } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java index f2f88ab745d..f220f44f84d 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheMetrics.java @@ -114,8 +114,8 @@ public void setMaximumCacheSize(long maximumCacheSize) { () -> () -> cacheSize.getCount() / (double) maximumCacheSize); } - public void setCacheInstanceCountGauge(Gauge getCacheMapCount) { + public void setTransactionCacheInstanceCountGauge(Gauge getCacheMapCount) { metricsManager.registerOrGetGauge( - CacheMetrics.class, AtlasDbMetricNames.LW_CACHE_INSTANCE_COUNT, () -> getCacheMapCount); + CacheMetrics.class, AtlasDbMetricNames.LW_TRANSACTION_CACHE_INSTANCE_COUNT, () -> getCacheMapCount); } } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java index fad94923137..32439774e0f 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImpl.java @@ -51,7 +51,7 @@ final class CacheStoreImpl implements CacheStore { this.maxCacheCount = maxCacheCount; this.cacheMap = new ConcurrentHashMap<>(); this.validationProbability = validationProbability; - metrics.setCacheInstanceCountGauge(cacheMap::size); + metrics.setTransactionCacheInstanceCountGauge(cacheMap::size); } @Override diff --git a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java index ee92ec48a81..583547bcdc3 100644 --- a/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java +++ b/atlasdb-impl-shared/src/test/java/com/palantir/atlasdb/keyvalue/api/cache/CacheStoreImplTest.java @@ -123,7 +123,7 @@ public void noOpCachesAreNotStored() { @SuppressWarnings("unchecked") private int getTransactionCacheInstanceCount() { ArgumentCaptor> cacheInstanceCount = ArgumentCaptor.forClass(Gauge.class); - verify(metrics).setCacheInstanceCountGauge(cacheInstanceCount.capture()); + verify(metrics).setTransactionCacheInstanceCountGauge(cacheInstanceCount.capture()); return cacheInstanceCount.getValue().getValue(); } } From a67effe073998364779d47a2c4aa1be640627ca6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Cegie=C5=82ka?= Date: Mon, 13 Sep 2021 15:05:25 +0100 Subject: [PATCH 6/6] Add longer changelog --- changelog/@unreleased/pr-5631.v2.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/@unreleased/pr-5631.v2.yml b/changelog/@unreleased/pr-5631.v2.yml index 30c3565cad5..6cb8108f217 100644 --- a/changelog/@unreleased/pr-5631.v2.yml +++ b/changelog/@unreleased/pr-5631.v2.yml @@ -1,5 +1,5 @@ type: improvement improvement: - description: Add metrics for cache instance count + description: Add metric for tracking the count of transaction caches stored in the transaction map, connected to memory leaks such as PDS-185998 and PDS-209612 links: - https://github.com/palantir/atlasdb/pull/5631