From cc52a453874b5722ee4ee2bf0887c638fe143dc0 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Mon, 6 Dec 2021 13:58:10 -0800 Subject: [PATCH 1/4] Log channel ID when logging error --- .../netty/internal/NettyRequestExecutor.java | 20 +-- .../internal/utils/ChannelLogHelper.java | 63 +++++++++ .../internal/utils/ChannelLogHelperTest.java | 125 ++++++++++++++++++ 3 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java create mode 100644 http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java index 2e03389ff153..b8cf2a8db7ee 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java @@ -67,6 +67,7 @@ import software.amazon.awssdk.http.nio.netty.internal.http2.HttpToHttp2OutboundAdapter; import software.amazon.awssdk.http.nio.netty.internal.nrs.HttpStreamsClientHandler; import software.amazon.awssdk.http.nio.netty.internal.nrs.StreamedHttpRequest; +import software.amazon.awssdk.http.nio.netty.internal.utils.ChannelLogHelper; import software.amazon.awssdk.http.nio.netty.internal.utils.ChannelUtils; import software.amazon.awssdk.http.nio.netty.internal.utils.NettyUtils; import software.amazon.awssdk.metrics.MetricCollector; @@ -129,7 +130,10 @@ private CompletableFuture createExecutionFuture(Promise channelPr } }); } catch (Throwable exc) { - log.warn("Unable to add a task to cancel the request to channel's EventLoop", exc); + ChannelLogHelper.warn(log, + ch, + () -> "Unable to add a task to cancel the request to channel's EventLoop", + exc); } } }); @@ -172,7 +176,7 @@ private void makeRequestListener(Future channelFuture) { } }); } else { - handleFailure(() -> "Failed to create connection to " + endpoint(), channelFuture.cause()); + handleFailure(channel, () -> "Failed to create connection to " + endpoint(), channelFuture.cause()); } } @@ -203,7 +207,7 @@ private boolean tryConfigurePipeline() { default: String errorMsg = "Unknown protocol: " + protocol; closeAndRelease(channel); - handleFailure(() -> errorMsg, new RuntimeException(errorMsg)); + handleFailure(channel, () -> errorMsg, new RuntimeException(errorMsg)); return false; } @@ -220,7 +224,7 @@ private boolean tryConfigurePipeline() { if (!channel.isActive()) { String errorMessage = "Channel was closed before it could be written to."; closeAndRelease(channel); - handleFailure(() -> errorMessage, new IOException(errorMessage)); + handleFailure(channel, () -> errorMessage, new IOException(errorMessage)); return false; } @@ -254,7 +258,7 @@ private void writeRequest(HttpRequest request) { } else { // TODO: Are there cases where we can keep the channel open? closeAndRelease(channel); - handleFailure(() -> "Failed to make request to " + endpoint(), wireCall.cause()); + handleFailure(channel, () -> "Failed to make request to " + endpoint(), wireCall.cause()); } }); @@ -297,8 +301,8 @@ private URI endpoint() { return context.executeRequest().request().getUri(); } - private void handleFailure(Supplier msg, Throwable cause) { - log.debug(msg.get(), cause); + private void handleFailure(Channel channel, Supplier msgSupplier, Throwable cause) { + ChannelLogHelper.debug(log, channel, msgSupplier, cause); cause = decorateException(cause); context.handler().onError(cause); executeFuture.completeExceptionally(cause); @@ -379,7 +383,7 @@ private String getMessageForTooManyAcquireOperationsError() { * @param channel The channel. */ private void closeAndRelease(Channel channel) { - log.trace("closing and releasing channel {}", channel.id().asLongText()); + ChannelLogHelper.trace(log, channel, () -> String.format("closing and releasing channel %s", channel.id().asLongText())); channel.attr(KEEP_ALIVE).set(false); channel.close(); context.channelPool().release(channel); diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java new file mode 100644 index 000000000000..2b7c6edb5ed1 --- /dev/null +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java @@ -0,0 +1,63 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.http.nio.netty.internal.utils; + +import io.netty.channel.Channel; +import java.util.function.Supplier; +import org.slf4j.Logger; +import software.amazon.awssdk.annotations.SdkInternalApi; + +/** + * Utility class for logging the channel ID along with the message when within the context of a channel. + */ +@SdkInternalApi +public final class ChannelLogHelper { + private ChannelLogHelper() { + } + + public static void debug(Logger logger, Channel channel, Supplier msgSupplier, Throwable t) { + if (!logger.isDebugEnabled()) { + return; + } + + Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); + logger.debug(finalMessage.get(), t); + } + + public static void warn(Logger logger, Channel channel, Supplier msgSupplier, Throwable t) { + if (!logger.isWarnEnabled()) { + return; + } + + Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); + logger.warn(finalMessage.get(), t); + } + + public static void trace(Logger logger, Channel channel, Supplier msgSupplier) { + if (!logger.isTraceEnabled()) { + return; + } + + Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); + logger.trace(finalMessage.get()); + } + + private static Supplier prependChannelIdIfAvailable(Supplier msgSupplier, Channel channel) { + String id = channel.id().asShortText(); + + return () -> String.format("[Channel %s] %s", id, msgSupplier.get()); + } +} diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java new file mode 100644 index 000000000000..9a8d1e6d8496 --- /dev/null +++ b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java @@ -0,0 +1,125 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.http.nio.netty.internal.utils; + +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; + +import io.netty.channel.ChannelId; +import io.netty.channel.DefaultChannelId; +import io.netty.channel.embedded.EmbeddedChannel; +import java.util.function.Supplier; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.slf4j.Logger; + +@RunWith(MockitoJUnitRunner.class) +public class ChannelLogHelperTest { + private static final String TEST_MSG = "test"; + private static final ChannelId CHANNEL_ID = DefaultChannelId.newInstance(); + private static final String EXPECTED_MESSAGE = String.format("[Channel %s] %s", CHANNEL_ID.asShortText(), TEST_MSG); + + private static EmbeddedChannel ch; + + @Mock + public Logger mockLogger; + + @Mock + public Supplier msgSupplier; + + @BeforeClass + public static void setup() { + ch = new EmbeddedChannel(CHANNEL_ID); + } + + @Before + public void methodSetup() { + when(msgSupplier.get()).thenReturn(TEST_MSG); + } + + @AfterClass + public static void teardown() throws InterruptedException { + ch.close().await(); + } + + @Test + public void debugNotEnabled_doesNotInvokeLogger() { + when(mockLogger.isDebugEnabled()).thenReturn(false); + + ChannelLogHelper.debug(mockLogger, ch, msgSupplier, null); + + verify(mockLogger, never()).debug(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void debugEnabled_invokesLogger() { + when(mockLogger.isDebugEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + ChannelLogHelper.debug(mockLogger, ch, msgSupplier, exception); + + verify(mockLogger).debug(EXPECTED_MESSAGE, exception); + } + + @Test + public void warnNotEnabled_doesNotInvokeLogger() { + when(mockLogger.isWarnEnabled()).thenReturn(false); + + ChannelLogHelper.warn(mockLogger, ch, msgSupplier, null); + + verify(mockLogger, never()).warn(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void warnEnabled_invokesLogger() { + when(mockLogger.isWarnEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + ChannelLogHelper.warn(mockLogger, ch, msgSupplier, exception); + + verify(mockLogger).warn(EXPECTED_MESSAGE, exception); + } + + @Test + public void traceNotEnabled_doesNotInvokeLogger() { + when(mockLogger.isTraceEnabled()).thenReturn(false); + + ChannelLogHelper.trace(mockLogger, ch, msgSupplier); + + verify(mockLogger, never()).trace(anyString()); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void traceEnabled_invokesLogger() { + when(mockLogger.isTraceEnabled()).thenReturn(true); + + ChannelLogHelper.trace(mockLogger, ch, msgSupplier); + + verify(mockLogger).trace(EXPECTED_MESSAGE); + } +} From 4484776d9ff103f8f562ac1e666968d5cb174f73 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Tue, 7 Dec 2021 15:04:15 -0800 Subject: [PATCH 2/4] Review comments --- .../netty/internal/NettyRequestExecutor.java | 23 +- .../internal/utils/ChannelLogHelper.java | 63 ------ .../internal/utils/NettyClientLogger.java | 183 ++++++++++++++++ .../internal/utils/ChannelLogHelperTest.java | 125 ----------- .../internal/utils/NettyClientLoggerTest.java | 203 ++++++++++++++++++ 5 files changed, 395 insertions(+), 202 deletions(-) delete mode 100644 http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java create mode 100644 http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java delete mode 100644 http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java create mode 100644 http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java index b8cf2a8db7ee..b8163b7e7505 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java @@ -57,8 +57,6 @@ import org.reactivestreams.Publisher; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.http.Protocol; import software.amazon.awssdk.http.nio.netty.internal.http2.FlushOnReadHandler; @@ -67,14 +65,14 @@ import software.amazon.awssdk.http.nio.netty.internal.http2.HttpToHttp2OutboundAdapter; import software.amazon.awssdk.http.nio.netty.internal.nrs.HttpStreamsClientHandler; import software.amazon.awssdk.http.nio.netty.internal.nrs.StreamedHttpRequest; -import software.amazon.awssdk.http.nio.netty.internal.utils.ChannelLogHelper; import software.amazon.awssdk.http.nio.netty.internal.utils.ChannelUtils; +import software.amazon.awssdk.http.nio.netty.internal.utils.NettyClientLogger; import software.amazon.awssdk.http.nio.netty.internal.utils.NettyUtils; import software.amazon.awssdk.metrics.MetricCollector; @SdkInternalApi public final class NettyRequestExecutor { - private static final Logger log = LoggerFactory.getLogger(NettyRequestExecutor.class); + private static final NettyClientLogger log = NettyClientLogger.getLogger(NettyRequestExecutor.class); private static final RequestAdapter REQUEST_ADAPTER_HTTP2 = new RequestAdapter(Protocol.HTTP2); private static final RequestAdapter REQUEST_ADAPTER_HTTP1_1 = new RequestAdapter(Protocol.HTTP1_1); private static final AtomicLong EXECUTION_COUNTER = new AtomicLong(0L); @@ -130,10 +128,7 @@ private CompletableFuture createExecutionFuture(Promise channelPr } }); } catch (Throwable exc) { - ChannelLogHelper.warn(log, - ch, - () -> "Unable to add a task to cancel the request to channel's EventLoop", - exc); + log.warn(() -> "Unable to add a task to cancel the request to channel's EventLoop", exc); } } }); @@ -155,13 +150,13 @@ private void verifyMetricsWereCollected(CompletableFuture metricsFuture) { } if (!metricsFuture.isDone()) { - log.debug("HTTP request metric collection did not finish in time, so results may be incomplete."); + log.debug(() -> "HTTP request metric collection did not finish in time, so results may be incomplete."); metricsFuture.cancel(false); return; } metricsFuture.exceptionally(t -> { - log.debug("HTTP request metric collection failed, so results may be incomplete.", t); + log.debug(() -> "HTTP request metric collection failed, so results may be incomplete.", t); return null; }); } @@ -302,7 +297,7 @@ private URI endpoint() { } private void handleFailure(Channel channel, Supplier msgSupplier, Throwable cause) { - ChannelLogHelper.debug(log, channel, msgSupplier, cause); + log.debug(channel, msgSupplier, cause); cause = decorateException(cause); context.handler().onError(cause); executeFuture.completeExceptionally(cause); @@ -383,7 +378,7 @@ private String getMessageForTooManyAcquireOperationsError() { * @param channel The channel. */ private void closeAndRelease(Channel channel) { - ChannelLogHelper.trace(log, channel, () -> String.format("closing and releasing channel %s", channel.id().asLongText())); + log.trace(channel, () -> String.format("closing and releasing channel %s", channel.id().asLongText())); channel.attr(KEEP_ALIVE).set(false); channel.close(); context.channelPool().release(channel); @@ -476,7 +471,7 @@ public String toString() { /** * Decorator around {@link StreamedHttpRequest} to adapt a publisher of {@link ByteBuffer} (i.e. {@link * software.amazon.awssdk.http.async.SdkHttpContentPublisher}) to a publisher of {@link HttpContent}. - *

+ *

* This publisher also prevents the adapted publisher from publishing more content to the subscriber than * the specified 'Content-Length' of the request. */ @@ -569,7 +564,7 @@ private static Optional contentLength(HttpRequest request) { try { return Optional.of(Long.parseLong(value)); } catch (NumberFormatException e) { - log.warn("Unable to parse 'Content-Length' header. Treating it as non existent."); + log.warn(() -> "Unable to parse 'Content-Length' header. Treating it as non existent."); } } return Optional.empty(); diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java deleted file mode 100644 index 2b7c6edb5ed1..000000000000 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelper.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.awssdk.http.nio.netty.internal.utils; - -import io.netty.channel.Channel; -import java.util.function.Supplier; -import org.slf4j.Logger; -import software.amazon.awssdk.annotations.SdkInternalApi; - -/** - * Utility class for logging the channel ID along with the message when within the context of a channel. - */ -@SdkInternalApi -public final class ChannelLogHelper { - private ChannelLogHelper() { - } - - public static void debug(Logger logger, Channel channel, Supplier msgSupplier, Throwable t) { - if (!logger.isDebugEnabled()) { - return; - } - - Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); - logger.debug(finalMessage.get(), t); - } - - public static void warn(Logger logger, Channel channel, Supplier msgSupplier, Throwable t) { - if (!logger.isWarnEnabled()) { - return; - } - - Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); - logger.warn(finalMessage.get(), t); - } - - public static void trace(Logger logger, Channel channel, Supplier msgSupplier) { - if (!logger.isTraceEnabled()) { - return; - } - - Supplier finalMessage = prependChannelIdIfAvailable(msgSupplier, channel); - logger.trace(finalMessage.get()); - } - - private static Supplier prependChannelIdIfAvailable(Supplier msgSupplier, Channel channel) { - String id = channel.id().asShortText(); - - return () -> String.format("[Channel %s] %s", id, msgSupplier.get()); - } -} diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java new file mode 100644 index 000000000000..8728486b6ded --- /dev/null +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java @@ -0,0 +1,183 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.http.nio.netty.internal.utils; + +import io.netty.channel.Channel; +import java.util.function.Supplier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.annotations.SdkTestInternalApi; + +/** + * Logger facade similar to {@link software.amazon.awssdk.utils.Logger}, that also includes the Channel ID in the message when + * provided. + */ +@SdkInternalApi +public final class NettyClientLogger { + private final Logger delegateLogger; + + @SdkTestInternalApi + NettyClientLogger(Logger delegateLogger) { + this.delegateLogger = delegateLogger; + } + + public static NettyClientLogger getLogger(Class clzz) { + Logger delegate = LoggerFactory.getLogger(clzz); + return new NettyClientLogger(delegate); + } + + /** + * Log a DEBUG level message. + * + * @param msgSupplier Supplier for the log message + */ + public void debug(Supplier msgSupplier) { + debug(msgSupplier, null); + } + + /** + * Log a DEBUG level message with the given exception. + * + * @param msgSupplier Supplier for the log message + * @param t The throwable to log + */ + public void debug(Supplier msgSupplier, Throwable t) { + if (!delegateLogger.isDebugEnabled()) { + return; + } + + delegateLogger.debug(msgSupplier.get(), t); + } + + /** + * Log a DEBUG level message including the channel information. + * + * @param channel The channel for this message is being logged + * @param msgSupplier Supplier for the log message + */ + public void debug(Channel channel, Supplier msgSupplier) { + debug(channel, msgSupplier, null); + } + + /** + * Log a DEBUG level message with the given exception and including the channel information. + * + * @param channel The channel for this message is being logged + * @param msgSupplier Supplier for the log message + * @param t The throwable to log + */ + public void debug(Channel channel, Supplier msgSupplier, Throwable t) { + if (!delegateLogger.isDebugEnabled()) { + return; + } + + Supplier finalMessage = prependChannelInfo(true, msgSupplier, channel); + delegateLogger.debug(finalMessage.get(), t); + } + + /** + * Log a WARN level message. + * + * @param msgSupplier Supplier for the log message + */ + public void warn(Supplier msgSupplier) { + warn(msgSupplier, null); + } + + /** + * Log a WARN level message with the given exception. + * + * @param msgSupplier Supplier for the log message + * @param t The throwable to log + */ + public void warn(Supplier msgSupplier, Throwable t) { + if (!delegateLogger.isWarnEnabled()) { + return; + } + + delegateLogger.warn(msgSupplier.get(), t); + } + + /** + * Log a WARN level message and including the channel information. + * + * @param channel The channel for this message is being logged + * @param msgSupplier Supplier for the log message + */ + public void warn(Channel channel, Supplier msgSupplier) { + warn(channel, msgSupplier, null); + } + + /** + * Log a WARN level message with the given exception and including the channel information. + * + * @param channel The channel for this message is being logged + * @param msgSupplier Supplier for the log message + * @param t The throwable to log + */ + public void warn(Channel channel, Supplier msgSupplier, Throwable t) { + if (!delegateLogger.isWarnEnabled()) { + return; + } + + Supplier finalMessage = prependChannelInfo(msgSupplier, channel); + delegateLogger.warn(finalMessage.get(), t); + } + + /** + * Log a TRACE level message. + * + * @param msgSupplier Supplier for the log message + */ + public void trace(Supplier msgSupplier) { + if (!delegateLogger.isTraceEnabled()) { + return; + } + + delegateLogger.trace(msgSupplier.get()); + } + + /** + * Log a TRACE level message including the channel information. + * + * @param channel The channel for this message is being logged + * @param msgSupplier Supplier for the log message + */ + public void trace(Channel channel, Supplier msgSupplier) { + if (!delegateLogger.isTraceEnabled()) { + return; + } + + Supplier finalMessage = prependChannelInfo(true, msgSupplier, channel); + delegateLogger.trace(finalMessage.get()); + } + + private static Supplier prependChannelInfo(Supplier msgSupplier, Channel channel) { + return prependChannelInfo(false, msgSupplier, channel); + } + + private static Supplier prependChannelInfo(boolean useShortId, Supplier msgSupplier, Channel channel) { + String id; + if (useShortId) { + id = channel.id().asShortText(); + } else { + id = channel.toString(); + } + + return () -> String.format("[Channel ID: %s] %s", id, msgSupplier.get()); + } +} diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java deleted file mode 100644 index 9a8d1e6d8496..000000000000 --- a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/ChannelLogHelperTest.java +++ /dev/null @@ -1,125 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.awssdk.http.nio.netty.internal.utils; - -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; -import static org.mockito.Mockito.when; - -import io.netty.channel.ChannelId; -import io.netty.channel.DefaultChannelId; -import io.netty.channel.embedded.EmbeddedChannel; -import java.util.function.Supplier; -import org.junit.AfterClass; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.runners.MockitoJUnitRunner; -import org.slf4j.Logger; - -@RunWith(MockitoJUnitRunner.class) -public class ChannelLogHelperTest { - private static final String TEST_MSG = "test"; - private static final ChannelId CHANNEL_ID = DefaultChannelId.newInstance(); - private static final String EXPECTED_MESSAGE = String.format("[Channel %s] %s", CHANNEL_ID.asShortText(), TEST_MSG); - - private static EmbeddedChannel ch; - - @Mock - public Logger mockLogger; - - @Mock - public Supplier msgSupplier; - - @BeforeClass - public static void setup() { - ch = new EmbeddedChannel(CHANNEL_ID); - } - - @Before - public void methodSetup() { - when(msgSupplier.get()).thenReturn(TEST_MSG); - } - - @AfterClass - public static void teardown() throws InterruptedException { - ch.close().await(); - } - - @Test - public void debugNotEnabled_doesNotInvokeLogger() { - when(mockLogger.isDebugEnabled()).thenReturn(false); - - ChannelLogHelper.debug(mockLogger, ch, msgSupplier, null); - - verify(mockLogger, never()).debug(anyString(), any(Throwable.class)); - verifyZeroInteractions(msgSupplier); - } - - @Test - public void debugEnabled_invokesLogger() { - when(mockLogger.isDebugEnabled()).thenReturn(true); - RuntimeException exception = new RuntimeException("boom!"); - - ChannelLogHelper.debug(mockLogger, ch, msgSupplier, exception); - - verify(mockLogger).debug(EXPECTED_MESSAGE, exception); - } - - @Test - public void warnNotEnabled_doesNotInvokeLogger() { - when(mockLogger.isWarnEnabled()).thenReturn(false); - - ChannelLogHelper.warn(mockLogger, ch, msgSupplier, null); - - verify(mockLogger, never()).warn(anyString(), any(Throwable.class)); - verifyZeroInteractions(msgSupplier); - } - - @Test - public void warnEnabled_invokesLogger() { - when(mockLogger.isWarnEnabled()).thenReturn(true); - RuntimeException exception = new RuntimeException("boom!"); - - ChannelLogHelper.warn(mockLogger, ch, msgSupplier, exception); - - verify(mockLogger).warn(EXPECTED_MESSAGE, exception); - } - - @Test - public void traceNotEnabled_doesNotInvokeLogger() { - when(mockLogger.isTraceEnabled()).thenReturn(false); - - ChannelLogHelper.trace(mockLogger, ch, msgSupplier); - - verify(mockLogger, never()).trace(anyString()); - verifyZeroInteractions(msgSupplier); - } - - @Test - public void traceEnabled_invokesLogger() { - when(mockLogger.isTraceEnabled()).thenReturn(true); - - ChannelLogHelper.trace(mockLogger, ch, msgSupplier); - - verify(mockLogger).trace(EXPECTED_MESSAGE); - } -} diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java new file mode 100644 index 000000000000..26548f08f56f --- /dev/null +++ b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java @@ -0,0 +1,203 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.http.nio.netty.internal.utils; + +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; + +import io.netty.channel.Channel; +import io.netty.channel.ChannelId; +import io.netty.channel.DefaultChannelId; +import io.netty.channel.embedded.EmbeddedChannel; +import java.util.function.Supplier; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.slf4j.Logger; + +@RunWith(MockitoJUnitRunner.class) +public class NettyClientLoggerTest { + private static final String TEST_MSG = "test"; + private static final ChannelId CHANNEL_ID = DefaultChannelId.newInstance(); + private static final String CHANNEL_TO_STRING = "NettyClientLoggerTest_TestChannel"; + private static final String EXPECTED_MESSAGE_SHORT = String.format("[Channel ID: %s] %s", + CHANNEL_ID.asShortText(), + TEST_MSG); + private static final String EXPECTED_MESSAGE_FULL = String.format("[Channel ID: %s] %s", + CHANNEL_TO_STRING, + TEST_MSG); + + @Mock + public Logger delegateLogger; + + @Mock + public Supplier msgSupplier; + + private NettyClientLogger logger; + private EmbeddedChannel ch; + + + @BeforeClass + public static void setup() throws InterruptedException { + } + + @Before + public void methodSetup() { + when(msgSupplier.get()).thenReturn(TEST_MSG); + logger = new NettyClientLogger(delegateLogger); + ch = spy(new EmbeddedChannel(CHANNEL_ID)); + when(ch.toString()).thenReturn(CHANNEL_TO_STRING); + } + + @After + public void methodTeardown() throws InterruptedException { + ch.close().await(); + } + + @Test + public void debugNotEnabled_doesNotInvokeLogger() { + when(delegateLogger.isDebugEnabled()).thenReturn(false); + Channel channel = mock(Channel.class); + + logger.debug(channel, msgSupplier, null); + + verify(delegateLogger, never()).debug(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + verifyZeroInteractions(channel); + } + + @Test + public void debugEnabled_invokesLogger() { + when(delegateLogger.isDebugEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + logger.debug(ch, msgSupplier, exception); + + verify(delegateLogger).debug(EXPECTED_MESSAGE_SHORT, exception); + } + + @Test + public void debugNotEnabled_channelNotProvided_doesNotInvokeLogger() { + when(delegateLogger.isDebugEnabled()).thenReturn(false); + + logger.debug(msgSupplier, null); + + verify(delegateLogger, never()).debug(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void debugEnabled_channelNotProvided_invokesLogger() { + when(delegateLogger.isDebugEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + logger.debug(msgSupplier, exception); + + verify(delegateLogger).debug(TEST_MSG, exception); + } + + @Test + public void warnNotEnabled_doesNotInvokeLogger() { + when(delegateLogger.isWarnEnabled()).thenReturn(false); + Channel channel = mock(Channel.class); + + logger.warn(channel, msgSupplier, null); + + verify(delegateLogger, never()).warn(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + verifyZeroInteractions(channel); + } + + @Test + public void warnEnabled_invokesLogger() { + when(delegateLogger.isWarnEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + logger.warn(ch, msgSupplier, exception); + + verify(delegateLogger).warn(EXPECTED_MESSAGE_FULL, exception); + } + + @Test + public void warnNotEnabled_noChannelProvided_doesNotInvokeLogger() { + when(delegateLogger.isWarnEnabled()).thenReturn(false); + + logger.warn(msgSupplier, null); + + verify(delegateLogger, never()).warn(anyString(), any(Throwable.class)); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void warnEnabled_noChannelProvided_invokesLogger() { + when(delegateLogger.isWarnEnabled()).thenReturn(true); + RuntimeException exception = new RuntimeException("boom!"); + + logger.warn(msgSupplier, exception); + + verify(delegateLogger).warn(TEST_MSG, exception); + } + + @Test + public void traceNotEnabled_doesNotInvokeLogger() { + when(delegateLogger.isTraceEnabled()).thenReturn(false); + Channel channel = mock(Channel.class); + + logger.trace(channel, msgSupplier); + + verify(delegateLogger, never()).trace(anyString()); + verifyZeroInteractions(msgSupplier); + verifyZeroInteractions(channel); + } + + @Test + public void traceEnabled_invokesLogger() { + when(delegateLogger.isTraceEnabled()).thenReturn(true); + + logger.trace(ch, msgSupplier); + + verify(delegateLogger).trace(EXPECTED_MESSAGE_SHORT); + } + + @Test + public void traceNotEnabled_noChannelProvided_doesNotInvokeLogger() { + when(delegateLogger.isTraceEnabled()).thenReturn(false); + + logger.trace(msgSupplier); + + verify(delegateLogger, never()).trace(anyString()); + verifyZeroInteractions(msgSupplier); + } + + @Test + public void traceEnabled_noChannelProvided_invokesLogger() { + when(delegateLogger.isTraceEnabled()).thenReturn(true); + + logger.trace(msgSupplier); + + verify(delegateLogger).trace(TEST_MSG); + } +} From ebde6d2442dec162519e1b4dc51f0b5a44c96882 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Tue, 7 Dec 2021 16:21:39 -0800 Subject: [PATCH 3/4] Review comments --- .idea/inspectionProfiles/AWS_Java_SDK_2_0.xml | 6 +- .../netty/internal/NettyRequestExecutor.java | 8 +- .../internal/utils/NettyClientLogger.java | 75 ++----------------- .../internal/utils/NettyClientLoggerTest.java | 33 +++++--- 4 files changed, 38 insertions(+), 84 deletions(-) diff --git a/.idea/inspectionProfiles/AWS_Java_SDK_2_0.xml b/.idea/inspectionProfiles/AWS_Java_SDK_2_0.xml index c1a6646e493b..0328aa0e76e2 100644 --- a/.idea/inspectionProfiles/AWS_Java_SDK_2_0.xml +++ b/.idea/inspectionProfiles/AWS_Java_SDK_2_0.xml @@ -67,7 +67,7 @@ - @@ -195,7 +195,7 @@ - @@ -260,7 +260,7 @@ - diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java index b8163b7e7505..23508225fdd7 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java @@ -128,7 +128,7 @@ private CompletableFuture createExecutionFuture(Promise channelPr } }); } catch (Throwable exc) { - log.warn(() -> "Unable to add a task to cancel the request to channel's EventLoop", exc); + log.warn(ch, () -> "Unable to add a task to cancel the request to channel's EventLoop", exc); } } }); @@ -150,13 +150,13 @@ private void verifyMetricsWereCollected(CompletableFuture metricsFuture) { } if (!metricsFuture.isDone()) { - log.debug(() -> "HTTP request metric collection did not finish in time, so results may be incomplete."); + log.debug(null, () -> "HTTP request metric collection did not finish in time, so results may be incomplete."); metricsFuture.cancel(false); return; } metricsFuture.exceptionally(t -> { - log.debug(() -> "HTTP request metric collection failed, so results may be incomplete.", t); + log.debug(null, () -> "HTTP request metric collection failed, so results may be incomplete.", t); return null; }); } @@ -564,7 +564,7 @@ private static Optional contentLength(HttpRequest request) { try { return Optional.of(Long.parseLong(value)); } catch (NumberFormatException e) { - log.warn(() -> "Unable to parse 'Content-Length' header. Treating it as non existent."); + log.warn(null, () -> "Unable to parse 'Content-Length' header. Treating it as non existent."); } } return Optional.empty(); diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java index 8728486b6ded..ce4e9169c340 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java @@ -40,29 +40,6 @@ public static NettyClientLogger getLogger(Class clzz) { return new NettyClientLogger(delegate); } - /** - * Log a DEBUG level message. - * - * @param msgSupplier Supplier for the log message - */ - public void debug(Supplier msgSupplier) { - debug(msgSupplier, null); - } - - /** - * Log a DEBUG level message with the given exception. - * - * @param msgSupplier Supplier for the log message - * @param t The throwable to log - */ - public void debug(Supplier msgSupplier, Throwable t) { - if (!delegateLogger.isDebugEnabled()) { - return; - } - - delegateLogger.debug(msgSupplier.get(), t); - } - /** * Log a DEBUG level message including the channel information. * @@ -85,33 +62,10 @@ public void debug(Channel channel, Supplier msgSupplier, Throwable t) { return; } - Supplier finalMessage = prependChannelInfo(true, msgSupplier, channel); + Supplier finalMessage = prependChannelInfo(msgSupplier, channel); delegateLogger.debug(finalMessage.get(), t); } - /** - * Log a WARN level message. - * - * @param msgSupplier Supplier for the log message - */ - public void warn(Supplier msgSupplier) { - warn(msgSupplier, null); - } - - /** - * Log a WARN level message with the given exception. - * - * @param msgSupplier Supplier for the log message - * @param t The throwable to log - */ - public void warn(Supplier msgSupplier, Throwable t) { - if (!delegateLogger.isWarnEnabled()) { - return; - } - - delegateLogger.warn(msgSupplier.get(), t); - } - /** * Log a WARN level message and including the channel information. * @@ -138,19 +92,6 @@ public void warn(Channel channel, Supplier msgSupplier, Throwable t) { delegateLogger.warn(finalMessage.get(), t); } - /** - * Log a TRACE level message. - * - * @param msgSupplier Supplier for the log message - */ - public void trace(Supplier msgSupplier) { - if (!delegateLogger.isTraceEnabled()) { - return; - } - - delegateLogger.trace(msgSupplier.get()); - } - /** * Log a TRACE level message including the channel information. * @@ -162,22 +103,22 @@ public void trace(Channel channel, Supplier msgSupplier) { return; } - Supplier finalMessage = prependChannelInfo(true, msgSupplier, channel); + Supplier finalMessage = prependChannelInfo(msgSupplier, channel); delegateLogger.trace(finalMessage.get()); } - private static Supplier prependChannelInfo(Supplier msgSupplier, Channel channel) { - return prependChannelInfo(false, msgSupplier, channel); - } + private Supplier prependChannelInfo(Supplier msgSupplier, Channel channel) { + if (channel == null) { + return msgSupplier; + } - private static Supplier prependChannelInfo(boolean useShortId, Supplier msgSupplier, Channel channel) { String id; - if (useShortId) { + if (!delegateLogger.isDebugEnabled()) { id = channel.id().asShortText(); } else { id = channel.toString(); } - return () -> String.format("[Channel ID: %s] %s", id, msgSupplier.get()); + return () -> String.format("[Channel: %s] %s", id, msgSupplier.get()); } } diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java index 26548f08f56f..9d2892bed843 100644 --- a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java +++ b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLoggerTest.java @@ -43,10 +43,10 @@ public class NettyClientLoggerTest { private static final String TEST_MSG = "test"; private static final ChannelId CHANNEL_ID = DefaultChannelId.newInstance(); private static final String CHANNEL_TO_STRING = "NettyClientLoggerTest_TestChannel"; - private static final String EXPECTED_MESSAGE_SHORT = String.format("[Channel ID: %s] %s", + private static final String EXPECTED_MESSAGE_SHORT = String.format("[Channel: %s] %s", CHANNEL_ID.asShortText(), TEST_MSG); - private static final String EXPECTED_MESSAGE_FULL = String.format("[Channel ID: %s] %s", + private static final String EXPECTED_MESSAGE_FULL = String.format("[Channel: %s] %s", CHANNEL_TO_STRING, TEST_MSG); @@ -96,14 +96,14 @@ public void debugEnabled_invokesLogger() { logger.debug(ch, msgSupplier, exception); - verify(delegateLogger).debug(EXPECTED_MESSAGE_SHORT, exception); + verify(delegateLogger).debug(EXPECTED_MESSAGE_FULL, exception); } @Test public void debugNotEnabled_channelNotProvided_doesNotInvokeLogger() { when(delegateLogger.isDebugEnabled()).thenReturn(false); - logger.debug(msgSupplier, null); + logger.debug(null, msgSupplier, null); verify(delegateLogger, never()).debug(anyString(), any(Throwable.class)); verifyZeroInteractions(msgSupplier); @@ -114,7 +114,7 @@ public void debugEnabled_channelNotProvided_invokesLogger() { when(delegateLogger.isDebugEnabled()).thenReturn(true); RuntimeException exception = new RuntimeException("boom!"); - logger.debug(msgSupplier, exception); + logger.debug(null, msgSupplier, exception); verify(delegateLogger).debug(TEST_MSG, exception); } @@ -138,6 +138,18 @@ public void warnEnabled_invokesLogger() { logger.warn(ch, msgSupplier, exception); + verify(delegateLogger).warn(EXPECTED_MESSAGE_SHORT, exception); + } + + @Test + public void warnEnabled_debugEnabled_invokesLogger() { + when(delegateLogger.isWarnEnabled()).thenReturn(true); + when(delegateLogger.isDebugEnabled()).thenReturn(true); + + RuntimeException exception = new RuntimeException("boom!"); + + logger.warn(ch, msgSupplier, exception); + verify(delegateLogger).warn(EXPECTED_MESSAGE_FULL, exception); } @@ -145,7 +157,7 @@ public void warnEnabled_invokesLogger() { public void warnNotEnabled_noChannelProvided_doesNotInvokeLogger() { when(delegateLogger.isWarnEnabled()).thenReturn(false); - logger.warn(msgSupplier, null); + logger.warn(null, msgSupplier, null); verify(delegateLogger, never()).warn(anyString(), any(Throwable.class)); verifyZeroInteractions(msgSupplier); @@ -156,7 +168,7 @@ public void warnEnabled_noChannelProvided_invokesLogger() { when(delegateLogger.isWarnEnabled()).thenReturn(true); RuntimeException exception = new RuntimeException("boom!"); - logger.warn(msgSupplier, exception); + logger.warn(null, msgSupplier, exception); verify(delegateLogger).warn(TEST_MSG, exception); } @@ -176,17 +188,18 @@ public void traceNotEnabled_doesNotInvokeLogger() { @Test public void traceEnabled_invokesLogger() { when(delegateLogger.isTraceEnabled()).thenReturn(true); + when(delegateLogger.isDebugEnabled()).thenReturn(true); logger.trace(ch, msgSupplier); - verify(delegateLogger).trace(EXPECTED_MESSAGE_SHORT); + verify(delegateLogger).trace(EXPECTED_MESSAGE_FULL); } @Test public void traceNotEnabled_noChannelProvided_doesNotInvokeLogger() { when(delegateLogger.isTraceEnabled()).thenReturn(false); - logger.trace(msgSupplier); + logger.trace(null, msgSupplier); verify(delegateLogger, never()).trace(anyString()); verifyZeroInteractions(msgSupplier); @@ -196,7 +209,7 @@ public void traceNotEnabled_noChannelProvided_doesNotInvokeLogger() { public void traceEnabled_noChannelProvided_invokesLogger() { when(delegateLogger.isTraceEnabled()).thenReturn(true); - logger.trace(msgSupplier); + logger.trace(null, msgSupplier); verify(delegateLogger).trace(TEST_MSG); } From a34d7e34d215ca1540fd53ced3d0cb353cd2d0d1 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Wed, 8 Dec 2021 09:44:52 -0800 Subject: [PATCH 4/4] Review comments --- .../internal/utils/NettyClientLogger.java | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java index ce4e9169c340..cab1181580b0 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyClientLogger.java @@ -23,8 +23,16 @@ import software.amazon.awssdk.annotations.SdkTestInternalApi; /** - * Logger facade similar to {@link software.amazon.awssdk.utils.Logger}, that also includes the Channel ID in the message when - * provided. + * Logger facade similar to {@link software.amazon.awssdk.utils.Logger}, that also includes channel information in the message + * when provided. When the logger has at least DEBUG level enabled, the logger uses {@link Channel#toString()} to provide the + * complete information about the channel. If only less verbose levels are available, then only the channel's ID is logged. + *

+ * Having the channel information associated with the log message whenever available makes correlating messages that are all + * logged within the context of that channel possible; this is impossible to do otherwise because there is a 1:M mapping from + * event loops to channels. + *

+ * NOTE: The absence of overrides that don't take a {@code Channel} parameter is deliberate. This is done to lessen the + * chances that a {code Channel} is omitted from the log by accident. */ @SdkInternalApi public final class NettyClientLogger { @@ -62,8 +70,8 @@ public void debug(Channel channel, Supplier msgSupplier, Throwable t) { return; } - Supplier finalMessage = prependChannelInfo(msgSupplier, channel); - delegateLogger.debug(finalMessage.get(), t); + String finalMessage = prependChannelInfo(msgSupplier, channel); + delegateLogger.debug(finalMessage, t); } /** @@ -88,8 +96,8 @@ public void warn(Channel channel, Supplier msgSupplier, Throwable t) { return; } - Supplier finalMessage = prependChannelInfo(msgSupplier, channel); - delegateLogger.warn(finalMessage.get(), t); + String finalMessage = prependChannelInfo(msgSupplier, channel); + delegateLogger.warn(finalMessage, t); } /** @@ -103,13 +111,13 @@ public void trace(Channel channel, Supplier msgSupplier) { return; } - Supplier finalMessage = prependChannelInfo(msgSupplier, channel); - delegateLogger.trace(finalMessage.get()); + String finalMessage = prependChannelInfo(msgSupplier, channel); + delegateLogger.trace(finalMessage); } - private Supplier prependChannelInfo(Supplier msgSupplier, Channel channel) { + private String prependChannelInfo(Supplier msgSupplier, Channel channel) { if (channel == null) { - return msgSupplier; + return msgSupplier.get(); } String id; @@ -119,6 +127,6 @@ private Supplier prependChannelInfo(Supplier msgSupplier, Channe id = channel.toString(); } - return () -> String.format("[Channel: %s] %s", id, msgSupplier.get()); + return String.format("[Channel: %s] %s", id, msgSupplier.get()); } }