-
Notifications
You must be signed in to change notification settings - Fork 188
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
Reorder formatString expression check for JacksonEvent #3533
Reorder formatString expression check for JacksonEvent #3533
Conversation
Signed-off-by: Taylor Gray <tylgry@amazon.com>
e2c2bdd
to
b08e9d4
Compare
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.
Can you add a unit test to help prevent this from getting swapped again? Perhaps verify that if val
is not null
then there is no call to evaluate()
.
d9119c2
Signed-off-by: Taylor Gray <tylgry@amazon.com>
d9119c2
to
8053d9e
Compare
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.
Thanks for adding the unit test!
@@ -567,8 +602,9 @@ public void testBuild_withFormatStringWithExpressionEvaluator() { | |||
|
|||
final ExpressionEvaluator expressionEvaluator = mock(ExpressionEvaluator.class); | |||
|
|||
when(expressionEvaluator.isValidExpressionStatement("foo")).thenReturn(false); | |||
verify(expressionEvaluator, times(0)).isValidExpressionStatement(eventKey); |
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.
There is also a never()
you can use instead of times(0)
. It's a bit more readable, but should have the same result. Also, I'd probably put an anyString()
in there instead of eventKey
. When verifying that something is not called, I prefer to be as loose as possible.
Reorder formatString expression check for JacksonEvent Signed-off-by: Taylor Gray <tylgry@amazon.com> (cherry picked from commit 53a06bd)
Description
Any call to JacksonEvent
formatString
method with an expression evaluator would callisValidExpressionStatement
. A call toisValidExpressionStatement
that was false, such as the configurations for normal keys (such as${some_key}
) resulted in the ANTLR parser logging something likeevery time this occurs, which is very often when the format is a dynamic index that is evaluated for every Event. This causes cpu spikes and a thread freeze from timeouts for the logger writing to console.
This mitigation includes swapping the order of the evaluation of the format expressions (
${}
), so that it is first evaluated whether or not the key is in the Event, and only if it is not do we evaluate whether the statement is a valid expression. The behavior should not change with this swap, but it will very heavily reduce the amount of logging that is seen from the ANTLR parser for this error. Eventually, we should try to keep this log from showing up completely when evaluating if an expression is valid.Issues Resolved
Resolves #3357
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.