From d0a3ac2c1f4032ae4943ea8f611f7abd0f0f6217 Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Mon, 10 Aug 2020 12:21:42 -0700 Subject: [PATCH] Addressed review comments. --- docs/advanced-functionality.rst | 8 +- .../com/o19s/es/ltr/LtrQueryParserPlugin.java | 8 +- .../ltr/action/TransportLTRStatsAction.java | 11 +- .../com/o19s/es/ltr/rest/RestLTRStats.java | 4 +- .../java/com/o19s/es/ltr/stats/LTRStat.java | 25 ++--- .../java/com/o19s/es/ltr/stats/LTRStats.java | 16 +-- .../java/com/o19s/es/ltr/stats/StatName.java | 17 +-- .../suppliers/CacheStatsOnNodeSupplier.java | 39 +++++-- .../ltr/stats/suppliers/CounterSupplier.java | 24 ----- .../suppliers/PluginHealthStatusSupplier.java | 4 +- .../stats/suppliers/StoreStatsSupplier.java | 33 ++++-- .../o19s/es/ltr/action/LTRStatsActionIT.java | 101 +++++++++++++----- .../action/TransportLTRStatsActionTests.java | 12 +-- .../com/o19s/es/ltr/stats/LTRStatTests.java | 21 +--- .../com/o19s/es/ltr/stats/LTRStatsTests.java | 26 ++--- .../CacheStatsOnNodeSupplierTests.java | 35 +++--- .../stats/suppliers/CounterSupplierTests.java | 12 --- .../rest-api-spec/api/ltr.get_stats.json | 8 +- 18 files changed, 199 insertions(+), 205 deletions(-) delete mode 100644 src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java delete mode 100644 src/test/java/com/o19s/es/ltr/stats/suppliers/CounterSupplierTests.java diff --git a/docs/advanced-functionality.rst b/docs/advanced-functionality.rst index 0004fe04..343071d8 100644 --- a/docs/advanced-functionality.rst +++ b/docs/advanced-functionality.rst @@ -218,7 +218,7 @@ Stats ============================= The stats API gives the overall plugin status and statistics:: - GET /_ltr/stats + GET /_ltr/_stats { "_nodes": { @@ -267,10 +267,10 @@ The stats API gives the overall plugin status and statistics:: You can also use filters to retrieve a single stat:: - GET /_ltr/stats/{stat} + GET /_ltr/_stats/{stat} Also you can limit the information to a single node in the cluster:: - GET /_ltr/stats/nodes/{nodeId} + GET /_ltr/_stats/nodes/{nodeId} - GET /_ltr/stats/{stat}/nodes/{nodeId} + GET /_ltr/_stats/{stat}/nodes/{nodeId} diff --git a/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java b/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java index 68377360..1a1dbed4 100644 --- a/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java +++ b/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java @@ -250,13 +250,13 @@ public Collection createComponents(Client client, } private LTRStats getStats(Client client, ClusterService clusterService) { - Map> stats = new HashMap<>(); + Map stats = new HashMap<>(); stats.put(StatName.CACHE.getName(), - new LTRStat<>(false, new CacheStatsOnNodeSupplier(caches))); + new LTRStat(false, new CacheStatsOnNodeSupplier(caches))); stats.put(StatName.STORES.getName(), - new LTRStat<>(true, new StoreStatsSupplier(client, clusterService))); + new LTRStat(true, new StoreStatsSupplier(client, clusterService))); stats.put(StatName.PLUGIN_STATUS.getName(), - new LTRStat<>(true, new PluginHealthStatusSupplier(clusterService))); + new LTRStat(true, new PluginHealthStatusSupplier(clusterService))); return new LTRStats(unmodifiableMap(stats)); } diff --git a/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java b/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java index d4fface0..9feb56fd 100644 --- a/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java +++ b/src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java @@ -5,14 +5,12 @@ import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsNodesRequest; import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsNodesResponse; import com.o19s.es.ltr.stats.LTRStats; -import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.nodes.TransportNodesAction; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -39,11 +37,6 @@ public TransportLTRStatsAction(ThreadPool threadPool, this.ltrStats = ltrStats; } - @Override - protected void doExecute(Task task, LTRStatsNodesRequest request, ActionListener listener) { - super.doExecute(task, request, listener); - } - @Override protected LTRStatsNodesResponse newResponse(LTRStatsNodesRequest request, List nodeResponses, @@ -54,7 +47,7 @@ protected LTRStatsNodesResponse newResponse(LTRStatsNodesRequest request, .entrySet() .stream() .filter(e -> statsToBeRetrieved.contains(e.getKey())) - .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue())); + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getStatValue())); return new LTRStatsNodesResponse(clusterService.getClusterName(), nodeResponses, failures, clusterStats); } @@ -79,7 +72,7 @@ protected LTRStatsNodeResponse nodeOperation(LTRStatsNodeRequest request) { .entrySet() .stream() .filter(e -> statsToBeRetrieved.contains(e.getKey())) - .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue())); + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getStatValue())); return new LTRStatsNodeResponse(clusterService.localNode(), statValues); } } diff --git a/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java b/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java index 9fc4c0ca..b1e6ea0f 100644 --- a/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java +++ b/src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java @@ -25,7 +25,7 @@ * APIs to retrieve stats on the plugin usage and performance. */ public class RestLTRStats extends BaseRestHandler { - public static final String LTR_STATS_BASE_URI = "/_ltr/stats"; + public static final String LTR_STATS_BASE_URI = "/_ltr/_stats"; private static final String NAME = "learning_to_rank_stats"; @Override @@ -76,7 +76,7 @@ private Set getStatsToBeRetrieved( RestRequest request, Set validStats, List requestedStats) { if (requestedStats.contains(LTRStatsNodesRequest.ALL_STATS_KEY)) { throw new IllegalArgumentException( - String.format(Locale.getDefault(), "Request %s contains both %s and individual stats", + String.format(Locale.ROOT, "Request %s contains both %s and individual stats", request.path(), LTRStatsNodesRequest.ALL_STATS_KEY)); } diff --git a/src/main/java/com/o19s/es/ltr/stats/LTRStat.java b/src/main/java/com/o19s/es/ltr/stats/LTRStat.java index 165f6deb..43143252 100644 --- a/src/main/java/com/o19s/es/ltr/stats/LTRStat.java +++ b/src/main/java/com/o19s/es/ltr/stats/LTRStat.java @@ -1,22 +1,17 @@ package com.o19s.es.ltr.stats; -import com.o19s.es.ltr.stats.suppliers.CounterSupplier; - import java.util.function.Supplier; /** * A container for a stat provided by the plugin. Each instance is associated with - * an underlying supplier. The supplier can be a counter in which case it can be - * incremented. The stat instance also stores a flag to indicate whether this is - * a cluster level or node level stat. - * - * @param the type of the value returned by the stat. + * an underlying supplier. The stat instance also stores a flag to indicate whether + * this is a cluster level or a node level stat. */ -public class LTRStat { +public class LTRStat { private final boolean clusterLevel; - private final Supplier supplier; + private final Supplier supplier; - public LTRStat(boolean clusterLevel, Supplier supplier) { + public LTRStat(boolean clusterLevel, Supplier supplier) { this.clusterLevel = clusterLevel; this.supplier = supplier; } @@ -25,15 +20,7 @@ public boolean isClusterLevel() { return clusterLevel; } - public T getValue() { + public Object getStatValue() { return supplier.get(); } - - public void increment() { - if (!(supplier instanceof CounterSupplier)) { - throw new UnsupportedOperationException( - "cannot increment the supplier: " + supplier.getClass().getName()); - } - ((CounterSupplier) supplier).increment(); - } } diff --git a/src/main/java/com/o19s/es/ltr/stats/LTRStats.java b/src/main/java/com/o19s/es/ltr/stats/LTRStats.java index 447ae3d2..0181f5a8 100644 --- a/src/main/java/com/o19s/es/ltr/stats/LTRStats.java +++ b/src/main/java/com/o19s/es/ltr/stats/LTRStats.java @@ -8,34 +8,34 @@ * This class is the main entry-point for access to the stats that the LTR plugin keeps track of. */ public class LTRStats { - private final Map> stats; + private final Map stats; - public LTRStats(Map> stats) { + public LTRStats(Map stats) { this.stats = stats; } - public Map> getStats() { + public Map getStats() { return stats; } - public LTRStat getStat(String key) throws IllegalArgumentException { - LTRStat stat = stats.get(key); + public LTRStat getStat(String key) throws IllegalArgumentException { + LTRStat stat = stats.get(key); if (stat == null) { throw new IllegalArgumentException("Stat=\"" + key + "\" does not exist"); } return stat; } - public Map> getNodeStats() { + public Map getNodeStats() { return getClusterOrNodeStats(false); } - public Map> getClusterStats() { + public Map getClusterStats() { return getClusterOrNodeStats(true); } - private Map> getClusterOrNodeStats(Boolean isClusterStats) { + private Map getClusterOrNodeStats(Boolean isClusterStats) { return stats.entrySet() .stream() .filter(e -> e.getValue().isClusterLevel() == isClusterStats) diff --git a/src/main/java/com/o19s/es/ltr/stats/StatName.java b/src/main/java/com/o19s/es/ltr/stats/StatName.java index b38d1258..ba1c68d4 100644 --- a/src/main/java/com/o19s/es/ltr/stats/StatName.java +++ b/src/main/java/com/o19s/es/ltr/stats/StatName.java @@ -7,22 +7,7 @@ public enum StatName { PLUGIN_STATUS("status"), STORES("stores"), - CACHE("cache"), - - STORE_STATUS("status"), - STORE_FEATURE_COUNT("feature_count"), - STORE_FEATURE_SET_COUNT("featureset_count"), - STORE_MODEL_COUNT("model_count"), - - CACHE_FEATURE("feature"), - CACHE_FEATURE_SET("featureset"), - CACHE_MODEL("model"), - - CACHE_HIT_COUNT("hit_count"), - CACHE_MISS_COUNT("miss_count"), - CACHE_EVICTION_COUNT("eviction_count"), - CACHE_ENTRY_COUNT("entry_count"), - CACHE_MEMORY_USAGE_IN_BYTES("memory_usage_in_bytes"); + CACHE("cache"); private final String name; diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java index 7c1518dc..b6ca9d01 100644 --- a/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java +++ b/src/main/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplier.java @@ -1,7 +1,6 @@ package com.o19s.es.ltr.stats.suppliers; import com.o19s.es.ltr.feature.store.index.Caches; -import com.o19s.es.ltr.stats.StatName; import org.elasticsearch.common.cache.Cache; import java.util.Collections; @@ -15,6 +14,28 @@ public class CacheStatsOnNodeSupplier implements Supplier>> { private final Caches caches; + public enum Stat { + CACHE_FEATURE("feature"), + CACHE_FEATURE_SET("featureset"), + CACHE_MODEL("model"), + + CACHE_HIT_COUNT("hit_count"), + CACHE_MISS_COUNT("miss_count"), + CACHE_EVICTION_COUNT("eviction_count"), + CACHE_ENTRY_COUNT("entry_count"), + CACHE_MEMORY_USAGE_IN_BYTES("memory_usage_in_bytes"); + + private final String name; + + Stat(String name) { + this.name = name; + } + + public String getName() { + return name; + } + } + public CacheStatsOnNodeSupplier(Caches caches) { this.caches = caches; } @@ -22,19 +43,19 @@ public CacheStatsOnNodeSupplier(Caches caches) { @Override public Map> get() { Map> values = new HashMap<>(); - values.put(StatName.CACHE_FEATURE.getName(), getCacheStats(caches.featureCache())); - values.put(StatName.CACHE_FEATURE_SET.getName(), getCacheStats(caches.featureSetCache())); - values.put(StatName.CACHE_MODEL.getName(), getCacheStats(caches.modelCache())); + values.put(Stat.CACHE_FEATURE.getName(), getCacheStats(caches.featureCache())); + values.put(Stat.CACHE_FEATURE_SET.getName(), getCacheStats(caches.featureSetCache())); + values.put(Stat.CACHE_MODEL.getName(), getCacheStats(caches.modelCache())); return Collections.unmodifiableMap(values); } private Map getCacheStats(Cache cache) { Map stat = new HashMap<>(); - stat.put(StatName.CACHE_HIT_COUNT.getName(), cache.stats().getHits()); - stat.put(StatName.CACHE_MISS_COUNT.getName(), cache.stats().getMisses()); - stat.put(StatName.CACHE_EVICTION_COUNT.getName(), cache.stats().getEvictions()); - stat.put(StatName.CACHE_ENTRY_COUNT.getName(), cache.count()); - stat.put(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName(), cache.weight()); + stat.put(Stat.CACHE_HIT_COUNT.getName(), cache.stats().getHits()); + stat.put(Stat.CACHE_MISS_COUNT.getName(), cache.stats().getMisses()); + stat.put(Stat.CACHE_EVICTION_COUNT.getName(), cache.stats().getEvictions()); + stat.put(Stat.CACHE_ENTRY_COUNT.getName(), cache.count()); + stat.put(Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName(), cache.weight()); return Collections.unmodifiableMap(stat); } } diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java deleted file mode 100644 index ce9f0f11..00000000 --- a/src/main/java/com/o19s/es/ltr/stats/suppliers/CounterSupplier.java +++ /dev/null @@ -1,24 +0,0 @@ -package com.o19s.es.ltr.stats.suppliers; - -import java.util.concurrent.atomic.LongAdder; -import java.util.function.Supplier; - -/** - * A supplier which provides an increment-only counter. - */ -public class CounterSupplier implements Supplier { - private final LongAdder counter; - - public CounterSupplier() { - this.counter = new LongAdder(); - } - - @Override - public Long get() { - return counter.longValue(); - } - - public void increment() { - counter.increment(); - } -} diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java index 2df4938c..8a76292b 100644 --- a/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java +++ b/src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java @@ -45,7 +45,7 @@ private String getAggregateStoresStatus() { } private String combineStatuses(String s1, String s2) { - if (s2 == null || STATUS_RED.equals(s1) || STATUS_RED.equals(s2)) { + if (STATUS_RED.equals(s1) || STATUS_RED.equals(s2)) { return STATUS_RED; } else if (STATUS_YELLOW.equals(s1) || STATUS_YELLOW.equals(s2)) { return STATUS_YELLOW; @@ -60,6 +60,6 @@ public String getLtrStoreHealthStatus(String storeName) { clusterService.state().getRoutingTable().index(storeName) ); - return indexHealth.getStatus().name().toLowerCase(Locale.getDefault()); + return indexHealth.getStatus().name().toLowerCase(Locale.ROOT); } } diff --git a/src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java b/src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java index 706bed16..f7f1cd3d 100644 --- a/src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java +++ b/src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java @@ -1,7 +1,6 @@ package com.o19s.es.ltr.stats.suppliers; import com.o19s.es.ltr.feature.store.index.IndexFeatureStore; -import com.o19s.es.ltr.stats.StatName; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; @@ -26,7 +25,6 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.function.Supplier; import java.util.stream.Stream; @@ -43,6 +41,23 @@ public class StoreStatsSupplier implements Supplier> createStoreStatsResponse(MultiSearchReq try { MultiSearchResponse msr = requestBuilder.execute().get(); assert indices.size() == msr.getResponses().length; - Map> stats = new ConcurrentHashMap<>(indices.size()); + Map> stats = new HashMap<>(indices.size()); Iterator indicesItr = indices.iterator(); Iterator responseItr = msr.iterator(); @@ -102,17 +117,17 @@ private Map> createStoreStatsResponse(MultiSearchReq private Map initStoreStat(String index) { Map storeStat = new HashMap<>(); - storeStat.put(StatName.STORE_STATUS.getName(), getLtrStoreHealthStatus(index)); - storeStat.put(StatName.STORE_FEATURE_COUNT.getName(), 0); - storeStat.put(StatName.STORE_FEATURE_SET_COUNT.getName(), 0); - storeStat.put(StatName.STORE_MODEL_COUNT.getName(), 0); + storeStat.put(Stat.STORE_STATUS.getName(), getLtrStoreHealthStatus(index)); + storeStat.put(Stat.STORE_FEATURE_COUNT.getName(), 0L); + storeStat.put(Stat.STORE_FEATURE_SET_COUNT.getName(), 0L); + storeStat.put(Stat.STORE_MODEL_COUNT.getName(), 0L); return storeStat; } private void updateCount(Terms.Bucket bucket, Map storeStat) { storeStat.computeIfPresent( typeToStatName(bucket.getKeyAsString()), - (k, v) -> bucket.getDocCount()); + (k, v) -> bucket.getDocCount() + (long) v); } private String typeToStatName(String type) { @@ -125,7 +140,7 @@ public String getLtrStoreHealthStatus(String storeName) { clusterService.state().metadata().index(storeName), clusterService.state().getRoutingTable().index(storeName)); - return indexHealth.getStatus().name().toLowerCase(Locale.getDefault()); + return indexHealth.getStatus().name().toLowerCase(Locale.ROOT); } private SearchRequestBuilder countSearchRequest(String index) { diff --git a/src/test/java/com/o19s/es/ltr/action/LTRStatsActionIT.java b/src/test/java/com/o19s/es/ltr/action/LTRStatsActionIT.java index 207b2ed0..5c0c4965 100644 --- a/src/test/java/com/o19s/es/ltr/action/LTRStatsActionIT.java +++ b/src/test/java/com/o19s/es/ltr/action/LTRStatsActionIT.java @@ -1,26 +1,30 @@ package com.o19s.es.ltr.action; +import com.o19s.es.ltr.LtrTestUtils; import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsNodesResponse; import com.o19s.es.ltr.action.LTRStatsAction.LTRStatsRequestBuilder; +import com.o19s.es.ltr.feature.store.StoredFeature; import com.o19s.es.ltr.feature.store.StoredFeatureSet; +import com.o19s.es.ltr.feature.store.StoredLtrModel; import com.o19s.es.ltr.feature.store.index.IndexFeatureStore; import com.o19s.es.ltr.stats.StatName; +import com.o19s.es.ltr.stats.suppliers.CacheStatsOnNodeSupplier; +import com.o19s.es.ltr.stats.suppliers.StoreStatsSupplier; -import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; -import static com.o19s.es.ltr.LtrTestUtils.randomFeature; -import static com.o19s.es.ltr.LtrTestUtils.randomFeatureSet; -import static com.o19s.es.ltr.LtrTestUtils.randomLinearModel; +import static com.o19s.es.ltr.feature.store.index.IndexFeatureStore.DEFAULT_STORE; import static com.o19s.es.ltr.feature.store.index.IndexFeatureStore.indexName; public class LTRStatsActionIT extends BaseIntegrationTest { - private static final String DEFAULT_STORE_NAME = - IndexFeatureStore.storeName(IndexFeatureStore.DEFAULT_STORE); + private static final String DEFAULT_STORE_NAME = IndexFeatureStore.storeName(DEFAULT_STORE); @SuppressWarnings("unchecked") public void testStatsNoStore() throws Exception { @@ -47,9 +51,9 @@ public void testAllStatsDefaultEmptyStore() throws ExecutionException, Interrupt assertEquals(1, stores.size()); assertTrue(stores.containsKey(DEFAULT_STORE_NAME)); Map storeStat = (Map) stores.get(DEFAULT_STORE_NAME); - assertEquals(0, storeStat.get(StatName.STORE_FEATURE_COUNT.getName())); - assertEquals(0, storeStat.get(StatName.STORE_FEATURE_SET_COUNT.getName())); - assertEquals(0, storeStat.get(StatName.STORE_MODEL_COUNT.getName())); + assertEquals(0L, storeStat.get(StoreStatsSupplier.Stat.STORE_FEATURE_COUNT.getName())); + assertEquals(0L, storeStat.get(StoreStatsSupplier.Stat.STORE_FEATURE_SET_COUNT.getName())); + assertEquals(0L, storeStat.get(StoreStatsSupplier.Stat.STORE_MODEL_COUNT.getName())); Map nodeStats = response.getNodes().get(0).getStatsMap(); assertFalse(nodeStats.isEmpty()); @@ -57,17 +61,18 @@ public void testAllStatsDefaultEmptyStore() throws ExecutionException, Interrupt Map cacheStats = (Map) nodeStats.get(StatName.CACHE.getName()); assertEquals(3, cacheStats.size()); - assertTrue(cacheStats.containsKey(StatName.CACHE_FEATURE.getName())); - assertTrue(cacheStats.containsKey(StatName.CACHE_FEATURE_SET.getName())); - assertTrue(cacheStats.containsKey(StatName.CACHE_MODEL.getName())); + assertTrue(cacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_FEATURE.getName())); + assertTrue(cacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_FEATURE_SET.getName())); + assertTrue(cacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_MODEL.getName())); - Map featureCacheStats = (Map) cacheStats.get(StatName.CACHE_FEATURE.getName()); + Map featureCacheStats = + (Map) cacheStats.get(CacheStatsOnNodeSupplier.Stat.CACHE_FEATURE.getName()); assertEquals(5, featureCacheStats.size()); - assertTrue(featureCacheStats.containsKey(StatName.CACHE_HIT_COUNT.getName())); - assertTrue(featureCacheStats.containsKey(StatName.CACHE_MISS_COUNT.getName())); - assertTrue(featureCacheStats.containsKey(StatName.CACHE_EVICTION_COUNT.getName())); - assertTrue(featureCacheStats.containsKey(StatName.CACHE_ENTRY_COUNT.getName())); - assertTrue(featureCacheStats.containsKey(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName())); + assertTrue(featureCacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_HIT_COUNT.getName())); + assertTrue(featureCacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_MISS_COUNT.getName())); + assertTrue(featureCacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_EVICTION_COUNT.getName())); + assertTrue(featureCacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_ENTRY_COUNT.getName())); + assertTrue(featureCacheStats.containsKey(CacheStatsOnNodeSupplier.Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName())); } @@ -88,11 +93,14 @@ public void testMultipleFeatureStores() throws Exception { } @SuppressWarnings("unchecked") - public void testStoreStats() throws ExecutionException, InterruptedException, IOException { - StoredFeatureSet featureSet = randomFeatureSet("featureset1"); - addElement(featureSet); - addElement(randomFeature("feature1")); - addElement(randomLinearModel("model1", featureSet)); + public void testStoreStats() throws Exception { + String customStoreName = "test"; + String customStore = indexName("test"); + createStore(customStore); + + Map> infos = new HashMap<>(); + infos.put(DEFAULT_STORE_NAME, addElements(DEFAULT_STORE)); + infos.put(customStoreName, addElements(customStore)); LTRStatsNodesResponse response = executeRequest(); assertFalse(response.hasFailures()); @@ -100,10 +108,49 @@ public void testStoreStats() throws ExecutionException, InterruptedException, IO Map clusterStats = response.getClusterStats(); Map stores = (Map) clusterStats.get(StatName.STORES.getName()); - Map storeStat = (Map) stores.get(DEFAULT_STORE_NAME); - assertEquals(1L, storeStat.get(StatName.STORE_FEATURE_SET_COUNT.getName())); - assertEquals(1L, storeStat.get(StatName.STORE_FEATURE_COUNT.getName())); - assertEquals(1L, storeStat.get(StatName.STORE_MODEL_COUNT.getName())); + assertStoreStats((Map) stores.get(DEFAULT_STORE_NAME), infos.get(DEFAULT_STORE_NAME)); + assertStoreStats((Map) stores.get(customStoreName), infos.get(customStoreName)); + } + + private void assertStoreStats(Map storeStat, Map expected) { + assertEquals(expected.get(StoredFeatureSet.TYPE), + storeStat.get(StoreStatsSupplier.Stat.STORE_FEATURE_SET_COUNT.getName())); + + assertEquals(expected.get(StoredFeature.TYPE), + storeStat.get(StoreStatsSupplier.Stat.STORE_FEATURE_COUNT.getName())); + + assertEquals(expected.get(StoredLtrModel.TYPE), + storeStat.get(StoreStatsSupplier.Stat.STORE_MODEL_COUNT.getName())); + } + + private Map addElements(String store) throws Exception { + Map counts = new HashMap<>(); + int nFeats = randomInt(20) + 1; + int nSets = randomInt(20) + 1; + int nModels = randomInt(20) + 1; + counts.put(StoredFeature.TYPE, (long) nFeats); + counts.put(StoredFeatureSet.TYPE, (long) nSets); + counts.put(StoredLtrModel.TYPE, (long) nModels); + addElements(store, nFeats, nSets, nModels); + return counts; + } + + private void addElements(String store, int nFeatures, int nSets, int nModels) throws Exception { + for (int i = 0; i < nFeatures; i++) { + StoredFeature feat = LtrTestUtils.randomFeature("feature" + i); + addElement(feat, store); + } + + List sets = new ArrayList<>(nSets); + for (int i = 0; i < nSets; i++) { + StoredFeatureSet set = LtrTestUtils.randomFeatureSet("set" + i); + addElement(set, store); + sets.add(set); + } + + for (int i = 0; i < nModels; i++) { + addElement(LtrTestUtils.randomLinearModel("model" + i, sets.get(random().nextInt(sets.size()))), store); + } } private LTRStatsNodesResponse executeRequest() throws ExecutionException, InterruptedException { diff --git a/src/test/java/com/o19s/es/ltr/action/TransportLTRStatsActionTests.java b/src/test/java/com/o19s/es/ltr/action/TransportLTRStatsActionTests.java index 1b9f70d1..d4dcc7ed 100644 --- a/src/test/java/com/o19s/es/ltr/action/TransportLTRStatsActionTests.java +++ b/src/test/java/com/o19s/es/ltr/action/TransportLTRStatsActionTests.java @@ -7,7 +7,6 @@ import com.o19s.es.ltr.stats.LTRStat; import com.o19s.es.ltr.stats.LTRStats; import com.o19s.es.ltr.stats.StatName; -import com.o19s.es.ltr.stats.suppliers.CounterSupplier; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.test.ESIntegTestCase; @@ -25,20 +24,15 @@ public class TransportLTRStatsActionTests extends ESIntegTestCase { private TransportLTRStatsAction action; private LTRStats ltrStats; - private Map> statsMap; - private StatName clusterStatName; - private StatName nodeStatName; + private Map statsMap; @Before public void setup() throws Exception { super.setUp(); - clusterStatName = StatName.PLUGIN_STATUS; - nodeStatName = StatName.CACHE; - statsMap = new HashMap<>(); - statsMap.put(clusterStatName.getName(), new LTRStat<>(false, new CounterSupplier())); - statsMap.put(nodeStatName.getName(), new LTRStat<>(true, () -> "test")); + statsMap.put(StatName.PLUGIN_STATUS.getName(), new LTRStat(false, () -> "cluster_stat")); + statsMap.put(StatName.CACHE.getName(), new LTRStat(true, () -> "node_stat")); ltrStats = new LTRStats(statsMap); diff --git a/src/test/java/com/o19s/es/ltr/stats/LTRStatTests.java b/src/test/java/com/o19s/es/ltr/stats/LTRStatTests.java index 00daff52..fc3dcd33 100644 --- a/src/test/java/com/o19s/es/ltr/stats/LTRStatTests.java +++ b/src/test/java/com/o19s/es/ltr/stats/LTRStatTests.java @@ -1,31 +1,18 @@ package com.o19s.es.ltr.stats; -import com.o19s.es.ltr.stats.suppliers.CounterSupplier; import org.elasticsearch.test.ESTestCase; public class LTRStatTests extends ESTestCase { public void testIsClusterLevel() { - LTRStat stat1 = new LTRStat<>(true, () -> "test"); + LTRStat stat1 = new LTRStat(true, () -> "test"); assertTrue(stat1.isClusterLevel()); - LTRStat stat2 = new LTRStat<>(false, () -> "test"); + LTRStat stat2 = new LTRStat(false, () -> "test"); assertFalse(stat2.isClusterLevel()); } public void testGetValue() { - LTRStat stat1 = new LTRStat<>(false, new CounterSupplier()); - assertEquals(0L, stat1.getValue().longValue()); - - LTRStat stat2 = new LTRStat<>(false, () -> "test"); - assertEquals("test", stat2.getValue()); - } - - public void testIncrementCounterSupplier() { - LTRStat incrementStat = new LTRStat<>(false, new CounterSupplier()); - - for (long i = 0L; i < 100; i++) { - assertEquals(i, incrementStat.getValue().longValue()); - incrementStat.increment(); - } + LTRStat stat2 = new LTRStat(false, () -> "test"); + assertEquals("test", stat2.getStatValue()); } } diff --git a/src/test/java/com/o19s/es/ltr/stats/LTRStatsTests.java b/src/test/java/com/o19s/es/ltr/stats/LTRStatsTests.java index 02f8d4fc..3b7c0f73 100644 --- a/src/test/java/com/o19s/es/ltr/stats/LTRStatsTests.java +++ b/src/test/java/com/o19s/es/ltr/stats/LTRStatsTests.java @@ -10,51 +10,51 @@ public class LTRStatsTests extends ESTestCase { - private Map> statsMap; + private Map statsMap; private LTRStats ltrStats; @Before public void setup() { statsMap = new HashMap<>(); - statsMap.put(StatName.PLUGIN_STATUS.getName(), new LTRStat<>(true, () -> "test")); - statsMap.put(StatName.CACHE.getName(), new LTRStat<>(false, () -> "test")); + statsMap.put(StatName.PLUGIN_STATUS.getName(), new LTRStat(true, () -> "test")); + statsMap.put(StatName.CACHE.getName(), new LTRStat(false, () -> "test")); ltrStats = new LTRStats(statsMap); } public void testGetStats() { - Map> stats = ltrStats.getStats(); + Map stats = ltrStats.getStats(); assertEquals(stats.size(), statsMap.size()); - for (Map.Entry> stat : stats.entrySet()) { + for (Map.Entry stat : stats.entrySet()) { assertStatPresence(stat.getKey(), stat.getValue()); } } public void testGetStat() { - LTRStat stat = ltrStats.getStat(StatName.PLUGIN_STATUS.getName()); + LTRStat stat = ltrStats.getStat(StatName.PLUGIN_STATUS.getName()); assertStatPresence(StatName.PLUGIN_STATUS.getName(), stat); } - private void assertStatPresence(String statName, LTRStat stat) { + private void assertStatPresence(String statName, LTRStat stat) { assertTrue(ltrStats.getStats().containsKey(statName)); assertSame(ltrStats.getStats().get(statName), stat); } public void testGetNodeStats() { - Map> stats = ltrStats.getStats(); - Set> nodeStats = new HashSet<>(ltrStats.getNodeStats().values()); + Map stats = ltrStats.getStats(); + Set nodeStats = new HashSet<>(ltrStats.getNodeStats().values()); - for (LTRStat stat : stats.values()) { + for (LTRStat stat : stats.values()) { assertTrue((stat.isClusterLevel() && !nodeStats.contains(stat)) || (!stat.isClusterLevel() && nodeStats.contains(stat))); } } public void testGetClusterStats() { - Map> stats = ltrStats.getStats(); - Set> clusterStats = new HashSet<>(ltrStats.getClusterStats().values()); + Map stats = ltrStats.getStats(); + Set clusterStats = new HashSet<>(ltrStats.getClusterStats().values()); - for (LTRStat stat : stats.values()) { + for (LTRStat stat : stats.values()) { assertTrue((stat.isClusterLevel() && clusterStats.contains(stat)) || (!stat.isClusterLevel() && !clusterStats.contains(stat))); } diff --git a/src/test/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplierTests.java b/src/test/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplierTests.java index 03f4450c..1ad804be 100644 --- a/src/test/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplierTests.java +++ b/src/test/java/com/o19s/es/ltr/stats/suppliers/CacheStatsOnNodeSupplierTests.java @@ -7,7 +7,6 @@ import com.o19s.es.ltr.feature.store.StoredFeatureSet; import com.o19s.es.ltr.feature.store.index.CachedFeatureStore; import com.o19s.es.ltr.feature.store.index.Caches; -import com.o19s.es.ltr.stats.StatName; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; import org.hamcrest.collection.IsMapContaining; @@ -16,6 +15,8 @@ import java.io.IOException; import java.util.Map; +import static com.o19s.es.ltr.stats.suppliers.CacheStatsOnNodeSupplier.Stat; + public class CacheStatsOnNodeSupplierTests extends ESTestCase { private MemStore memStore; private Caches caches; @@ -31,9 +32,9 @@ public void setup() { public void testGetCacheStatsInitialState() { Map> stats = cacheStatsOnNodeSupplier.get(); - assertCacheStats(stats.get(StatName.CACHE_FEATURE.getName()), 0, 0, 0, 0); - assertCacheStats(stats.get(StatName.CACHE_FEATURE_SET.getName()), 0, 0, 0, 0); - assertCacheStats(stats.get(StatName.CACHE_MODEL.getName()), 0, 0, 0, 0); + assertCacheStats(stats.get(Stat.CACHE_FEATURE.getName()), 0, 0, 0, 0); + assertCacheStats(stats.get(Stat.CACHE_FEATURE_SET.getName()), 0, 0, 0, 0); + assertCacheStats(stats.get(Stat.CACHE_MODEL.getName()), 0, 0, 0, 0); } public void testGetCacheStats() throws IOException { @@ -67,26 +68,26 @@ public void testGetCacheStats() throws IOException { Map> stats = cacheStatsOnNodeSupplier.get(); - assertCacheStats(stats.get(StatName.CACHE_FEATURE.getName()), 1, 2, 0, 2); - assertCacheStats(stats.get(StatName.CACHE_FEATURE_SET.getName()), 0, 1, 0, 1); - assertCacheStats(stats.get(StatName.CACHE_MODEL.getName()), 2, 2, 1, 1); + assertCacheStats(stats.get(Stat.CACHE_FEATURE.getName()), 1, 2, 0, 2); + assertCacheStats(stats.get(Stat.CACHE_FEATURE_SET.getName()), 0, 1, 0, 1); + assertCacheStats(stats.get(Stat.CACHE_MODEL.getName()), 2, 2, 1, 1); assertMemoryUsage(stats); } private void assertCacheStats(Map stat, long hits, long misses, long evictions, int entries) { - assertThat(stat, IsMapContaining.hasEntry(StatName.CACHE_HIT_COUNT.getName(), hits)); - assertThat(stat, IsMapContaining.hasEntry(StatName.CACHE_MISS_COUNT.getName(), misses)); - assertThat(stat, IsMapContaining.hasEntry(StatName.CACHE_EVICTION_COUNT.getName(), evictions)); - assertThat(stat, IsMapContaining.hasEntry(StatName.CACHE_ENTRY_COUNT.getName(), entries)); + assertThat(stat, IsMapContaining.hasEntry(Stat.CACHE_HIT_COUNT.getName(), hits)); + assertThat(stat, IsMapContaining.hasEntry(Stat.CACHE_MISS_COUNT.getName(), misses)); + assertThat(stat, IsMapContaining.hasEntry(Stat.CACHE_EVICTION_COUNT.getName(), evictions)); + assertThat(stat, IsMapContaining.hasEntry(Stat.CACHE_ENTRY_COUNT.getName(), entries)); } private void assertMemoryUsage(Map> stats) { - assertTrue((long) stats.get(StatName.CACHE_FEATURE.getName()) - .get(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); - assertTrue((long) stats.get(StatName.CACHE_FEATURE_SET.getName()) - .get(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); - assertTrue((long) stats.get(StatName.CACHE_MODEL.getName()) - .get(StatName.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); + assertTrue((long) stats.get(Stat.CACHE_FEATURE.getName()) + .get(Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); + assertTrue((long) stats.get(Stat.CACHE_FEATURE_SET.getName()) + .get(Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); + assertTrue((long) stats.get(Stat.CACHE_MODEL.getName()) + .get(Stat.CACHE_MEMORY_USAGE_IN_BYTES.getName()) > 0); } } diff --git a/src/test/java/com/o19s/es/ltr/stats/suppliers/CounterSupplierTests.java b/src/test/java/com/o19s/es/ltr/stats/suppliers/CounterSupplierTests.java deleted file mode 100644 index a621eda3..00000000 --- a/src/test/java/com/o19s/es/ltr/stats/suppliers/CounterSupplierTests.java +++ /dev/null @@ -1,12 +0,0 @@ -package com.o19s.es.ltr.stats.suppliers; - -import org.elasticsearch.test.ESTestCase; - -public class CounterSupplierTests extends ESTestCase { - public void testGetAndIncrement() { - CounterSupplier counterSupplier = new CounterSupplier(); - assertEquals((Long) 0L, counterSupplier.get()); - counterSupplier.increment(); - assertEquals((Long) 1L, counterSupplier.get()); - } -} diff --git a/src/test/resources/rest-api-spec/api/ltr.get_stats.json b/src/test/resources/rest-api-spec/api/ltr.get_stats.json index 8b4ae3c0..5b342540 100644 --- a/src/test/resources/rest-api-spec/api/ltr.get_stats.json +++ b/src/test/resources/rest-api-spec/api/ltr.get_stats.json @@ -4,13 +4,13 @@ "url": { "paths": [ { - "path": "/_ltr/stats", + "path": "/_ltr/_stats", "methods": [ "GET" ] }, { - "path": "/_ltr/stats/nodes/{nodeId}", + "path": "/_ltr/_stats/nodes/{nodeId}", "parts": { "nodeId": { "required": false, @@ -23,7 +23,7 @@ ] }, { - "path": "/_ltr/stats/{stat}", + "path": "/_ltr/_stats/{stat}", "parts": { "stat": { "required": false, @@ -36,7 +36,7 @@ ] }, { - "path": "/_ltr/stats/{stat}/nodes/{nodeId}", + "path": "/_ltr/_stats/{stat}/nodes/{nodeId}", "parts": { "nodeId": { "required": false,