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

Fix a bug where the default HeadersSanitizer does not work in logging decorators #5519

Merged
merged 3 commits into from
Apr 2, 2024
Merged

Fix a bug where the default HeadersSanitizer does not work in logging decorators #5519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bd8a4e
Fix a bug where the default `HeadersSanitizer` in logging decorators
ikhoon Mar 19, 2024
8423005
remove shouldBuildLogWriter
ikhoon Mar 19, 2024
cd0203c
remove cruft
ikhoon Mar 19, 2024
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
27 changes: 20 additions & 7 deletions core/src/main/java/com/linecorp/armeria/client/logging/LoggingClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ public Function<? super HttpClient, LoggingClient> newDecorator() {
// Override the return type of the chaining methods in the superclass.

@Override
protected LoggingClientBuilder defaultLogger(Logger logger) {
return (LoggingClientBuilder) super.defaultLogger(logger);
protected LoggingClientBuilder defaultLogger(Logger defaultLogger) {
return (LoggingClientBuilder) super.defaultLogger(defaultLogger);
}

@Override
Expand Down Expand Up @@ -95,23 +95,33 @@ public LoggingClientBuilder failureSamplingRate(float samplingRate) {
return (LoggingClientBuilder) super.failureSamplingRate(samplingRate);
}

@Override
public LoggingClientBuilder logWriter(LogWriter logWriter) {
return (LoggingClientBuilder) super.logWriter(logWriter);
}

// Override the return type of the chaining methods in the super-superclass.
// All methods below are deprecated.

@Deprecated
@Override
public LoggingClientBuilder logger(Logger logger) {
return (LoggingClientBuilder) super.logger(logger);
}

@Deprecated
@Override
public LoggingClientBuilder logger(String loggerName) {
return (LoggingClientBuilder) super.logger(loggerName);
}

@Deprecated
@Override
public LoggingClientBuilder requestLogLevel(LogLevel requestLogLevel) {
return (LoggingClientBuilder) super.requestLogLevel(requestLogLevel);
}

@Deprecated
@Override
public LoggingClientBuilder requestLogLevel(Class<? extends Throwable> clazz, LogLevel requestLogLevel) {
return (LoggingClientBuilder) super.requestLogLevel(clazz, requestLogLevel);
Expand All @@ -124,31 +134,37 @@ public LoggingClientBuilder requestLogLevelMapper(
return (LoggingClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
}

@Deprecated
@Override
public LoggingClientBuilder requestLogLevelMapper(RequestLogLevelMapper requestLogLevelMapper) {
return (LoggingClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
}

@Deprecated
@Override
public LoggingClientBuilder responseLogLevel(HttpStatus status, LogLevel logLevel) {
return (LoggingClientBuilder) super.responseLogLevel(status, logLevel);
}

@Deprecated
@Override
public LoggingClientBuilder responseLogLevel(HttpStatusClass statusClass, LogLevel logLevel) {
return (LoggingClientBuilder) super.responseLogLevel(statusClass, logLevel);
}

@Deprecated
@Override
public LoggingClientBuilder responseLogLevel(Class<? extends Throwable> clazz, LogLevel logLevel) {
return (LoggingClientBuilder) super.responseLogLevel(clazz, logLevel);
}

@Deprecated
@Override
public LoggingClientBuilder successfulResponseLogLevel(LogLevel successfulResponseLogLevel) {
return (LoggingClientBuilder) super.successfulResponseLogLevel(successfulResponseLogLevel);
}

@Deprecated
@Override
public LoggingClientBuilder failureResponseLogLevel(LogLevel failureResponseLogLevel) {
return (LoggingClientBuilder) super.failureResponseLogLevel(failureResponseLogLevel);
Expand All @@ -161,6 +177,7 @@ public LoggingClientBuilder responseLogLevelMapper(
return (LoggingClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
}

@Deprecated
@Override
public LoggingClientBuilder responseLogLevelMapper(ResponseLogLevelMapper responseLogLevelMapper) {
return (LoggingClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
Expand Down Expand Up @@ -238,13 +255,9 @@ public LoggingClientBuilder responseCauseSanitizer(
return (LoggingClientBuilder) super.responseCauseSanitizer(responseCauseSanitizer);
}

@Deprecated
@Override
public LoggingClientBuilder responseCauseFilter(Predicate<Throwable> responseCauseFilter) {
return (LoggingClientBuilder) super.responseCauseFilter(responseCauseFilter);
}

@Override
public LoggingClientBuilder logWriter(LogWriter logWriter) {
return (LoggingClientBuilder) super.logWriter(logWriter);
}
}
26 changes: 19 additions & 7 deletions core/src/main/java/com/linecorp/armeria/client/logging/LoggingRpcClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@
// Override the return type of the chaining methods in the superclass.

@Override
protected LoggingRpcClientBuilder defaultLogger(Logger logger) {
return (LoggingRpcClientBuilder) super.defaultLogger(logger);
protected LoggingRpcClientBuilder defaultLogger(Logger defaultLogger) {
return (LoggingRpcClientBuilder) super.defaultLogger(defaultLogger);
}

@Override
Expand Down Expand Up @@ -98,23 +98,32 @@
return (LoggingRpcClientBuilder) super.failureSamplingRate(samplingRate);
}

@Override
public LoggingRpcClientBuilder logWriter(LogWriter logWriter) {
return (LoggingRpcClientBuilder) super.logWriter(logWriter);

Check warning on line 103 in core/src/main/java/com/linecorp/armeria/client/logging/LoggingRpcClientBuilder.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/client/logging/LoggingRpcClientBuilder.java#L103 

Added line #L103 was not covered by tests
}

// Override the return type of the chaining methods in the super-superclass.

@Deprecated
@Override
public LoggingRpcClientBuilder logger(Logger logger) {
return (LoggingRpcClientBuilder) super.logger(logger);
}

@Deprecated
@Override
public LoggingRpcClientBuilder logger(String loggerName) {
return (LoggingRpcClientBuilder) super.logger(loggerName);
}

@Deprecated
@Override
public LoggingRpcClientBuilder requestLogLevel(LogLevel requestLogLevel) {
return (LoggingRpcClientBuilder) super.requestLogLevel(requestLogLevel);
}

@Deprecated
@Override
public LoggingRpcClientBuilder requestLogLevel(Class<? extends Throwable> clazz, LogLevel requestLogLevel) {
return (LoggingRpcClientBuilder) super.requestLogLevel(clazz, requestLogLevel);
Expand All @@ -127,31 +136,37 @@
return (LoggingRpcClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
}

@Deprecated
@Override
public LoggingRpcClientBuilder requestLogLevelMapper(RequestLogLevelMapper requestLogLevelMapper) {
return (LoggingRpcClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
}

@Deprecated
@Override
public LoggingRpcClientBuilder responseLogLevel(HttpStatus status, LogLevel logLevel) {
return (LoggingRpcClientBuilder) super.responseLogLevel(status, logLevel);
}

@Deprecated
@Override
public LoggingRpcClientBuilder responseLogLevel(HttpStatusClass statusClass, LogLevel logLevel) {
return (LoggingRpcClientBuilder) super.responseLogLevel(statusClass, logLevel);
}

@Deprecated
@Override
public LoggingRpcClientBuilder responseLogLevel(Class<? extends Throwable> clazz, LogLevel logLevel) {
return (LoggingRpcClientBuilder) super.responseLogLevel(clazz, logLevel);
}

@Deprecated
@Override
public LoggingRpcClientBuilder successfulResponseLogLevel(LogLevel successfulResponseLogLevel) {
return (LoggingRpcClientBuilder) super.successfulResponseLogLevel(successfulResponseLogLevel);
}

@Deprecated
@Override
public LoggingRpcClientBuilder failureResponseLogLevel(LogLevel failureResponseLogLevel) {
return (LoggingRpcClientBuilder) super.failureResponseLogLevel(failureResponseLogLevel);
Expand All @@ -164,6 +179,7 @@
return (LoggingRpcClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
}

@Deprecated
@Override
public LoggingRpcClientBuilder responseLogLevelMapper(ResponseLogLevelMapper responseLogLevelMapper) {
return (LoggingRpcClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
Expand Down Expand Up @@ -240,13 +256,9 @@
return (LoggingRpcClientBuilder) super.responseCauseSanitizer(responseCauseSanitizer);
}

@Deprecated
@Override
public LoggingRpcClientBuilder responseCauseFilter(Predicate<Throwable> responseCauseFilter) {
return (LoggingRpcClientBuilder) super.responseCauseFilter(responseCauseFilter);
}

@Override
public LoggingRpcClientBuilder logWriter(LogWriter logWriter) {
return (LoggingRpcClientBuilder) super.logWriter(logWriter);
}
}
9 changes: 9 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/logging/AbstractLogFormatterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ abstract class AbstractLogFormatterBuilder<T> {
* .requestHeadersSanitizer(headersSanitizer)
* ...
* }</pre>
*
* @see HeadersSanitizer
*/
public AbstractLogFormatterBuilder<T> requestHeadersSanitizer(
BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> requestHeadersSanitizer) {
Expand Down Expand Up @@ -101,6 +103,8 @@ final HeadersSanitizer<T> requestHeadersSanitizer() {
* .responseHeadersSanitizer(headersSanitizer)
* ...
* }</pre>
*
* @see HeadersSanitizer
*/
public AbstractLogFormatterBuilder<T> responseHeadersSanitizer(
BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> responseHeadersSanitizer) {
Expand Down Expand Up @@ -135,6 +139,8 @@ final HeadersSanitizer<T> responseHeadersSanitizer() {
* .requestTrailersSanitizer(headersSanitizer)
* ...
* }</pre>
*
* @see HeadersSanitizer
*/
public AbstractLogFormatterBuilder<T> requestTrailersSanitizer(
BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> requestTrailersSanitizer) {
Expand Down Expand Up @@ -169,6 +175,8 @@ final HeadersSanitizer<T> requestTrailersSanitizer() {
* .responseTrailersSanitizer(headersSanitizer)
* ...
* }</pre>
*
* @see HeadersSanitizer
*/
public AbstractLogFormatterBuilder<T> responseTrailersSanitizer(
BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> responseTrailersSanitizer) {
Expand Down Expand Up @@ -204,6 +212,7 @@ final HeadersSanitizer<T> responseTrailersSanitizer() {
* builder.responseTrailersSanitizer(headersSanitizer);
* }</pre>
*
* @see HeadersSanitizer
* @see #requestHeadersSanitizer(BiFunction)
* @see #requestTrailersSanitizer(BiFunction)
* @see #responseHeadersSanitizer(BiFunction)
Expand Down
15 changes: 9 additions & 6 deletions core/src/main/java/com/linecorp/armeria/common/logging/DefaultLogWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@
import static com.linecorp.armeria.common.logging.LogWriterBuilder.DEFAULT_RESPONSE_LOG_LEVEL_MAPPER;
import static java.util.Objects.requireNonNull;

import java.util.function.Predicate;
import java.util.function.BiPredicate;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.MoreObjects;

import com.linecorp.armeria.common.RequestContext;
import com.linecorp.armeria.common.util.SafeCloseable;

final class DefaultLogWriter implements LogWriter {
Expand All @@ -37,20 +38,21 @@ final class DefaultLogWriter implements LogWriter {
static final DefaultLogWriter DEFAULT =
new DefaultLogWriter(defaultLogger, DEFAULT_REQUEST_LOG_LEVEL_MAPPER,
DEFAULT_RESPONSE_LOG_LEVEL_MAPPER,
throwable -> false, LogFormatter.ofText());
(ctx, throwable) -> false, LogFormatter.ofText());

private static boolean warnedNullRequestLogLevelMapper;
private static boolean warnedNullResponseLogLevelMapper;

private final Logger logger;
private final RequestLogLevelMapper requestLogLevelMapper;
private final ResponseLogLevelMapper responseLogLevelMapper;
private final Predicate<? super Throwable> responseCauseFilter;
private final BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter;
private final LogFormatter logFormatter;

DefaultLogWriter(Logger logger, RequestLogLevelMapper requestLogLevelMapper,
ResponseLogLevelMapper responseLogLevelMapper,
Predicate<? super Throwable> responseCauseFilter, LogFormatter logFormatter) {
BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter,
LogFormatter logFormatter) {
this.logger = logger;
this.requestLogLevelMapper = requestLogLevelMapper;
this.responseLogLevelMapper = responseLogLevelMapper;
Expand Down Expand Up @@ -94,7 +96,8 @@ public void logResponse(RequestLog log) {

if (responseLogLevel.isEnabled(logger)) {
final String responseStr = logFormatter.formatResponse(log);
try (SafeCloseable ignored = log.context().push()) {
final RequestContext ctx = log.context();
try (SafeCloseable ignored = ctx.push()) {
if (responseCause == null) {
responseLogLevel.log(logger, responseStr);
return;
Expand All @@ -108,7 +111,7 @@ public void logResponse(RequestLog log) {
responseLogLevel.log(logger, logFormatter.formatRequest(log));
}

if (responseCauseFilter.test(responseCause)) {
if (responseCauseFilter.test(ctx, responseCause)) {
responseLogLevel.log(logger, responseStr);
} else {
responseLogLevel.log(logger, responseStr, responseCause);
Expand Down
Loading
Loading