Repository: netty/netty
Issue: Ability to change the logger and level used by DnsNameResolverBuilder#traceEnabled

### Expected behavior
Ability to change the logger and level used by `DnsNameResolverBuilder#traceEnabled`

### Actual behavior
When `DnsNameResolverBuilder#traceEnabled` is enabled, it uses always the logger and level below
https://github.com/netty/netty/blob/46cb4015ff25ff2dd3d0dede5c3185ac498e56b0/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifeCycleObserverFactory.java#L26-L28

It would be nice if those can be configured.

Possible solutions:
- `DnsNameResolverBuilder#traceEnabled` where one can specify logger and level

Or

- `TraceDnsQueryLifeCycleObserverFactory` can be made public and also it can expose construction similar to what `LoggingHandler` provides i.e.

```
    public TraceDnsQueryLifeCycleObserverFactory(Class<?> clazz, LogLevel level) {
        ObjectUtil.checkNotNull(clazz, "clazz");
        ObjectUtil.checkNotNull(level, "level");
        this.logger = InternalLoggerFactory.getInstance(clazz);
        this.level = level.toInternalLevel();
    }
```

With this approach one will be able to add such observer with `DnsNameResolverBuilder#dnsQueryLifecycleObserverFactory`

this second approach will require a dependency to `netty-handler` module

Wdyt?


### Netty version
current snapshot

### JVM version (e.g. `java -version`)
jdk 8

### OS version (e.g. `uname -a`)
Mac OS 

Issue comments:
It's not clear to me why you want to change the log level, and if you want to change it to TRACE or INFO, or some other level? Log levels are normally hard-coded on the usage site, and the current code follows that convention.

This setting is about whether or not logging is enabled. Detail configuration of how logging works is something we'd prefer to leave to the logging framework.
@chrisvest The use case is the following: Reactor Netty wants to expose this as a configuration so that the user can decide which logger and what level to use. With the current approach the user sees that the tracing is enabled but needs to know the exact logger and level that are needed (which are not from Reactor Netty but from a library that Reactor Netty uses)
But that's the case with all logging in Netty, isn't it? Well, almost I suppose. The logging implementation that is used everywhere is discovered at runtime, and the level is always hard coded, with the exception of `LoggingHandler` and `Http2FrameLogger`. With that in mind I think the second approach makes the most sense: expose some clean-up version of `TraceDnsQueryLifeCycleObserverFactory`, and remove `traceEnabled`.
> But that's the case with all logging in Netty, isn't it? Well, almost I suppose. The logging implementation that is used everywhere is discovered at runtime, and the level is always hard coded, with the exception of `LoggingHandler` and `Http2FrameLogger`. With that in mind I think the second approach makes the most sense: expose some clean-up version of `TraceDnsQueryLifeCycleObserverFactory`, and remove `traceEnabled`.

Yes I agree, from my point of view it is like `LoggingHandler`, I can add this handler to the pipeline. The same is with `TraceDnsQueryLifeCycleObserverFactory` I can add it to the configuration.

