add keyExcludes to MDC resolvers to prevent overwriting of structural… - #4186
add keyExcludes to MDC resolvers to prevent overwriting of structural…#4186ramanathan1504 wants to merge 4 commits into
Conversation
|
|
||
| @Override | ||
| public void accept(final String key, final Object value, final LoopContext loopContext) { | ||
| if (loopContext.keyExcludes != null && loopContext.keyExcludes.contains(key)) { |
There was a problem hiding this comment.
What about requiring loopContext.keyExcludes to be not null, so we don't need a null-check? The performance should be similar.
There was a problem hiding this comment.
are you describing using Collections.emptySet() to avoid the != null check? I can make that change.
There was a problem hiding this comment.
Yes, this should not impact performance and would be easier to understand by developers. You can even add a JSpecify @NonNull annotation on the field, so it is clear it can not be null.
ppkarwasz
left a comment
ppkarwasz
left a comment
There was a problem hiding this comment.
Hi @ramanathan1504,
Looks good to me, with two small comments:
github-project-automation
Bot
moved this to Changes requested
in Log4j pull request tracker
| @Measurement(iterations = 5, time = 1) | ||
| @Fork(1) | ||
| @State(Scope.Benchmark) | ||
| public class MdcKeyFilterBenchmark { |
There was a problem hiding this comment.
We probably should compare the performance of two ReadOnlyStringMapResolver instances, instead of comparing Pattern vs Map. This might give us an idea if the changes are worth it.
Map is clearly faster than Pattern, but the relative performance gain can not be determined, since it lacks the time needed to format the map as JSON.
There was a problem hiding this comment.
Benchmark Mode Cnt Score Error Units
MdcKeyFilterBenchmark.testKeyExcludesResolver thrpt 5 5.049 ± 0.175 ops/us
MdcKeyFilterBenchmark.testPatternResolver thrpt 5 1.846 ± 0.023 ops/us
2 participants
Fixes #4166
Add
keyExcludesto MDC resolvers to prevent user-controlled variables from overwriting structural log fields.