From a2d1986e8b0c68cf0db86e3623868307fc22d8a8 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 8 Mar 2024 18:49:03 -0800 Subject: [PATCH] Addressed Sagar's comment on allowing user to pick which dimension combinations to track Signed-off-by: Peter Alfonsi --- .../cache/store/disk/EhcacheDiskCache.java | 2 +- .../cache/stats/MultiDimensionCacheStats.java | 67 ++++++---- .../common/cache/stats/StatsHolder.java | 110 ++++++++++++++- .../cache/store/OpenSearchOnHeapCache.java | 2 +- .../stats/MultiDimensionCacheStatsTests.java | 125 +++++++++++++++++- .../common/cache/stats/StatsHolderTests.java | 17 ++- 6 files changed, 284 insertions(+), 39 deletions(-) diff --git a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java index ba199edb87ef3..4c7ec7b5ff6b4 100644 --- a/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java +++ b/plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java @@ -158,7 +158,7 @@ private EhcacheDiskCache(Builder builder) { ); this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder); List dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - this.statsHolder = new StatsHolder(dimensionNames, builder.getSettings()); + this.statsHolder = new StatsHolder(dimensionNames, builder.getSettings(), StatsHolder.TrackingMode.ALL_COMBINATIONS); } @SuppressWarnings({ "rawtypes" }) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/MultiDimensionCacheStats.java b/server/src/main/java/org/opensearch/common/cache/stats/MultiDimensionCacheStats.java index 7a62b8d0b235b..c2951c6ea636a 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/MultiDimensionCacheStats.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/MultiDimensionCacheStats.java @@ -13,7 +13,9 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.ConcurrentMap; /** @@ -23,11 +25,6 @@ */ public class MultiDimensionCacheStats implements CacheStats { - /** - * For memory purposes, don't track stats for more than this many distinct combinations of dimension values. - */ - public final static int DEFAULT_MAX_DIMENSION_VALUES = 20_000; - // The value of the tier dimension for entries in this Stats object. This is handled separately for efficiency, // as it always has the same value for every entry in the stats object. // Package-private for testing. @@ -62,28 +59,31 @@ public CacheStatsResponse getTotalStats() { /** * Get the stats response aggregated by dimensions. If there are no values for the specified dimensions, - * returns an all-zero response. + * returns an all-zero response. If the specified dimensions don't form a valid key, as determined by the statsHolder's + * tracking mode, throws an IllegalArgumentException. */ @Override public CacheStatsResponse getStatsByDimensions(List dimensions) { - if (!checkDimensionNames(dimensions)) { - throw new IllegalArgumentException("Can't get stats for unrecognized dimensions"); + List modifiedDimensions = new ArrayList<>(dimensions); + CacheStatsDimension tierDim = getTierDimension(dimensions); + if (tierDim != null) { + modifiedDimensions.remove(tierDim); + } + + if (!checkDimensions(modifiedDimensions)) { + throw new IllegalArgumentException("Can't retrieve stats for this combination of dimensions"); } - CacheStatsDimension tierDim = getTierDimension(dimensions); if (tierDim == null || tierDim.dimensionValue.equals(tierDimensionValue)) { // If there is no tier dimension, or if the tier dimension value matches the one for this stats object, return an aggregated // response over the non-tier dimensions - List modifiedDimensions = new ArrayList<>(dimensions); - if (tierDim != null) { - modifiedDimensions.remove(tierDim); - } - ConcurrentMap map = statsHolder.getStatsMap(); - CacheStatsResponse response = new CacheStatsResponse(); - if (modifiedDimensions.size() == statsHolder.getDimensionNames().size()) { + // In the SEPARATE_DIMENSIONS_ONLY and SPECIFIC_COMBINATIONS cases, we don't do any adding; just return directly from the map. + // Also do this if mode is ALL_COMBINATIONS and our dimensions have a value for every dimension name. + if (statsHolder.mode != StatsHolder.TrackingMode.ALL_COMBINATIONS + || modifiedDimensions.size() == statsHolder.getDimensionNames().size()) { CacheStatsResponse resultFromMap = map.getOrDefault(new StatsHolder.Key(modifiedDimensions), new CacheStatsResponse()); response.add(resultFromMap); // Again return a copy return response; @@ -111,17 +111,38 @@ private CacheStatsDimension getTierDimension(List dimension return null; } - private boolean checkDimensionNames(List dimensions) { - for (CacheStatsDimension dim : dimensions) { - if (!(statsHolder.getDimensionNames().contains(dim.dimensionName) - || dim.dimensionName.equals(CacheStatsDimension.TIER_DIMENSION_NAME))) { - // Reject dimension names that aren't in the list and aren't the tier dimension - return false; - } + // Check the dimensions passed in are a valid request, according to the stats holder's tracking mode + private boolean checkDimensions(List dimensions) { + switch (statsHolder.mode) { + case SEPARATE_DIMENSIONS_ONLY: + if (!(dimensions.size() == 1 && statsHolder.getDimensionNames().contains(dimensions.get(0).dimensionName))) { + return false; + } + break; + case ALL_COMBINATIONS: + for (CacheStatsDimension dim : dimensions) { + if (!statsHolder.getDimensionNames().contains(dim.dimensionName)) { + return false; + } + } + break; + case SPECIFIC_COMBINATIONS: + if (!statsHolder.getSpecificCombinations().contains(getDimensionNamesSet(dimensions))) { + return false; + } + break; } return true; } + private Set getDimensionNamesSet(List dimensions) { + Set dimSet = new HashSet<>(); + for (CacheStatsDimension dim : dimensions) { + dimSet.add(dim.dimensionName); + } + return dimSet; + } + @Override public long getTotalHits() { return statsHolder.getTotalStats().getHits(); diff --git a/server/src/main/java/org/opensearch/common/cache/stats/StatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/StatsHolder.java index c9a0cc691aaa1..7a8ece58438ed 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/StatsHolder.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/StatsHolder.java @@ -17,6 +17,7 @@ import org.opensearch.core.common.io.stream.Writeable; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -40,6 +41,33 @@ public class StatsHolder implements Writeable { // The list of permitted dimensions. private final List dimensionNames; + /** + * Determines which combinations of dimension values are tracked separately by this StatsHolder. In every case, + * incoming keys still must have all dimension values populated. + */ + public enum TrackingMode { + /** + * Tracks stats for each dimension separately. Does not support retrieving stats by combinations of dimension values, + * only by a single dimension value. + */ + SEPARATE_DIMENSIONS_ONLY, + /** + * Tracks stats for every combination of dimension values. Can retrieve stats for any combination of dimensions, + * by adding together the combinations. + */ + ALL_COMBINATIONS, + /** + * Tracks stats for a specified subset of combinations. Each combination is kept aggregated in memory. Only stats for + * the pre-specified combinations can be retrieved. + */ + SPECIFIC_COMBINATIONS + } + + // The mode for this instance. + public final TrackingMode mode; + // The specific combinations of dimension names to track, if mode is SPECIFIC_COMBINATIONS. + private final Set> specificCombinations; + // A map from a set of cache stats dimensions -> stats for that combination of dimensions. private final ConcurrentMap statsMap; @@ -48,11 +76,30 @@ public class StatsHolder implements Writeable { private final Logger logger = LogManager.getLogger(StatsHolder.class); - public StatsHolder(List dimensionNames, Settings settings) { + public StatsHolder(List dimensionNames, Settings settings, TrackingMode mode) { + assert (!mode.equals(TrackingMode.SPECIFIC_COMBINATIONS)) + : "Must use constructor specifying specificCombinations when tracking mode is set to SPECIFIC_COMBINATIONS"; this.dimensionNames = dimensionNames; this.statsMap = new ConcurrentHashMap<>(); this.totalStats = new CacheStatsResponse(); this.maxDimensionValues = MAX_DIMENSION_VALUES_SETTING.get(settings); + this.mode = mode; + this.specificCombinations = new HashSet<>(); + } + + public StatsHolder(List dimensionNames, Settings settings, TrackingMode mode, Set> specificCombinations) { + if (!mode.equals(TrackingMode.SPECIFIC_COMBINATIONS)) { + logger.warn("Ignoring specific combinations; tracking mode is not set to SPECIFIC_COMBINATIONS"); + } + this.dimensionNames = dimensionNames; + this.statsMap = new ConcurrentHashMap<>(); + this.totalStats = new CacheStatsResponse(); + this.maxDimensionValues = MAX_DIMENSION_VALUES_SETTING.get(settings); + this.mode = mode; + for (Set combination : specificCombinations) { + assert combination.size() > 0 : "Must have at least one dimension name in the combination to record"; + } + this.specificCombinations = specificCombinations; } public StatsHolder(StreamInput in) throws IOException { @@ -64,6 +111,13 @@ public StatsHolder(StreamInput in) throws IOException { this.statsMap = new ConcurrentHashMap(readMap); this.totalStats = new CacheStatsResponse(in); this.maxDimensionValues = in.readVInt(); + this.mode = in.readEnum(TrackingMode.class); + this.specificCombinations = new HashSet<>(); + int numCombinations = in.readVInt(); + for (int i = 0; i < numCombinations; i++) { + String[] names = in.readStringArray(); + specificCombinations.add(new HashSet<>(List.of(names))); + } } public List getDimensionNames() { @@ -78,6 +132,7 @@ public CacheStatsResponse getTotalStats() { return totalStats; } + // For all these increment functions, the dimensions list comes from the key, and contains all dimensions present in dimensionNames. public void incrementHitsByDimensions(List dimensions) { internalIncrement(dimensions, (response, amount) -> response.hits.inc(amount), 1); } @@ -121,13 +176,51 @@ public long count() { } private void internalIncrement(List dimensions, BiConsumer incrementer, long amount) { - CacheStatsResponse stats = internalGetStats(dimensions); - incrementer.accept(stats, amount); - incrementer.accept(totalStats, amount); + for (CacheStatsResponse stats : getStatsToIncrement(dimensions)) { + incrementer.accept(stats, amount); + incrementer.accept(totalStats, amount); + } + } + + private List getStatsToIncrement(List keyDimensions) { + List result = new ArrayList<>(); + switch (mode) { + case SEPARATE_DIMENSIONS_ONLY: + for (CacheStatsDimension dim : keyDimensions) { + result.add(internalGetStats(List.of(dim))); + } + break; + case ALL_COMBINATIONS: + assert keyDimensions.size() == dimensionNames.size(); + result.add(internalGetStats(keyDimensions)); + break; + case SPECIFIC_COMBINATIONS: + for (Set combination : specificCombinations) { + result.add(internalGetStats(filterDimensionsMatchingCombination(combination, keyDimensions))); + } + break; + } + return result; + } + + private List filterDimensionsMatchingCombination( + Set dimCombination, + List dimensions + ) { + List result = new ArrayList<>(); + for (CacheStatsDimension dim : dimensions) { + if (dimCombination.contains(dim.dimensionName)) { + result.add(dim); + } + } + return result; + } + + Set> getSpecificCombinations() { + return specificCombinations; } private CacheStatsResponse internalGetStats(List dimensions) { - assert dimensions.size() == dimensionNames.size(); CacheStatsResponse response = statsMap.get(new Key(dimensions)); if (response == null) { response = new CacheStatsResponse(); @@ -151,6 +244,13 @@ public void writeTo(StreamOutput out) throws IOException { ); totalStats.writeTo(out); out.writeVInt(maxDimensionValues); + out.writeEnum(mode); + // Write Set> as repeated String[] + out.writeVInt(specificCombinations.size()); + for (Set combination : specificCombinations) { + out.writeStringArray(combination.toArray(new String[0])); + } + } /** diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java index 018f005f95a05..53b642082068d 100644 --- a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java +++ b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java @@ -58,7 +58,7 @@ public OpenSearchOnHeapCache(Builder builder) { } cache = cacheBuilder.build(); this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null"); - this.statsHolder = new StatsHolder(dimensionNames, builder.getSettings()); + this.statsHolder = new StatsHolder(dimensionNames, builder.getSettings(), StatsHolder.TrackingMode.ALL_COMBINATIONS); this.removalListener = builder.getRemovalListener(); } diff --git a/server/src/test/java/org/opensearch/common/cache/stats/MultiDimensionCacheStatsTests.java b/server/src/test/java/org/opensearch/common/cache/stats/MultiDimensionCacheStatsTests.java index cae9360a09b23..843531fe569df 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/MultiDimensionCacheStatsTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/MultiDimensionCacheStatsTests.java @@ -29,7 +29,11 @@ public class MultiDimensionCacheStatsTests extends OpenSearchTestCase { public void testSerialization() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - StatsHolder statsHolder = new StatsHolder(dimensionNames, StatsHolderTests.getSettings(20_000)); + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.ALL_COMBINATIONS + ); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); populateStats(statsHolder, usedDimensionValues, 100, 10); MultiDimensionCacheStats stats = new MultiDimensionCacheStats(statsHolder, tierDimensionValue); @@ -45,7 +49,11 @@ public void testSerialization() throws Exception { public void testAddAndGet() throws Exception { List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); - StatsHolder statsHolder = new StatsHolder(dimensionNames, StatsHolderTests.getSettings(20_000)); + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.ALL_COMBINATIONS + ); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); Map, CacheStatsResponse> expected = populateStats(statsHolder, usedDimensionValues, 1000, 10); @@ -107,7 +115,11 @@ public void testAddAndGet() throws Exception { public void testEmptyDimsList() throws Exception { // If the dimension list is empty, the map should have only one entry, from the empty set -> the total stats. - StatsHolder statsHolder = new StatsHolder(List.of(), StatsHolderTests.getSettings(20_000)); + StatsHolder statsHolder = new StatsHolder( + List.of(), + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.ALL_COMBINATIONS + ); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 100); populateStats(statsHolder, usedDimensionValues, 10, 100); MultiDimensionCacheStats stats = new MultiDimensionCacheStats(statsHolder, tierDimensionValue); @@ -123,7 +135,11 @@ public void testEmptyDimsList() throws Exception { public void testTierLogic() throws Exception { List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); - StatsHolder statsHolder = new StatsHolder(dimensionNames, StatsHolderTests.getSettings(20_000)); + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.ALL_COMBINATIONS + ); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); Map, CacheStatsResponse> expected = populateStats(statsHolder, usedDimensionValues, 1000, 10); MultiDimensionCacheStats stats = new MultiDimensionCacheStats(statsHolder, tierDimensionValue); @@ -157,6 +173,107 @@ public void testTierLogic() throws Exception { assertEquals(new CacheStatsResponse(), stats.getStatsByDimensions(List.of(wrongTierDim))); } + public void testSeparateDimensionOnlyTrackingMode() throws Exception { + List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.SEPARATE_DIMENSIONS_ONLY + ); + + Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); + Map, CacheStatsResponse> expected = populateStats(statsHolder, usedDimensionValues, 1000, 10); + MultiDimensionCacheStats stats = new MultiDimensionCacheStats(statsHolder, tierDimensionValue); + + Random rand = Randomness.get(); + + for (String dimName : dimensionNames) { + for (int i = 0; i < 20; i++) { + // pick a random already used value + List usedValues = usedDimensionValues.get(dimName); + String dimValue = usedValues.get(rand.nextInt(usedValues.size())); + CacheStatsDimension dimension = new CacheStatsDimension(dimName, dimValue); + + CacheStatsResponse expectedResponse = new CacheStatsResponse(); + for (Set combination : expected.keySet()) { + if (combination.contains(dimension)) { + expectedResponse.add(expected.get(combination)); + } + } + assertEquals(expectedResponse, stats.getStatsByDimensions(List.of(dimension))); + } + } + + List illegalArgument = List.of( + new CacheStatsDimension(dimensionNames.get(0), "a"), + new CacheStatsDimension(dimensionNames.get(1), "b") + ); + assertThrows(IllegalArgumentException.class, () -> stats.getStatsByDimensions(illegalArgument)); + } + + public void testSpecificCombinationsTrackingMode() throws Exception { + List dimensionNames = List.of("dim1", "dim2", "dim3", "dim4"); + Set> combinations = Set.of(Set.of("dim1", "dim2"), Set.of("dim3"), Set.of("dim4")); + assertThrows(AssertionError.class, () -> { + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.SPECIFIC_COMBINATIONS + ); + }); + + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + StatsHolderTests.getSettings(20_000), + StatsHolder.TrackingMode.SPECIFIC_COMBINATIONS, + combinations + ); + + Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 2); + Map, CacheStatsResponse> expected = populateStats(statsHolder, usedDimensionValues, 1000, 10); + MultiDimensionCacheStats stats = new MultiDimensionCacheStats(statsHolder, tierDimensionValue); + + Random rand = Randomness.get(); + + for (Set combination : combinations) { + for (int i = 0; i < 20; i++) { + // pick random already used values + Set dimensionsToSearch = new HashSet<>(); + for (String dimName : combination) { + List usedValues = usedDimensionValues.get(dimName); + String dimValue = usedValues.get(rand.nextInt(usedValues.size())); + dimensionsToSearch.add(new CacheStatsDimension(dimName, dimValue)); + } + + CacheStatsResponse expectedResponse = new CacheStatsResponse(); + for (Set expectedMapCombination : expected.keySet()) { + boolean includesAll = true; + for (CacheStatsDimension dimension : dimensionsToSearch) { + if (!expectedMapCombination.contains(dimension)) { + includesAll = false; + break; + } + } + if (includesAll) { + expectedResponse.add(expected.get(expectedMapCombination)); + } + } + CacheStatsResponse actual = stats.getStatsByDimensions(new ArrayList<>(dimensionsToSearch)); + assertEquals(expectedResponse, actual); + } + } + + // check other groupings of dimension values throw errors + List> invalidRequests = List.of( + List.of(new CacheStatsDimension("dim1", "a")), + List.of(new CacheStatsDimension("dim1", "a"), new CacheStatsDimension("dim3", "b")), + List.of(new CacheStatsDimension("dim3", "a"), new CacheStatsDimension("dim4", "b")) + ); + for (List invalidRequest : invalidRequests) { + assertThrows(IllegalArgumentException.class, () -> stats.getStatsByDimensions(invalidRequest)); + } + } + static Map> getUsedDimensionValues(StatsHolder statsHolder, int numValuesPerDim) { Map> usedDimensionValues = new HashMap<>(); for (int i = 0; i < statsHolder.getDimensionNames().size(); i++) { diff --git a/server/src/test/java/org/opensearch/common/cache/stats/StatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/StatsHolderTests.java index 984d7d281414c..d2aa9c67462fe 100644 --- a/server/src/test/java/org/opensearch/common/cache/stats/StatsHolderTests.java +++ b/server/src/test/java/org/opensearch/common/cache/stats/StatsHolderTests.java @@ -26,11 +26,17 @@ public class StatsHolderTests extends OpenSearchTestCase { // Since StatsHolder does not expose getter methods for aggregating stats, - // we test the incrementing functionality in combination with MultiDimensionCacheStats, + // we test the incrementing functionality and the different tracking modes in combination with MultiDimensionCacheStats, // in MultiDimensionCacheStatsTests.java. public void testSerialization() throws Exception { - List dimensionNames = List.of("dim1", "dim2"); - StatsHolder statsHolder = new StatsHolder(dimensionNames, getSettings(10_000)); + List dimensionNames = List.of("dim1", "dim2", "dim3"); + Set> specificCombinations = Set.of(Set.of("dim1"), Set.of("dim2", "dim3")); + StatsHolder statsHolder = new StatsHolder( + dimensionNames, + getSettings(10_000), + StatsHolder.TrackingMode.SPECIFIC_COMBINATIONS, + specificCombinations + ); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); populateStats(statsHolder, usedDimensionValues, 100, 10); @@ -61,14 +67,13 @@ public void testKeyEquality() throws Exception { public void testReset() throws Exception { List dimensionNames = List.of("dim1", "dim2"); - StatsHolder statsHolder = new StatsHolder(dimensionNames, getSettings(20_000)); + StatsHolder statsHolder = new StatsHolder(dimensionNames, getSettings(20_000), StatsHolder.TrackingMode.ALL_COMBINATIONS); Map> usedDimensionValues = getUsedDimensionValues(statsHolder, 10); Map, CacheStatsResponse> expected = populateStats(statsHolder, usedDimensionValues, 100, 10); statsHolder.reset(); for (Set dimSet : expected.keySet()) { - List dims = new ArrayList<>(dimSet); CacheStatsResponse originalResponse = expected.get(dimSet); originalResponse.memorySize = new CounterMetric(); originalResponse.entries = new CounterMetric(); @@ -92,6 +97,8 @@ static void checkStatsHolderEquality(StatsHolder statsHolder, StatsHolder deseri assertEquals(statsHolder.getStatsMap(), deserialized.getStatsMap()); assertEquals(statsHolder.getDimensionNames(), deserialized.getDimensionNames()); assertEquals(statsHolder.totalStats, deserialized.totalStats); + assertEquals(statsHolder.mode, deserialized.mode); + assertEquals(statsHolder.getSpecificCombinations(), deserialized.getSpecificCombinations()); } static Settings getSettings(int maxDimensionValues) {