-
Notifications
You must be signed in to change notification settings - Fork 199
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
Reduce noisy truncation logging #1968
Conversation
maxLength, | ||
trimTo80(value)); | ||
} | ||
logger.debug( |
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.
should this in the else block? without it, it will get log twice, warn and debug for the very first time.
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.
the debug message has more detail (non-trimmed value)
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.
in that case, do we need to trim in warn logger level?
since it's only logging once, i think it's fine to show details.
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.
I think logging an (expected) sql text > 8192 characters (like in #1964) could still be annoying enough for users to ask to suppress it (trying to make this warning not be so bad for the expected cases)
maxLength, | ||
trimTo80(value)); | ||
} | ||
logger.debug("truncated {} property value to {} characters: {}", propertyKey, maxLength, value); |
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.
same here.
if (!alreadyLoggedAttributeNames.add(attributeName)) { | ||
// this can be expected, so don't want to flood the logs with a lot of these | ||
// (and don't want to log the full value, e.g. sql text > 8192 characters) | ||
logger.warn( |
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.
should we continue to use the AggregatingLogger? it will log for the first 5 mins..
i think aggregation is still helpful for other customers.. when truncation happens occasionally.
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.
i'm not sure i follow "it will log for the first 5 mins"?
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.
it's using a scheduler to run every 5 mins. it's not possible to log once using aggregated logger unless to stop the scheduler, but we don't have any condition to stop it.. i see why switch from aggregated logger to regular logger.
warningLogger.recordWarning("truncated property[" + key + "]: " + value); | ||
if (alreadyLoggedPropertyKeys.size() < 10 && !alreadyLoggedPropertyKeys.add(propertyKey)) { | ||
// this can be expected, so don't want to flood the logs with a lot of these | ||
logger.warn( |
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 is almost identical to line 43. "property" and "attribute" are the only difference.
should we reword it a bit to make it reusable in both cases.
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.
ya, i tried a few variations, but ended up opting for clearer messages/code, with slight duplication
No description provided.