Revert "Make CBE message creation more robust (#87687) (#87874)"

This reverts commit 4a6e16c.

Author: DaveCTurner

docs/changelog/87687.yaml

@@ -77,43 +77,33 @@ public double getOverhead() {
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject(name.toLowerCase(Locale.ROOT));
-        addBytesFieldsSafe(builder, limit, Fields.LIMIT, Fields.LIMIT_HUMAN);
-        addBytesFieldsSafe(builder, estimated, Fields.ESTIMATED, Fields.ESTIMATED_HUMAN);
+        builder.field(Fields.LIMIT, limit);
+        builder.field(Fields.LIMIT_HUMAN, new ByteSizeValue(limit));
+        builder.field(Fields.ESTIMATED, estimated);
+        builder.field(Fields.ESTIMATED_HUMAN, new ByteSizeValue(estimated));
         builder.field(Fields.OVERHEAD, overhead);
         builder.field(Fields.TRIPPED_COUNT, trippedCount);
         builder.endObject();
         return builder;
     }
 
-    private void addBytesFieldsSafe(XContentBuilder builder, long bytes, String rawFieldName, String humanFieldName) throws IOException {
-        builder.field(rawFieldName, bytes);
-        if (0 <= bytes) {
-            builder.field(humanFieldName, new ByteSizeValue(bytes));
-        } else {
-            // Something's definitely wrong, maybe a breaker was freed twice? Still, we're just writing out stats here, so we should keep
-            // going if we're running in production.
-            assert HierarchyCircuitBreakerService.permitNegativeValues : this;
-            // noinspection ResultOfMethodCallIgnored - we call toString() to log a warning
-            toString();
-            builder.field(humanFieldName, "");
-        }
-    }
-
     @Override
     public String toString() {
-        final var stringBuilder = new StringBuilder();
-        stringBuilder.append("[");
-        stringBuilder.append(this.name);
-        stringBuilder.append(",limit=");
-        HierarchyCircuitBreakerService.appendBytesSafe(stringBuilder, this.limit);
-        stringBuilder.append(",estimated=");
-        HierarchyCircuitBreakerService.appendBytesSafe(stringBuilder, this.estimated);
-        stringBuilder.append(",overhead=");
-        stringBuilder.append(this.overhead);
-        stringBuilder.append(",tripped=");
-        stringBuilder.append(this.trippedCount);
-        stringBuilder.append("]");
-        return stringBuilder.toString();
+        return "["
+            + this.name
+            + ",limit="
+            + this.limit
+            + "/"
+            + new ByteSizeValue(this.limit)
+            + ",estimated="
+            + this.estimated
+            + "/"
+            + new ByteSizeValue(this.estimated)
+            + ",overhead="
+            + this.overhead
+            + ",tripped="
+            + this.trippedCount
+            + "]";
     }
 
     static final class Fields {

server/src/main/java/org/elasticsearch/indices/breaker/CircuitBreakerStats.java

@@ -35,9 +35,9 @@
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.ReentrantLock;
-import java.util.function.BiConsumer;
 import java.util.function.Function;
 import java.util.function.LongSupplier;
+import java.util.stream.Collectors;
 
 import static org.elasticsearch.core.Strings.format;
 import static org.elasticsearch.indices.breaker.BreakerSettings.CIRCUIT_BREAKER_LIMIT_SETTING;
@@ -397,78 +397,47 @@ public void checkParentLimit(long newBytesReserved, String label) throws Circuit
         long parentLimit = this.parentSettings.getLimit();
         if (memoryUsed.totalUsage > parentLimit && overLimitStrategy.overLimit(memoryUsed).totalUsage > parentLimit) {
             this.parentTripCount.incrementAndGet();
-            final String messageString = buildParentTripMessage(
-                newBytesReserved,
-                label,
-                memoryUsed,
-                parentLimit,
-                this.trackRealMemoryUsage,
-                this.breakers
+            final StringBuilder message = new StringBuilder(
+                "[parent] Data too large, data for ["
+                    + label
+                    + "]"
+                    + " would be ["
+                    + memoryUsed.totalUsage
+                    + "/"
+                    + new ByteSizeValue(memoryUsed.totalUsage)
+                    + "]"
+                    + ", which is larger than the limit of ["
+                    + parentLimit
+                    + "/"
+                    + new ByteSizeValue(parentLimit)
+                    + "]"
             );
+            if (this.trackRealMemoryUsage) {
+                final long realUsage = memoryUsed.baseUsage;
+                message.append(", real usage: [");
+                message.append(realUsage);
+                message.append("/");
+                message.append(new ByteSizeValue(realUsage));
+                message.append("], new bytes reserved: [");
+                message.append(newBytesReserved);
+                message.append("/");
+                message.append(new ByteSizeValue(newBytesReserved));
+                message.append("]");
+            }
+            message.append(", usages [");
+            message.append(this.breakers.entrySet().stream().map(e -> {
+                final CircuitBreaker breaker = e.getValue();
+                final long breakerUsed = (long) (breaker.getUsed() * breaker.getOverhead());
+                return e.getKey() + "=" + breakerUsed + "/" + new ByteSizeValue(breakerUsed);
+            }).collect(Collectors.joining(", ")));
+            message.append("]");
             // derive durability of a tripped parent breaker depending on whether the majority of memory tracked by
             // child circuit breakers is categorized as transient or permanent.
             CircuitBreaker.Durability durability = memoryUsed.transientChildUsage >= memoryUsed.permanentChildUsage
                 ? CircuitBreaker.Durability.TRANSIENT
                 : CircuitBreaker.Durability.PERMANENT;
-            logger.debug(() -> format("%s", messageString));
-            throw new CircuitBreakingException(messageString, memoryUsed.totalUsage, parentLimit, durability);
-        }
-    }
-
-    // exposed for tests
-    static String buildParentTripMessage(
-        long newBytesReserved,
-        String label,
-        MemoryUsage memoryUsed,
-        long parentLimit,
-        boolean trackRealMemoryUsage,
-        Map<String, CircuitBreaker> breakers
-    ) {
-        final var message = new StringBuilder();
-        message.append("[parent] Data too large, data for [");
-        message.append(label);
-        message.append("] would be [");
-        appendBytesSafe(message, memoryUsed.totalUsage);
-        message.append("], which is larger than the limit of [");
-        appendBytesSafe(message, parentLimit);
-        message.append("]");
-        if (trackRealMemoryUsage) {
-            final long realUsage = memoryUsed.baseUsage;
-            message.append(", real usage: [");
-            appendBytesSafe(message, realUsage);
-            message.append("], new bytes reserved: [");
-            appendBytesSafe(message, newBytesReserved);
-            message.append("]");
-        }
-        message.append(", usages [");
-        breakers.forEach(new BiConsumer<>() {
-            private boolean first = true;
-
-            @Override
-            public void accept(String key, CircuitBreaker breaker) {
-                if (first) {
-                    first = false;
-                } else {
-                    message.append(", ");
-                }
-                message.append(key).append("=");
-                appendBytesSafe(message, (long) (breaker.getUsed() * breaker.getOverhead()));
-            }
-        });
-        message.append("]");
-        return message.toString();
-    }
-
-    static void appendBytesSafe(StringBuilder stringBuilder, long bytes) {
-        stringBuilder.append(bytes);
-        if (bytes >= 0) {
-            stringBuilder.append("/");
-            stringBuilder.append(new ByteSizeValue(bytes));
-        } else {
-            // Something's definitely wrong, maybe a breaker was freed twice? Still, we're just creating an exception message here, so we
-            // should keep going if we're running in production.
-            logger.error("negative value in circuit breaker: {}", stringBuilder);
-            assert permitNegativeValues : stringBuilder.toString();
+            logger.debug(() -> format("%s", message.toString()));
+            throw new CircuitBreakingException(message.toString(), memoryUsed.totalUsage, parentLimit, durability);
         }
     }
 
@@ -667,7 +636,4 @@ TimeValue getLockTimeout() {
             return lockTimeout;
         }
     }
