Enhance Parent circuit breaker error message

[dakrone]

This adds information about either the current real usage (if tracking "real"
memory usage) or the child breaker usages to the exception message when the
parent circuit breaker trips.

The messages now look like:

[parent] Data too large, data for [my_request] would be [211288064/201.5mb], which is larger than the limit of [209715200/200mb], usages [request=157286400/150mb, fielddata=54001664/51.5mb, in_flight_requests=0/0b, accounting=0/0b]

Or when tracking real memory usage:

[parent] Data too large, data for [request] would be [251/251b], which is larger than the limit of [200/200b], real usage: [181/181b], new bytes reserved: [70/70b]

[elasticmachine]

Pinging @elastic/es-core-infra

[dakrone]

I noticed a few of these recently and thought having the message show the sub-usages could be useful.

[dakrone]

@elasticmachine test this please

[danielmitterdorfer]

I think the new error message is definitely better than before. Thanks! I left a comment / question around the calculation of the value.

[danielmitterdorfer]

Calling currentMemoryUsage() again here is tricky because the value is likely different than the one that cause the breaker to trip. To get the correct value, we could calculate it as final long realUsage = totalUsed - newBytesReserved which would basically revert the calculation in #parentUsed(long) but that also doesn't feel entirely right. Alternatively, we could also return a more structured representation which returns the memory usage as a dedicated property. Wdyt?

[dakrone]

I thought about this, but I thought that giving the real memory usage at exception construction time was the least-worst option (as part as plain-text is concerned).

For a structured error, I do like the idea of making this exception more structured, it's just a matter of coming up with the structure.

We currently have:

• bytesWanted
• bytesLimit

How about adding

• currentUsage (we could always retrieve it even if "track real memory" is disabled, just for message purposes)
• breakerUsages - a map of the current breakers to their usages

The only downside for this is that we already have logic for serializing the CircuitBreakingException, and to make this structured we'd need a new exception (ParentCircuitBreakingException) since it doesn't make sense to add this additional structure to every circuit breaker trip (or maybe it does? what do you think?)

[danielmitterdorfer]

For a structured error, I do like the idea of making this exception more structured, it's just a matter of coming up with the structure.

My point was not to change CircuitBreakingException but rather to have #parentUsed(long) return a more structured representation, something along the lines of:

private static class ParentMemUsage {
    private final long baseUsage;
    private final long reservation;
    private final long totalUsage;
    
    public ParentMemUsage(long baseUsage, long reservation, long totalUsage) {
        // ...
    }
    // ... getters here ...
}

private long parentUsed(long newBytesReserved) {
    if (this.trackRealMemoryUsage) {
        long current = currentMemoryUsage();
        return new ParentMemUsage(current, newBytesReserved,  current + newBytesReserved);
    } else {
        long parentEstimated = 0;
        for (CircuitBreaker breaker : this.breakers.values()) {
            parentEstimated += breaker.getUsed() * breaker.getOverhead();
        }
        return new ParentMemUsage(parentEstimated, newBytesReserved, parentEstimated);
    }
}

We could then just call ParentMemUsage#getBaseUsage() in order to retrieve the original value again in case an exception occurs. A downside of this is that we allocate an additional object but it might be possible that the compiler is able to optimize it away (by inlining #parentUsed(long) and then eliminating the allocation via escape analysis).

[dakrone]

Okay, I gave this a shot, let me know what you think

[danielmitterdorfer]

Thanks for iterating! LGTM