Pull request review comments:
Should we use a `static` logger and so use `InternalLoggerFactory.getInstance(LoggingDnsQueryLifeCycleObserverFactory.class)` ?
final 
2020
private ?
![img](http://pa1.narvii.com/6187/d349302822f47c8ebb904fdcf971cfcaaefdb0c2_hq.gif)
May be add `requireNonNull(classContext)`
May be add `requireNonNull(nameContext)`
nit: I would just call this `name`

Patch that resolved the issue:
diff --git a/resolver-dns/pom.xml b/resolver-dns/pom.xml
index 8a918c4b97b..18cb12778ae 100644
--- a/resolver-dns/pom.xml
+++ b/resolver-dns/pom.xml
@@ -63,6 +63,11 @@
       <artifactId>netty-codec-dns</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>netty-handler</artifactId>
+      <version>${project.version}</version>
+    </dependency>
     <dependency>
       <groupId>org.apache.directory.server</groupId>
       <artifactId>apacheds-protocol-dns</artifactId>
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java
index cf485903d1a..44008fde58b 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java
@@ -272,7 +272,6 @@ protected DnsServerAddressStream initialValue() {
      * @param resolvedAddressTypes the preferred address types
      * @param recursionDesired if recursion desired flag must be set
      * @param maxQueriesPerResolve the maximum allowed number of DNS queries for a given name resolution
-     * @param traceEnabled if trace is enabled
      * @param maxPayloadSize the capacity of the datagram packet buffer
      * @param optResourceEnabled if automatic inclusion of a optional records is enabled
      * @param hostsFileEntriesResolver the {@link HostsFileEntriesResolver} used to check for local aliases
@@ -296,7 +295,6 @@ public DnsNameResolver(
             ResolvedAddressTypes resolvedAddressTypes,
             boolean recursionDesired,
             int maxQueriesPerResolve,
-            boolean traceEnabled,
             int maxPayloadSize,
             boolean optResourceEnabled,
             HostsFileEntriesResolver hostsFileEntriesResolver,
@@ -306,7 +304,7 @@ public DnsNameResolver(
             boolean decodeIdn) {
         this(eventLoop, channelFactory, resolveCache,
              new AuthoritativeDnsServerCacheAdapter(authoritativeDnsServerCache), dnsQueryLifecycleObserverFactory,
-             queryTimeoutMillis, resolvedAddressTypes, recursionDesired, maxQueriesPerResolve, traceEnabled,
+             queryTimeoutMillis, resolvedAddressTypes, recursionDesired, maxQueriesPerResolve,
              maxPayloadSize, optResourceEnabled, hostsFileEntriesResolver, dnsServerAddressStreamProvider,
              searchDomains, ndots, decodeIdn);
     }
@@ -324,7 +322,6 @@ public DnsNameResolver(
      * @param resolvedAddressTypes the preferred address types
      * @param recursionDesired if recursion desired flag must be set
      * @param maxQueriesPerResolve the maximum allowed number of DNS queries for a given name resolution
-     * @param traceEnabled if trace is enabled
      * @param maxPayloadSize the capacity of the datagram packet buffer
      * @param optResourceEnabled if automatic inclusion of a optional records is enabled
      * @param hostsFileEntriesResolver the {@link HostsFileEntriesResolver} used to check for local aliases
@@ -348,7 +345,6 @@ public DnsNameResolver(
             ResolvedAddressTypes resolvedAddressTypes,
             boolean recursionDesired,
             int maxQueriesPerResolve,
-            boolean traceEnabled,
             int maxPayloadSize,
             boolean optResourceEnabled,
             HostsFileEntriesResolver hostsFileEntriesResolver,
@@ -358,7 +354,7 @@ public DnsNameResolver(
             boolean decodeIdn) {
         this(eventLoop, channelFactory, null, resolveCache, NoopDnsCnameCache.INSTANCE, authoritativeDnsServerCache,
              dnsQueryLifecycleObserverFactory, queryTimeoutMillis, resolvedAddressTypes, recursionDesired,
-             maxQueriesPerResolve, traceEnabled, maxPayloadSize, optResourceEnabled, hostsFileEntriesResolver,
+             maxQueriesPerResolve, maxPayloadSize, optResourceEnabled, hostsFileEntriesResolver,
              dnsServerAddressStreamProvider, searchDomains, ndots, decodeIdn, false);
     }
 
@@ -374,7 +370,6 @@ public DnsNameResolver(
             ResolvedAddressTypes resolvedAddressTypes,
             boolean recursionDesired,
             int maxQueriesPerResolve,
-            boolean traceEnabled,
             int maxPayloadSize,
             boolean optResourceEnabled,
             HostsFileEntriesResolver hostsFileEntriesResolver,
@@ -397,11 +392,7 @@ public DnsNameResolver(
                 requireNonNull(dnsServerAddressStreamProvider, "dnsServerAddressStreamProvider");
         this.resolveCache = requireNonNull(resolveCache, "resolveCache");
         this.cnameCache = requireNonNull(cnameCache, "cnameCache");
-        this.dnsQueryLifecycleObserverFactory = traceEnabled ?
-                dnsQueryLifecycleObserverFactory instanceof NoopDnsQueryLifecycleObserverFactory ?
-                        new TraceDnsQueryLifeCycleObserverFactory() :
-                        new BiDnsQueryLifecycleObserverFactory(new TraceDnsQueryLifeCycleObserverFactory(),
-                                                               dnsQueryLifecycleObserverFactory) :
+        this.dnsQueryLifecycleObserverFactory =
                 requireNonNull(dnsQueryLifecycleObserverFactory, "dnsQueryLifecycleObserverFactory");
         this.searchDomains = searchDomains != null ? searchDomains.clone() : DEFAULT_SEARCH_DOMAINS;
         this.ndots = ndots >= 0 ? ndots : DEFAULT_OPTIONS.ndots();
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
index a03978eb91b..d107aa1a9a2 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
@@ -318,18 +318,6 @@ public DnsNameResolverBuilder maxQueriesPerResolve(int maxQueriesPerResolve) {
         return this;
     }
 
-    /**
-     * Sets if this resolver should generate the detailed trace information in an exception message so that
-     * it is easier to understand the cause of resolution failure.
-     *
-     * @param traceEnabled true if trace is enabled
-     * @return {@code this}
-     */
-    public DnsNameResolverBuilder traceEnabled(boolean traceEnabled) {
-        this.traceEnabled = traceEnabled;
-        return this;
-    }
-
     /**
      * Sets the capacity of the datagram packet buffer (in bytes).  The default value is {@code 4096} bytes.
      *
@@ -482,7 +470,6 @@ public DnsNameResolver build() {
                 resolvedAddressTypes,
                 recursionDesired,
                 maxQueriesPerResolve,
-                traceEnabled,
                 maxPayloadSize,
                 optResourceEnabled,
                 hostsFileEntriesResolver,
@@ -540,7 +527,6 @@ public DnsNameResolverBuilder copy() {
         copiedBuilder.resolvedAddressTypes(resolvedAddressTypes);
         copiedBuilder.recursionDesired(recursionDesired);
         copiedBuilder.maxQueriesPerResolve(maxQueriesPerResolve);
-        copiedBuilder.traceEnabled(traceEnabled);
         copiedBuilder.maxPayloadSize(maxPayloadSize);
         copiedBuilder.optResourceEnabled(optResourceEnabled);
         copiedBuilder.hostsFileEntriesResolver(hostsFileEntriesResolver);
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifeCycleObserverFactory.java b/resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifeCycleObserverFactory.java
new file mode 100644
index 00000000000..f24222b3537
--- /dev/null
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifeCycleObserverFactory.java
@@ -0,0 +1,85 @@
+/*
+ * Copyright 2020 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License 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 io.netty.resolver.dns;
+
+import io.netty.handler.codec.dns.DnsQuestion;
+import io.netty.handler.logging.LogLevel;
+import io.netty.util.internal.logging.InternalLogLevel;
+import io.netty.util.internal.logging.InternalLogger;
+import io.netty.util.internal.logging.InternalLoggerFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A {@link DnsQueryLifecycleObserverFactory} that enables detailed logging in the {@link DnsNameResolver}.
+ * <p>
+ * When {@linkplain DnsNameResolverBuilder#dnsQueryLifecycleObserverFactory(DnsQueryLifecycleObserverFactory)
+ * configured on the resolver}, detailed trace information will be generated so that it is easier to understand the
+ * cause of resolution failure.
+ */
+public final class LoggingDnsQueryLifeCycleObserverFactory implements DnsQueryLifecycleObserverFactory {
+    private static final InternalLogger DEFAULT_LOGGER =
+            InternalLoggerFactory.getInstance(LoggingDnsQueryLifeCycleObserverFactory.class);
+    private final InternalLogger logger;
+    private final InternalLogLevel level;
+
+    /**
+     * Create {@link DnsQueryLifecycleObserver} instances that log events at the default {@link LogLevel#DEBUG} level.
+     */
+    public LoggingDnsQueryLifeCycleObserverFactory() {
+        this(LogLevel.DEBUG);
+    }
+
+    /**
+     * Create {@link DnsQueryLifecycleObserver} instances that log events at the given log level.
+     * @param level The log level to use for logging resolver events.
+     */
+    public LoggingDnsQueryLifeCycleObserverFactory(LogLevel level) {
+        this.level = checkAndConvertLevel(level);
+        logger = DEFAULT_LOGGER;
+    }
+
+    /**
+     * Create {@link DnsQueryLifecycleObserver} instances that log events to a logger with the given class context,
+     * at the given log level.
+     * @param classContext The class context for the logger to use.
+     * @param level The log level to use for logging resolver events.
+     */
+    public LoggingDnsQueryLifeCycleObserverFactory(Class<?> classContext, LogLevel level) {
+        this.level = checkAndConvertLevel(level);
+        logger = InternalLoggerFactory.getInstance(requireNonNull(classContext, "classContext"));
+    }
+
+    /**
+     * Create {@link DnsQueryLifecycleObserver} instances that log events to a logger with the given name context,
+     * at the given log level.
+     * @param name The name for the logger to use.
+     * @param level The log level to use for logging resolver events.
+     */
+    public LoggingDnsQueryLifeCycleObserverFactory(String name, LogLevel level) {
+        this.level = checkAndConvertLevel(level);
+        logger = InternalLoggerFactory.getInstance(requireNonNull(name, "name"));
+    }
+
+    private static InternalLogLevel checkAndConvertLevel(LogLevel level) {
+        return requireNonNull(level, "level").toInternalLevel();
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver newDnsQueryLifecycleObserver(DnsQuestion question) {
+        return new LoggingDnsQueryLifecycleObserver(question, logger, level);
+    }
+}
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifecycleObserver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifecycleObserver.java
similarity index 93%
rename from resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifecycleObserver.java
rename to resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifecycleObserver.java
index 2a71aed5624..3f90957d6d8 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifecycleObserver.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/LoggingDnsQueryLifecycleObserver.java
@@ -26,13 +26,13 @@
 
 import static java.util.Objects.requireNonNull;
 
-final class TraceDnsQueryLifecycleObserver implements DnsQueryLifecycleObserver {
+final class LoggingDnsQueryLifecycleObserver implements DnsQueryLifecycleObserver {
     private final InternalLogger logger;
     private final InternalLogLevel level;
     private final DnsQuestion question;
     private InetSocketAddress dnsServerAddress;
 
-    TraceDnsQueryLifecycleObserver(DnsQuestion question, InternalLogger logger, InternalLogLevel level) {
+    LoggingDnsQueryLifecycleObserver(DnsQuestion question, InternalLogger logger, InternalLogLevel level) {
         this.question = requireNonNull(question, "question");
         this.logger = requireNonNull(logger, "logger");
         this.level = requireNonNull(level, "level");
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifeCycleObserverFactory.java b/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifeCycleObserverFactory.java
deleted file mode 100644
index b0ad8cc785e..00000000000
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifeCycleObserverFactory.java
+++ /dev/null
@@ -1,45 +0,0 @@
-/*
- * Copyright 2017 The Netty Project
- *
- * The Netty Project licenses this file to you under the Apache License,
- * version 2.0 (the "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at:
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License 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 io.netty.resolver.dns;
-
-import io.netty.handler.codec.dns.DnsQuestion;
-import io.netty.util.internal.logging.InternalLogLevel;
-import io.netty.util.internal.logging.InternalLogger;
-import io.netty.util.internal.logging.InternalLoggerFactory;
-
-import static java.util.Objects.requireNonNull;
-
-final class TraceDnsQueryLifeCycleObserverFactory implements DnsQueryLifecycleObserverFactory {
-    private static final InternalLogger DEFAULT_LOGGER =
-            InternalLoggerFactory.getInstance(TraceDnsQueryLifeCycleObserverFactory.class);
-    private static final InternalLogLevel DEFAULT_LEVEL = InternalLogLevel.DEBUG;
-    private final InternalLogger logger;
-    private final InternalLogLevel level;
-
-    TraceDnsQueryLifeCycleObserverFactory() {
-        this(DEFAULT_LOGGER, DEFAULT_LEVEL);
-    }
-
-    TraceDnsQueryLifeCycleObserverFactory(InternalLogger logger, InternalLogLevel level) {
-        this.logger = requireNonNull(logger, "logger");
-        this.level = requireNonNull(level, "level");
-    }
-
-    @Override
-    public DnsQueryLifecycleObserver newDnsQueryLifecycleObserver(DnsQuestion question) {
-        return new TraceDnsQueryLifecycleObserver(question, logger, level);
-    }
-}


Test patch:
diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
index f15df51e74f..3154f1808f0 100644
--- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
+++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
@@ -922,7 +922,7 @@ private static void testCNAMERecursiveResolveMultipleNameServers(boolean ipv4Pre
                     group.next(), new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class),
                     NoopDnsCache.INSTANCE, nsCache, NoopDnsQueryLifecycleObserverFactory.INSTANCE, 3000,
                     ipv4Preferred ? ResolvedAddressTypes.IPV4_ONLY : ResolvedAddressTypes.IPV6_ONLY, true,
-                    10, true, 4096, false, HostsFileEntriesResolver.DEFAULT,
+                    10, 4096, false, HostsFileEntriesResolver.DEFAULT,
                     new SequentialDnsServerAddressStreamProvider(dnsServer2.localAddress(), dnsServer3.localAddress()),
                     DnsNameResolver.DEFAULT_SEARCH_DOMAINS, 0, true) {
                 @Override
@@ -1268,7 +1268,7 @@ private static void testRecursiveResolveCache(boolean cache)
         final DnsNameResolver resolver = new DnsNameResolver(
                 group.next(), new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class),
                 NoopDnsCache.INSTANCE, nsCache, lifecycleObserverFactory, 3000, ResolvedAddressTypes.IPV4_ONLY, true,
-                10, true, 4096, false, HostsFileEntriesResolver.DEFAULT,
+                10, 4096, false, HostsFileEntriesResolver.DEFAULT,
                 new SingletonDnsServerAddressStreamProvider(dnsServer.localAddress()),
                 DnsNameResolver.DEFAULT_SEARCH_DOMAINS, 0, true) {
             @Override
@@ -1425,7 +1425,7 @@ protected DnsMessage filterMessage(DnsMessage message) {
         final DnsNameResolver resolver = new DnsNameResolver(
                 group.next(), new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class),
                 cache, authoritativeDnsServerCache, NoopDnsQueryLifecycleObserverFactory.INSTANCE, 2000,
-                ResolvedAddressTypes.IPV4_ONLY, true, 10, true, 4096,
+                ResolvedAddressTypes.IPV4_ONLY, true, 10, 4096,
                 false, HostsFileEntriesResolver.DEFAULT,
                 new SingletonDnsServerAddressStreamProvider(redirectServer.localAddress()),
                 DnsNameResolver.DEFAULT_SEARCH_DOMAINS, 0, true) {
@@ -1587,7 +1587,7 @@ public boolean clear(String hostname) {
                 group.next(), new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class),
                 NoopDnsCache.INSTANCE, authoritativeDnsServerCache,
                 NoopDnsQueryLifecycleObserverFactory.INSTANCE, 2000, ResolvedAddressTypes.IPV4_ONLY,
-                true, 10, true, 4096,
+                true, 10, 4096,
                 false, HostsFileEntriesResolver.DEFAULT,
                 new SingletonDnsServerAddressStreamProvider(redirectServer.localAddress()),
                 DnsNameResolver.DEFAULT_SEARCH_DOMAINS, 0, true) {
@@ -1724,7 +1724,7 @@ public boolean clear(String hostname) {
                 loop, new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class),
                 cache, authoritativeDnsServerCache,
                 NoopDnsQueryLifecycleObserverFactory.INSTANCE, 2000, ResolvedAddressTypes.IPV4_ONLY,
-                true, 10, true, 4096,
+                true, 10, 4096,
                 false, HostsFileEntriesResolver.DEFAULT,
                 new SingletonDnsServerAddressStreamProvider(redirectServer.localAddress()),
                 DnsNameResolver.DEFAULT_SEARCH_DOMAINS, 0, true) {
@@ -2532,7 +2532,6 @@ public void testInstanceWithNullPreferredAddressType() {
                 null, // resolvedAddressTypes, see https://github.com/netty/netty/pull/8445
                 true, // recursionDesired
                 1, // maxQueriesPerResolve
-                false, // traceEnabled
                 4096, // maxPayloadSize
                 true, // optResourceEnabled
                 HostsFileEntriesResolver.DEFAULT, // hostsFileEntriesResolver