-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Circuit Breakers: Log if CircuitBreaker is tripping #8050
Conversation
LGTM |
@@ -79,8 +79,12 @@ public MemoryCircuitBreaker(ByteSizeValue limit, double overheadConstant, Memory | |||
*/ | |||
public void circuitBreak(String fieldName, long bytesNeeded) throws CircuitBreakingException { | |||
this.trippedCount.incrementAndGet(); | |||
throw new CircuitBreakingException("Data too large, data for field [" + fieldName + "] would be larger than limit of [" + | |||
memoryBytesLimit + "/" + new ByteSizeValue(memoryBytesLimit) + "]"); | |||
String message = "Data too large, data for field [" + fieldName + "] would be larger than limit of [" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string could be final
also
LGTM, I vote for INFO level :) |
updated the PR |
LGTM |
since this is a message that we already return to the user, and potentially can be repeated a lot, typically in ES we do those as debug messages. If we decide to change those logging levels, I prefer to do it across the board then to be inconsistent. Btw, what I saw is that most of the times where "info" logging was requested, actually what was missing is adding it to our stats API. |
memoryBytesLimit + "/" + new ByteSizeValue(memoryBytesLimit) + "]"); | ||
final String message = "Data too large, data for field [" + fieldName + "] would be larger than limit of [" + | ||
memoryBytesLimit + "/" + new ByteSizeValue(memoryBytesLimit) + "]"; | ||
if (logger.isInfoEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this if
here?
I moved back to debug and removed the guard - I don't want this to be controversial |
Today we only throw an exception which might not be logged at all. This commit adds debug logging if we are tripping a CB. Closes elastic#8050
Today we only throw an exception which might not be logged at all. This commit adds debug logging if we are tripping a CB. Closes #8050
Today we only throw an exception which might not be logged at all. This commit adds debug logging if we are tripping a CB. Closes #8050
Today we only throw an exception which might not be logged at all. This commit adds debug logging if we are tripping a CB. Closes elastic#8050
today we only throw an exception which might not be logged at all. this PR adds debug logging if we tripping a CB. It's debug for now but I might even vote for info?