Use faster and more approximate snippetting

3 files changed, 41 insertions, 128 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java
index cbc8ffbf1b7..bc88702b0fc 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java
@@ -2,6 +2,8 @@
 
 package com.yahoo.vespa.hosted.node.admin.task.util.process;
 
+import com.yahoo.vespa.hosted.node.admin.task.util.text.SnippetGenerator;
+
 /**
  * Base class for child process related exceptions, with a util to build an error message
  * that includes a large part of the output.
@@ -10,10 +12,7 @@ package com.yahoo.vespa.hosted.node.admin.task.util.process;
  */
 @SuppressWarnings("serial")
 public abstract class ChildProcessException extends RuntimeException {
-    private static final int MAX_OUTPUT_PREFIX = 200;
-    private static final int MAX_OUTPUT_SUFFIX = 200;
-    // Omitting a number of chars less than 10 or less than 10% would be ridiculous.
-    private static final int MAX_OUTPUT_SLACK = Math.max(10, (10 * (MAX_OUTPUT_PREFIX + MAX_OUTPUT_SUFFIX)) / 100);
+    private static final SnippetGenerator snippetGenerator = new SnippetGenerator();
 
     /**
      * An exception with a message of the following format:
@@ -36,44 +35,13 @@ public abstract class ChildProcessException extends RuntimeException {
         super(makeSnippet(problem, commandLine, possiblyHugeOutput), cause);
     }
 
-    private static String makeSnippet(String problem,
-                               String commandLine,
-                               String possiblyHugeOutput) {
-        return makeSnippet(
-                problem,
-                commandLine,
-                possiblyHugeOutput,
-                MAX_OUTPUT_PREFIX,
-                MAX_OUTPUT_SUFFIX,
-                MAX_OUTPUT_SLACK);
-    }
-
-    // Package-private instead of private for testing.
-    static String makeSnippet(String problem,
-                              String commandLine,
-                              String possiblyHugeOutput,
-                              int maxOutputPrefix,
-                              int maxOutputSuffix,
-                              int maxOutputSlack) {
-        StringBuilder stringBuilder = new StringBuilder()
-                .append("Command '")
-                .append(commandLine)
-                .append("' ")
-                .append(problem)
-                .append(": stdout/stderr: '");
-
-        if (possiblyHugeOutput.length() <= maxOutputPrefix + maxOutputSuffix + maxOutputSlack) {
-            stringBuilder.append(possiblyHugeOutput);
-        } else {
-            stringBuilder.append(possiblyHugeOutput, 0, maxOutputPrefix)
-                    .append("... [")
-                    .append(possiblyHugeOutput.length() - maxOutputPrefix - maxOutputSuffix)
-                    .append(" chars omitted] ...")
-                    .append(possiblyHugeOutput.substring(possiblyHugeOutput.length() - maxOutputSuffix));
-        }
-
-        stringBuilder.append("'");
-
-        return stringBuilder.toString();
+    private static String makeSnippet(String problem, String commandLine, String possiblyHugeOutput) {
+        return "Command '" +
+                commandLine +
+                "' " +
+                problem +
+                ": stdout/stderr: '" +
+                snippetGenerator.makeSnippet(possiblyHugeOutput, 500) +
+                "'";
     }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGenerator.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGenerator.java
index 1cb75434486..9a1876eb523 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGenerator.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGenerator.java
@@ -2,73 +2,29 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.text;
 
 /**
- * @author hakonhall
+ * @author hakon
  */
 public class SnippetGenerator {
 
-    public static final String OMIT_PREFIX = "[...";
-    public static final String OMIT_SUFFIX = " chars omitted]";
-    /**
-     * If {@code maxLength in }{@link #makeSnippet(String, int maxLength)} is less than this limit,
-     * a snippet would actually be larger than maxLength.
-     */
-    public static final int LEAST_MAXLENGTH = OMIT_PREFIX.length() + 2 + OMIT_SUFFIX.length();
+    private static final int ASSUMED_OMIT_TEXT_LENGTH = omitText(1234).length();
 
-    public String makeSnippet(String possiblyHugeString, int maxLength) {
-        if (possiblyHugeString.length() <= maxLength) {
-            return possiblyHugeString;
-        }
+    private static String omitText(int omittedLength) { return "[..." + omittedLength + " chars omitted]"; }
 
-        // We will preserve a prefix and suffix, but replace the middle part of possiblyHugeString
-        // with a cut text:
-        //   possiblyHugeString = prefix + omitted + suffix
-        //   result = prefix + cut + suffix
-        //   cut = OMIT_PREFIX + size + OMIT_SUFFIX
-        //   size = format("%d", omitted.length())
-        //   digits = size.length()
-        int assumedDigits = 2; // Just an initial guess: size.length() between 10 and 99
-        int prefixLength, suffixLength;
-        String size;
-
-        while (true) {
-            int assumedCutLength = OMIT_PREFIX.length() + assumedDigits + OMIT_SUFFIX.length();
-            // Make prefixLength ~ suffixLength, with prefixLength >= suffixLength
-            suffixLength = Math.max((maxLength - assumedCutLength) / 2, 0);
-            prefixLength = Math.max(maxLength - assumedCutLength - suffixLength, 0);
-            // RHS is guaranteed to be >= 0
-            int omittedLength = possiblyHugeString.length() - prefixLength - suffixLength;
-            size = String.format("%d", omittedLength);
-
-            // If assumedCutLength happens to be wrong, we retry with an adjusted setting.
-            int actualDigits = size.length();
-            if (actualDigits == assumedDigits) {
-                break;
-            }
-
-            // Is this loop guaranteed to finish? Yes, because from one iteration to the next,
-            // omittedLength can change by at most 9 (size having 1 digit to size with 10 digits
-            // or vice versa):
-            //  - If actualDigits < assumedDigits, omittedLength will decrease on next iteration
-            //    by 1-9, and so size can at most decrease by another 1 for that iteration. And,
-            //    if it did decrease by 1, it cannot decrease again (and must therefore break
-            //    the loop) in the iteration after, since a drop of (at most) 18 cannot remove
-            //    2 digits from a number.
-            //  - If actualDigits > assumedDigits, a similar argument holds.
-            assumedDigits = actualDigits;
-        }
+    /** Returns a snippet of approximate size. */
+    public String makeSnippet(String text, int sizeHint) {
+        if (text.length() < sizeHint) return text;
 
+        int maxSuffixLength = Math.max(0, (sizeHint - ASSUMED_OMIT_TEXT_LENGTH) / 2);
+        int maxPrefixLength = Math.max(0, sizeHint - ASSUMED_OMIT_TEXT_LENGTH - maxSuffixLength);
         String snippet =
-                possiblyHugeString.substring(0, prefixLength) +
-                OMIT_PREFIX +
-                size +
-                OMIT_SUFFIX +
-                possiblyHugeString.substring(possiblyHugeString.length() - suffixLength);
+                text.substring(0, maxPrefixLength) +
+                omitText(text.length() - maxPrefixLength - maxSuffixLength) +
+                text.substring(text.length() - maxSuffixLength);
+
+        // It would be silly to return a snippet when the full text is barely longer.
+        // Note: Say ASSUMED_OMIT_TEXT_LENGTH=23: text will be returned whenever sizeHint<23 and text.length()<28.
+        if (text.length() <= 1.05 * snippet.length() + 5) return text;
 
-        if (snippet.length() > maxLength) {
-            // This can happen if maxLength is too small.
-            return possiblyHugeString.substring(0, maxLength);
-        } else {
-            return snippet;
-        }
+        return snippet;
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGeneratorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGeneratorTest.java
index 59ae4633dce..8a67052a286 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGeneratorTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/text/SnippetGeneratorTest.java
@@ -6,61 +6,50 @@ import org.junit.Test;
 import static org.junit.Assert.assertEquals;
 
 /**
- * @author hakonhall
+ * @author hakon
  */
 public class SnippetGeneratorTest {
     private final SnippetGenerator generator = new SnippetGenerator();
 
-    private void assertSnippet(String text, int maxLength, String expectedSnippet) {
-        assertEquals(expectedSnippet, generator.makeSnippet(text, maxLength));
+    private void assertSnippet(String text, int sizeHint, String expectedSnippet) {
+        assertEquals(expectedSnippet, generator.makeSnippet(text, sizeHint));
     }
 
     @Test
-    public void prefixSnippetForReallySmallMaxLength() {
+    public void prefixSnippetForReallySmallSizeHint() {
         assertSnippet(
                 "This is a long text that should be snippeted", 0,
-                "");
+                "[...44 chars omitted]");
 
         assertSnippet(
                 "This is a long text that should be snippeted", 1,
-                "T");
-
-        assertSnippet(
-                "This is a long text that should be snippeted", 10,
-                "This is a ");
-
-        assertSnippet(
-                "This is a long text that should be snippeted", 20,
-                "This is a long text ");
+                "[...44 chars omitted]");
     }
 
     @Test
     public void snippet() {
         assertSnippet(
-                "This is a long text that should be snippeted", 21,
+                "This is a long text that should be snippeted", 23,
                 "[...44 chars omitted]");
 
         assertSnippet(
-                "This is a long text that should be snippeted", 22,
+                "This is a long text that should be snippeted", 24,
                 "T[...43 chars omitted]");
 
         assertSnippet(
                 "This is a long text that should be snippeted", 30,
-                "This [...35 chars omitted]eted");
+                "This[...37 chars omitted]ted");
 
-        assertSnippet(
-                "This is a long text that should be snippeted", 31,
-                "This [...34 chars omitted]peted");
-
-        assertSnippet(
-                "This is a long text that should be snippeted", 43,
-                "This is a l[...22 chars omitted]e snippeted");
     }
 
     @Test
     public void noShorteningNeeded() {
         assertSnippet(
-                "This is a long text that should be snippeted", 44,
+                "This is a long text that should be snippeted", 39,
+                "This is [...28 chars omitted]nippeted");
+
+        assertSnippet(
+                "This is a long text that should be snippeted", 40,
                 "This is a long text that should be snippeted");
 
         assertSnippet(