Skip to content
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.

Sign up for GitHub

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

Add exclude pattern to access log #16891

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Add exclude pattern to access log #16891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...-http/deployment/src/test/java/io/quarkus/vertx/http/accesslog/AccessLogFileTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public JavaArchive get() {
p.setProperty("quarkus.http.access-log.base-file-name", "server");
p.setProperty("quarkus.http.access-log.log-directory", logDirectory.toAbsolutePath().toString());
p.setProperty("quarkus.http.access-log.pattern", "long");
p.setProperty("quarkus.http.access-log.exclude-pattern", "/health|/liveliness");
ByteArrayOutputStream out = new ByteArrayOutputStream();
p.store(out, null);

Expand Down Expand Up @@ -104,6 +105,8 @@ public void testSingleLogMessageToFile() throws IOException, InterruptedExceptio
new HttpClientConfig().setParam(CoreProtocolPNames.PROTOCOL_VERSION, new ProtocolVersion("HTTP", 1, 0)));
final RequestSpecification requestSpec = new RequestSpecBuilder().setConfig(http10Config).build();
final String paramValue = UUID.randomUUID().toString();
RestAssured.given(requestSpec).get("/health"); //should be ignored
RestAssured.given(requestSpec).get("/liveliness"); //should be ignored
RestAssured.given(requestSpec).get("/does-not-exist?foo=" + paramValue);

Awaitility.given().pollInterval(100, TimeUnit.MILLISECONDS)
Expand All @@ -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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a other test in case of ingestion of an invalid regex.

Assertions.assertTrue(data.contains("/does-not-exist"));
Assertions.assertTrue(data.contains("?foo=" + paramValue),
"access log is missing query params");
Expand Down
6 changes: 6 additions & 0 deletions ...sions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/AccessLogConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ public class AccessLogConfig {
@ConfigItem(defaultValue = "false")
public boolean enabled;

/**
* A regular expression that can be used to exclude some paths from logging.
*/
@ConfigItem
Optional<String> excludePattern;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created smallrye/smallrye-config#558 which should allow using Optional<Pattern> excludePattern here


/**
* The access log pattern.
*
Expand Down
3 changes: 2 additions & 1 deletion ...ons/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,8 @@ public void handle(HttpServerRequest event) {
} else {
receiver = new JBossLoggingAccessLogReceiver(accessLog.category);
}
AccessLogHandler handler = new AccessLogHandler(receiver, accessLog.pattern, getClass().getClassLoader());
AccessLogHandler handler = new AccessLogHandler(receiver, accessLog.pattern, getClass().getClassLoader(),
accessLog.excludePattern);
httpRouteRouter.route().order(Integer.MIN_VALUE).handler(handler);
quarkusWrapperNeeded = true;
}
Expand Down
20 changes: 19 additions & 1 deletion ...ntime/src/main/java/io/quarkus/vertx/http/runtime/filters/accesslog/AccessLogHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
package io.quarkus.vertx.http.runtime.filters.accesslog;

import java.util.Collections;
import java.util.Optional;
import java.util.StringJoiner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import io.quarkus.vertx.http.runtime.attribute.ExchangeAttribute;
import io.quarkus.vertx.http.runtime.attribute.ExchangeAttributeParser;
Expand Down Expand Up @@ -90,18 +93,26 @@ public class AccessLogHandler implements Handler<RoutingContext> {
private final AccessLogReceiver accessLogReceiver;
private final String formatString;
private final ExchangeAttribute tokens;
private final Pattern excludePattern;

public AccessLogHandler(final AccessLogReceiver accessLogReceiver, final String formatString, ClassLoader classLoader) {
public AccessLogHandler(final AccessLogReceiver accessLogReceiver, final String formatString, ClassLoader classLoader,
Optional<String> excludePattern) {
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if it is not a valid regex?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would like throw an exception (can't remember which one)

Copy link
Contributor

@netodevel netodevel Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

} else {
this.excludePattern = null;
}
}

public AccessLogHandler(final AccessLogReceiver accessLogReceiver, String formatString, final ExchangeAttribute attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String formatString Should it be final? Like other constructor above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes no practical difference.

this.accessLogReceiver = accessLogReceiver;
this.formatString = handleCommonNames(formatString);
this.tokens = attribute;
this.excludePattern = null;
}

private static String handleCommonNames(String formatString) {
Expand All @@ -122,6 +133,13 @@ private static String handleCommonNames(String formatString) {

@Override
public void handle(RoutingContext rc) {
if (excludePattern != null) {
Matcher m = excludePattern.matcher(rc.request().path());
if (m.matches()) {
rc.next();
return;
}
}
QuarkusRequestWrapper.get(rc.request()).addRequestDoneHandler(new Handler<Void>() {
@Override
public void handle(Void event) {
Expand Down