Implement a configurable ES result size (#685)

.gitignore

@@ -15,6 +15,7 @@ bin
 *.iml
 *.p12
 .idea
+.idea-run-configurations/
 /reports/
 /assets/out/
 .meghanada
@@ -30,3 +31,12 @@ build/
 out/
 gradle-app.setting
 .gradle
+
+# VS Code auto-created files
+.classpath
+.project
+.settings
+.java-version
+
+# direnv config file
+/.envrc

checkstyle.xml

@@ -23,6 +23,9 @@
     </module>
 
     <module name="SuppressionFilter">
+        <!-- note that $basedir must be set to TLD of heroic in your local dev
+        environment for checkstyle to function. You may wish to emply the direnv
+        utility to set this for you. -->
         <property name="file" value="${basedir}/suppressions.xml"/>
     </module>
 
@@ -69,7 +72,7 @@
     </module>
 
     <module name="Header">
-        <property name="headerFile" value="${basedir}/tools/java.header" />
+        <property name="headerFile" value="${basedir}/tools/java.header"/>
         <property name="ignoreLines" value="2" />
         <property name="fileExtensions" value="java" />
     </module>

heroic-component/src/main/java/com/spotify/heroic/common/Series.java

@@ -37,6 +37,7 @@
 import eu.toolchain.serializer.AutoSerialize;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
@@ -250,24 +251,20 @@ public static Series of(
         String key, Iterator<Map.Entry<String, String>> tagPairs,
         Iterator<Map.Entry<String, String>> resourcePairs
     ) {
-        final TreeMap<String, String> tags = new TreeMap<>();
-        final TreeMap<String, String> resource = new TreeMap<>();
-
-        while (tagPairs.hasNext()) {
-            final Map.Entry<String, String> pair = tagPairs.next();
-            final String tk = checkNotNull(pair.getKey());
-            final String tv = pair.getValue();
-            tags.put(tk, tv);
-        }
+        return new Series(key, mapEntriesToSortedMap(tagPairs),
+            mapEntriesToSortedMap(resourcePairs));
+    }
+
+    private static TreeMap<String, String> mapEntriesToSortedMap(
+        Iterator<Entry<String, String>> mapEntries) {
+        final TreeMap<String, String> treeMap = new TreeMap<>();
 
-        while (resourcePairs.hasNext()) {
-            final Map.Entry<String, String> pair = resourcePairs.next();
-            final String tk = checkNotNull(pair.getKey());
-            final String tv = pair.getValue();
-            resource.put(tk, tv);
+        while (mapEntries.hasNext()) {
+            var pair = mapEntries.next();
+            treeMap.put(checkNotNull(pair.getKey()), pair.getValue());
         }
 
-        return new Series(key, tags, resource);
+        return treeMap;
     }
 
     public static Series of(

heroic-component/src/main/java/com/spotify/heroic/suggest/NumSuggestionsLimit.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2015 Spotify AB.
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.spotify.heroic.suggest;
+
+import com.spotify.heroic.common.OptionalLimit;
+import java.util.Optional;
+
+/**
+ * A simple class to centralize logic around limiting the number of suggestions requested from ES by
+ * Heroic. It defaults to 50 but any request to the backend (e.g. MemoryBackend) can override that
+ * number.
+ */
+public class NumSuggestionsLimit {
+
+    /** No request is allowed to request more than this many tags, keys or tag values. */
+    public static final int LIMIT_CEILING = 250;
+
+    /**
+     * How many suggestions we should request from ES, unless the suggest API request specifies
+     * otherwise.
+     *
+     * <p>This applies to the requests made for keys, tag and tag values. This defaults to 50,
+     * otherwise * 10,000 is used as the default which is wasteful and could lag the grafana UI.
+     */
+    public static final int DEFAULT_LIMIT = 50;
+
+    private final int limit;
+
+    private NumSuggestionsLimit() {
+        limit = DEFAULT_LIMIT;
+    }
+
+    private NumSuggestionsLimit(int limit) {
+        int okLimit = Math.min(LIMIT_CEILING, limit);
+        this.limit = okLimit;
+    }
+
+    public static NumSuggestionsLimit of(Optional<Integer> limit) {
+        return limit.isEmpty() ? new NumSuggestionsLimit() : new NumSuggestionsLimit(limit.get());
+    }
+
+    public static NumSuggestionsLimit of() {
+        return new NumSuggestionsLimit();
+    }
+
+    public static NumSuggestionsLimit of(int limit) {
+        return new NumSuggestionsLimit(limit);
+    }
+
+    public int getLimit() {
+        return limit;
+    }
+
+    /**
+     * use this.limit unless limit is not empty, then return the numeric result.
+     *
+     * @param limit the limit to respect if non-empty, usually from a request object
+     * @return a new NSL object - for fluent coding support
+     */
+    public NumSuggestionsLimit create(OptionalLimit limit) {
+        int num = limit.orElse(OptionalLimit.of(this.limit)).asInteger().get();
+        return new NumSuggestionsLimit(num);
+    }
+
+    /**
+     * use this.limit unless limit is not empty, then return the numeric result.
+     *
+     * @param limit the limit to respect if non-empty, usually from a request object
+     * @return the resulting, updated numeric limit
+     */
+    public int calculateNewLimit(OptionalLimit limit) {
+        return create(limit).getLimit();
+    }
+
+    public OptionalLimit asOptionalLimit() {
+        return OptionalLimit.of(limit);
+    }
+
+    public Optional<Integer> asOptionalInt() {
+        return Optional.of(getLimit());
+    }
+}

heroic-component/src/main/java/com/spotify/heroic/suggest/SuggestBackend.java

@@ -56,6 +56,7 @@ public interface SuggestBackend extends Grouped, Initializing, Collected {
     AsyncFuture<TagValueSuggest> tagValueSuggest(TagValueSuggest.Request request);
 
     AsyncFuture<WriteSuggest> write(WriteSuggest.Request request);
+
     default AsyncFuture<WriteSuggest> write(WriteSuggest.Request request, Span parentSpan) {
         // Ignore the parent span if the module does not specifically implement it.
         return write(request);

heroic-component/src/test/java/com/spotify/heroic/suggest/NumSuggestionsLimitTest.java

@@ -0,0 +1,40 @@
+package com.spotify.heroic.suggest;
+
+import static org.junit.Assert.assertEquals;
+
+import com.spotify.heroic.common.OptionalLimit;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class NumSuggestionsLimitTest {
+  public static final int LIMIT_FORTY = 40;
+  private NumSuggestionsLimit limit;
+
+  @Before
+  public void setup() {
+    this.limit = NumSuggestionsLimit.of(LIMIT_FORTY);
+  }
+
+  @Test
+  public void TestCorrectLimitIsApplied() {
+
+    // Check that a supplied limit is selected
+    int result = this.limit.calculateNewLimit(OptionalLimit.of(5));
+    assertEquals(5, result);
+
+    // Check that none of the above have affected the NSL's stored number
+    result = this.limit.calculateNewLimit(OptionalLimit.empty());
+    assertEquals(LIMIT_FORTY, result);
+
+    // Check that a giant request limit is not respected
+    result = this.limit.calculateNewLimit(OptionalLimit.of(10_000));
+    assertEquals(NumSuggestionsLimit.LIMIT_CEILING, result);
+
+    // Check that none of the above have affected the NSL's stored number. The
+    // above operations return a new object each time.
+    assertEquals(LIMIT_FORTY, this.limit.getLimit());
+  }
+}