Add exclude pattern to access log #16891
Conversation
| this.accessLogReceiver = accessLogReceiver; | ||
| this.formatString = handleCommonNames(formatString); | ||
| this.tokens = new ExchangeAttributeParser(classLoader, Collections.singletonList(new SubstituteEmptyWrapper("-"))) | ||
| .parse(this.formatString); | ||
| if (excludePattern.isPresent()) { | ||
| this.excludePattern = Pattern.compile(excludePattern.get()); |
There was a problem hiding this comment.
what happens if it is not a valid regex?
There was a problem hiding this comment.
That would like throw an exception (can't remember which one)
There was a problem hiding this comment.
anyway i think there must be a test scenario for the exception that takes place
another question that came to me,
should this invalid data not let the application start?
There was a problem hiding this comment.
Pattern.compile is part of the JDK, I am not going at add additional tests that verify the JDK is working as expected (unless we have reason to suspect there is a bug in the JDK, which is just not the case here).
If this is invalid the application will fail to start.
There was a problem hiding this comment.
@stuartwdouglas,
I understand that it is a JDK feature .. but what I suggested was not to test Pattern.compile was the integration of your code with it.
Analogy:
You do not test how postgresql works, but you do test the integration of your app with it.
and still has the counterpoint of how a dev in the future knows that an exception in this code snippet should stop the app from starting?
how to guarantee that the change he will make will not impact?
| @@ -117,6 +120,8 @@ public void run() throws Throwable { | |||
| Path path = logDirectory.resolve("server.log"); | |||
| Assertions.assertTrue(Files.exists(path)); | |||
| String data = new String(Files.readAllBytes(path), StandardCharsets.UTF_8); | |||
| Assertions.assertFalse(data.contains("/health")); | |||
| Assertions.assertFalse(data.contains("/liveliness")); | |||
There was a problem hiding this comment.
I think there should be a other test in case of ingestion of an invalid regex.
| this.excludePattern = Pattern.compile(excludePattern.get()); | ||
| } else { | ||
| this.excludePattern = null; | ||
| } | ||
| } | ||
|
|
||
| public AccessLogHandler(final AccessLogReceiver accessLogReceiver, String formatString, final ExchangeAttribute attribute) { |
There was a problem hiding this comment.
String formatString Should it be final? Like other constructor above.
There was a problem hiding this comment.
It makes no practical difference.
| * A regular expression that can be used to exclude some paths from logging. | ||
| */ | ||
| @ConfigItem | ||
| Optional<String> excludePattern; |
There was a problem hiding this comment.
I created smallrye/smallrye-config#558 which should allow using Optional<Pattern> excludePattern here
|
Any chance there'd be an example for path such as |
Fixes #13377