-
-    // exposed for testing
-    static boolean permitNegativeValues = false;
 }

server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java

@@ -11,7 +11,6 @@
 import org.elasticsearch.common.breaker.ChildMemoryCircuitBreaker;
 import org.elasticsearch.common.breaker.CircuitBreaker;
 import org.elasticsearch.common.breaker.CircuitBreakingException;
-import org.elasticsearch.common.breaker.NoopCircuitBreaker;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
@@ -26,7 +25,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.CyclicBarrier;
@@ -48,7 +46,6 @@
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
-import static org.hamcrest.Matchers.oneOf;
 import static org.hamcrest.Matchers.sameInstance;
 
 public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
@@ -876,77 +873,4 @@ public void testApplySettingForUpdatingUseRealMemory() {
             );
         }
     }
-
-    public void testBuildParentTripMessage() {
-        class TestChildCircuitBreaker extends NoopCircuitBreaker {
-            private final long used;
-
-            TestChildCircuitBreaker(long used) {
-                super("child");
-                this.used = used;
-            }
-
-            @Override
-            public long getUsed() {
-                return used;
-            }
-
-            @Override
-            public double getOverhead() {
-                return 1.0;
-            }
-        }
-
-        assertThat(
-            HierarchyCircuitBreakerService.buildParentTripMessage(
-                1L,
-                "test",
-                new HierarchyCircuitBreakerService.MemoryUsage(2L, 3L, 4L, 5L),
-                6L,
-                false,
-                Map.of("child", new TestChildCircuitBreaker(7L), "otherChild", new TestChildCircuitBreaker(8L))
-            ),
-            oneOf(
-                "[parent] Data too large, data for [test] would be [3/3b], which is larger than the limit of [6/6b], "
-                    + "usages [child=7/7b, otherChild=8/8b]",
-                "[parent] Data too large, data for [test] would be [3/3b], which is larger than the limit of [6/6b], "
-                    + "usages [otherChild=8/8b, child=7/7b]"
-            )
-        );
-
-        assertThat(
-            HierarchyCircuitBreakerService.buildParentTripMessage(
-                1L,
-                "test",
-                new HierarchyCircuitBreakerService.MemoryUsage(2L, 3L, 4L, 5L),
-                6L,
-                true,
-                Map.of()
-            ),
-            equalTo(
-                "[parent] Data too large, data for [test] would be [3/3b], which is larger than the limit of [6/6b], "
-                    + "real usage: [2/2b], new bytes reserved: [1/1b], usages []"
-            )
-        );
-
-        try {
-            HierarchyCircuitBreakerService.permitNegativeValues = true;
-            assertThat(
-                HierarchyCircuitBreakerService.buildParentTripMessage(
-                    -1L,
-                    "test",
-                    new HierarchyCircuitBreakerService.MemoryUsage(-2L, -3L, -4L, -5L),
-                    -6L,
-                    true,
-                    Map.of("child1", new TestChildCircuitBreaker(-7L))
-                ),
-                equalTo(
-                    "[parent] Data too large, data for [test] would be [-3], which is larger than the limit of [-6], "
-                        + "real usage: [-2], new bytes reserved: [-1], usages [child1=-7]"
-                )
-            );
-        } finally {
-            HierarchyCircuitBreakerService.permitNegativeValues = false;
-        }
-    }
 }