Replace DnsNameResolverContext#trace special code with an implementation of DnsQueryLifecycleObserver

Motivation:
DnsQueryLifecycleObserver is designed to capture the life cycle of every query. DnsNameResolverContext has a custom trace mechanism which consists of a StringBuilder and manual calls throughout the class. We can remove some special case code in DnsNameResolverContext and instead use a special implementation of DnsQueryLifecycleObserver when trace is enabled.

Modifications:
- Remove all references to the boolean trace variables in DnsNameResolverContext and DnsNameResolver
- Introduce TraceDnsQueryLifecycleObserver which will be used when trace is enabled and will log similar data as what trace currently provides

Result:
Less special case code in DnsNameResolverContext and instead delegate to TraceDnsQueryLifecycleObserver to capture trace information.

resolver-dns/src/main/java/io/netty/resolver/dns/BiDnsQueryLifecycleObserver.java

@@ -0,0 +1,111 @@
+/*
+ * 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.channel.ChannelFuture;
+import io.netty.handler.codec.dns.DnsQuestion;
+import io.netty.handler.codec.dns.DnsResponseCode;
+import io.netty.util.internal.UnstableApi;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
+/**
+ * Combines two {@link DnsQueryLifecycleObserver} into a single {@link DnsQueryLifecycleObserver}.
+ */
+@UnstableApi
+public final class BiDnsQueryLifecycleObserver implements DnsQueryLifecycleObserver {
+    private final DnsQueryLifecycleObserver a;
+    private final DnsQueryLifecycleObserver b;
+
+    /**
+     * Create a new instance.
+     * @param a The {@link DnsQueryLifecycleObserver} that will receive events first.
+     * @param b The {@link DnsQueryLifecycleObserver} that will receive events second.
+     */
+    public BiDnsQueryLifecycleObserver(DnsQueryLifecycleObserver a, DnsQueryLifecycleObserver b) {
+        this.a = checkNotNull(a, "a");
+        this.b = checkNotNull(b, "b");
+    }
+
+    @Override
+    public void queryWritten(InetSocketAddress dnsServerAddress, ChannelFuture future) {
+        try {
+            a.queryWritten(dnsServerAddress, future);
+        } finally {
+            b.queryWritten(dnsServerAddress, future);
+        }
+    }
+
+    @Override
+    public void queryCancelled(int queriesRemaining) {
+        try {
+            a.queryCancelled(queriesRemaining);
+        } finally {
+            b.queryCancelled(queriesRemaining);
+        }
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryRedirected(List<InetSocketAddress> nameServers) {
+        try {
+            a.queryRedirected(nameServers);
+        } finally {
+            b.queryRedirected(nameServers);
+        }
+        return this;
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryCNAMEd(DnsQuestion cnameQuestion) {
+        try {
+            a.queryCNAMEd(cnameQuestion);
+        } finally {
+            b.queryCNAMEd(cnameQuestion);
+        }
+        return this;
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryNoAnswer(DnsResponseCode code) {
+        try {
+            a.queryNoAnswer(code);
+        } finally {
+            b.queryNoAnswer(code);
+        }
+        return this;
+    }
+
+    @Override
+    public void queryFailed(Throwable cause) {
+        try {
+            a.queryFailed(cause);
+        } finally {
+            b.queryFailed(cause);
+        }
+    }
+
+    @Override
+    public void querySucceed() {
+        try {
+            a.querySucceed();
+        } finally {
+            b.querySucceed();
+        }
+    }
+}

resolver-dns/src/main/java/io/netty/resolver/dns/BiDnsQueryLifecycleObserverFactory.java

@@ -0,0 +1,46 @@
+/*
+ * 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.UnstableApi;
+
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
+/**
+ * Combines two {@link DnsQueryLifecycleObserverFactory} into a single {@link DnsQueryLifecycleObserverFactory}.
+ */
+@UnstableApi
+public final class BiDnsQueryLifecycleObserverFactory implements DnsQueryLifecycleObserverFactory {
+    private final DnsQueryLifecycleObserverFactory a;
+    private final DnsQueryLifecycleObserverFactory b;
+
+    /**
+     * Create a new instance.
+     * @param a The {@link DnsQueryLifecycleObserverFactory} that will receive events first.
+     * @param b The {@link DnsQueryLifecycleObserverFactory} that will receive events second.
+     */
+    public BiDnsQueryLifecycleObserverFactory(DnsQueryLifecycleObserverFactory a, DnsQueryLifecycleObserverFactory b) {
+        this.a = checkNotNull(a, "a");
+        this.b = checkNotNull(b, "b");
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver newDnsQueryLifecycleObserver(DnsQuestion question) {
+        return new BiDnsQueryLifecycleObserver(a.newDnsQueryLifecycleObserver(question),
+                                               b.newDnsQueryLifecycleObserver(question));
+    }
+}

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java

@@ -167,7 +167,6 @@ public class DnsNameResolver extends InetNameResolver {
 
     private final long queryTimeoutMillis;
     private final int maxQueriesPerResolve;
-    private final boolean traceEnabled;
     private final ResolvedAddressTypes resolvedAddressTypes;
     private final InternetProtocolFamily[] resolvedInternetProtocolFamilies;
     private final boolean recursionDesired;
@@ -232,7 +231,6 @@ public class DnsNameResolver extends InetNameResolver {
         this.resolvedAddressTypes = resolvedAddressTypes != null ? resolvedAddressTypes : DEFAULT_RESOLVE_ADDRESS_TYPES;
         this.recursionDesired = recursionDesired;
         this.maxQueriesPerResolve = checkPositive(maxQueriesPerResolve, "maxQueriesPerResolve");
-        this.traceEnabled = traceEnabled;
         this.maxPayloadSize = checkPositive(maxPayloadSize, "maxPayloadSize");
         this.optResourceEnabled = optResourceEnabled;
         this.hostsFileEntriesResolver = checkNotNull(hostsFileEntriesResolver, "hostsFileEntriesResolver");
@@ -240,7 +238,11 @@ public class DnsNameResolver extends InetNameResolver {
                 checkNotNull(dnsServerAddressStreamProvider, "dnsServerAddressStreamProvider");
         this.resolveCache = checkNotNull(resolveCache, "resolveCache");
         this.authoritativeDnsServerCache = checkNotNull(authoritativeDnsServerCache, "authoritativeDnsServerCache");
-        this.dnsQueryLifecycleObserverFactory =
+        this.dnsQueryLifecycleObserverFactory = traceEnabled ?
+                    dnsQueryLifecycleObserverFactory instanceof NoopDnsQueryLifecycleObserverFactory ?
+                    new TraceDnsQueryLifeCycleObserverFactory() :
+                new BiDnsQueryLifecycleObserverFactory(new TraceDnsQueryLifeCycleObserverFactory(),
+                                                       dnsQueryLifecycleObserverFactory) :
                 checkNotNull(dnsQueryLifecycleObserverFactory, "dnsQueryLifecycleObserverFactory");
         this.searchDomains = searchDomains != null ? searchDomains.clone() : DEFAULT_SEARCH_DOMAINS;
         this.ndots = ndots >= 0 ? ndots : DEFAULT_NDOTS;
@@ -399,14 +401,6 @@ public class DnsNameResolver extends InetNameResolver {
         return maxQueriesPerResolve;
     }
 
-    /**
-     * Returns 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. The default value if {@code true}.
-     */
-    public boolean isTraceEnabled() {
-        return traceEnabled;
-    }
-
     /**
      * Returns the capacity of the datagram packet buffer (in bytes).  The default value is {@code 4096} bytes.
      */

resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java

@@ -92,7 +92,6 @@ abstract class DnsNameResolverContext<T> {
     private final DnsServerAddressStream nameServerAddrs;
     private final String hostname;
     private final DnsCache resolveCache;
-    private final boolean traceEnabled;
     private final int maxAllowedQueries;
     private final InternetProtocolFamily[] resolvedInternetProtocolFamilies;
     private final DnsRecord[] additionals;
@@ -103,15 +102,14 @@ abstract class DnsNameResolverContext<T> {
 
     private String pristineHostname;
     private List<DnsCacheEntry> resolvedEntries;
-    private StringBuilder trace;
     private int allowedQueries;
     private boolean triedCNAME;
 
-    protected DnsNameResolverContext(DnsNameResolver parent,
+    DnsNameResolverContext(DnsNameResolver parent,
-                                     String hostname,
+                           String hostname,
-                                     DnsRecord[] additionals,
+                           DnsRecord[] additionals,
-                                     DnsCache resolveCache,
+                           DnsCache resolveCache,
-                                     DnsServerAddressStream nameServerAddrs) {
+                           DnsServerAddressStream nameServerAddrs) {
         this.parent = parent;
         this.hostname = hostname;
         this.additionals = additionals;
@@ -120,7 +118,6 @@ abstract class DnsNameResolverContext<T> {
         this.nameServerAddrs = ObjectUtil.checkNotNull(nameServerAddrs, "nameServerAddrs");
         maxAllowedQueries = parent.maxQueriesPerResolve();
         resolvedInternetProtocolFamilies = parent.resolvedInternetProtocolFamiliesUnsafe();
-        traceEnabled = parent.isTraceEnabled();
         allowedQueries = maxAllowedQueries;
     }
 
@@ -300,11 +297,7 @@ abstract class DnsNameResolverContext<T> {
                         onResponse(nameServerAddrStream, question, future.getNow(), queryLifecycleObserver, promise);
                     } else {
                         // Server did not respond or I/O error occurred; try again.
-                        Throwable cause = future.cause();
+                        queryLifecycleObserver.queryFailed(future.cause());
-                        queryLifecycleObserver.queryFailed(cause);
-                        if (traceEnabled) {
-                            addTrace(cause);
-                        }
                         query(nameServerAddrStream, question, promise);
                     }
                 } finally {
@@ -338,12 +331,6 @@ abstract class DnsNameResolverContext<T> {
                 return;
             }
 
-            if (traceEnabled) {
-                addTrace(envelope.sender(),
-                         "response code: " + code + " with " + res.count(DnsSection.ANSWER) + " answer(s) and " +
-                         res.count(DnsSection.AUTHORITY) + " authority resource(s)");
-            }
-
             // Retry with the next server if the server did not tell us that the domain does not exist.
             if (code != DnsResponseCode.NXDOMAIN) {
                 query(nameServerAddrStream, question, queryLifecycleObserver.queryNoAnswer(code), promise);
@@ -398,12 +385,7 @@ abstract class DnsNameResolverContext<T> {
                     addNameServerToCache(authoritativeNameServer, resolved, r.timeToLive());
                 }
 
-                if (nameServers.isEmpty()) {
+                if (!nameServers.isEmpty()) {
-                    if (traceEnabled) {
-                        addTrace(envelope.sender(),
-                                 "no matching authoritative name server found in the ADDITIONALS section");
-                    }
-                } else {
                     query(parent.uncachedRedirectDnsServerStream(nameServers), question,
                           queryLifecycleObserver.queryRedirected(unmodifiableList(nameServers)), promise);
                     return true;
@@ -489,15 +471,11 @@ abstract class DnsNameResolverContext<T> {
             return;
         }
 
-        if (traceEnabled) {
-            addTrace(envelope.sender(), "no matching " + qType + " record found");
-        }
-
         if (cnames.isEmpty()) {
             queryLifecycleObserver.queryFailed(NO_MATCHING_RECORD_QUERY_FAILED_EXCEPTION);
         } else {
             // We asked for A/AAAA but we got only CNAME.
-            onResponseCNAME(question, envelope, cnames, false, queryLifecycleObserver, promise);
+            onResponseCNAME(question, envelope, cnames, queryLifecycleObserver, promise);
         }
     }
 
@@ -526,12 +504,12 @@ abstract class DnsNameResolverContext<T> {
     private void onResponseCNAME(DnsQuestion question, AddressedEnvelope<DnsResponse, InetSocketAddress> envelope,
                                  final DnsQueryLifecycleObserver queryLifecycleObserver,
                                  Promise<T> promise) {
-        onResponseCNAME(question, envelope, buildAliasMap(envelope.content()), true, queryLifecycleObserver, promise);
+        onResponseCNAME(question, envelope, buildAliasMap(envelope.content()), queryLifecycleObserver, promise);
     }
 
     private void onResponseCNAME(
             DnsQuestion question, AddressedEnvelope<DnsResponse, InetSocketAddress> response,
-            Map<String, String> cnames, boolean trace, final DnsQueryLifecycleObserver queryLifecycleObserver,
+            Map<String, String> cnames, final DnsQueryLifecycleObserver queryLifecycleObserver,
             Promise<T> promise) {
 
         // Resolve the host name in the question into the real host name.
@@ -554,9 +532,6 @@ abstract class DnsNameResolverContext<T> {
             followCname(response.sender(), name, resolved, queryLifecycleObserver, promise);
         } else {
             queryLifecycleObserver.queryFailed(CNAME_NOT_FOUND_QUERY_FAILED_EXCEPTION);
-            if (trace && traceEnabled) {
-                addTrace(response.sender(), "no matching CNAME record found");
-            }
         }
     }
 
@@ -679,10 +654,6 @@ abstract class DnsNameResolverContext<T> {
                 .append(' ');
             }
         }
-        if (trace != null) {
-            buf.append(':')
-               .append(trace);
-        }
         final UnknownHostException cause = new UnknownHostException(buf.toString());
 
         resolveCache.cache(hostname, additionals, cause, parent.ch.eventLoop());
@@ -716,21 +687,6 @@ abstract class DnsNameResolverContext<T> {
     private void followCname(InetSocketAddress nameServerAddr, String name, String cname,
                              final DnsQueryLifecycleObserver queryLifecycleObserver,
                              Promise<T> promise) {
-
-        if (traceEnabled) {
-            if (trace == null) {
-                trace = new StringBuilder(128);
-            }
-
-            trace.append(StringUtil.NEWLINE);
-            trace.append("\tfrom ");
-            trace.append(nameServerAddr);
-            trace.append(": ");
-            trace.append(name);
-            trace.append(" CNAME ");
-            trace.append(cname);
-        }
-
         // Use the same server for both CNAME queries
         DnsServerAddressStream stream = DnsServerAddresses.singleton(getNameServers(cname).next()).stream();
 
@@ -774,39 +730,10 @@ abstract class DnsNameResolverContext<T> {
             return new DefaultDnsQuestion(hostname, type);
         } catch (IllegalArgumentException e) {
             // java.net.IDN.toASCII(...) may throw an IllegalArgumentException if it fails to parse the hostname
-            if (traceEnabled) {
-                addTrace(e);
-            }
             return null;
         }
     }
 
-    private void addTrace(InetSocketAddress nameServerAddr, String msg) {
-        assert traceEnabled;
-
-        if (trace == null) {
-            trace = new StringBuilder(128);
-        }
-
-        trace.append(StringUtil.NEWLINE);
-        trace.append("\tfrom ");
-        trace.append(nameServerAddr);
-        trace.append(": ");
-        trace.append(msg);
-    }
-
-    private void addTrace(Throwable cause) {
-        assert traceEnabled;
-
-        if (trace == null) {
-            trace = new StringBuilder(128);
-        }
-
-        trace.append(StringUtil.NEWLINE);
-        trace.append("Caused by: ");
-        trace.append(cause);
-    }
-
     /**
      * Holds the closed DNS Servers for a domain.
      */

resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifeCycleObserverFactory.java

@@ -0,0 +1,45 @@
+/*
+ * 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 io.netty.util.internal.ObjectUtil.checkNotNull;
+
+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 = checkNotNull(logger, "logger");
+        this.level = checkNotNull(level, "level");
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver newDnsQueryLifecycleObserver(DnsQuestion question) {
+        return new TraceDnsQueryLifecycleObserver(question, logger, level);
+    }
+}

resolver-dns/src/main/java/io/netty/resolver/dns/TraceDnsQueryLifecycleObserver.java

@@ -0,0 +1,87 @@
+/*
+ * 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.channel.ChannelFuture;
+import io.netty.handler.codec.dns.DnsQuestion;
+import io.netty.handler.codec.dns.DnsResponseCode;
+import io.netty.util.internal.logging.InternalLogLevel;
+import io.netty.util.internal.logging.InternalLogger;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
+final class TraceDnsQueryLifecycleObserver implements DnsQueryLifecycleObserver {
+    private final InternalLogger logger;
+    private final InternalLogLevel level;
+    private final DnsQuestion question;
+    private InetSocketAddress dnsServerAddress;
+
+    TraceDnsQueryLifecycleObserver(DnsQuestion question, InternalLogger logger, InternalLogLevel level) {
+        this.question = checkNotNull(question, "question");
+        this.logger = checkNotNull(logger, "logger");
+        this.level = checkNotNull(level, "level");
+    }
+
+    @Override
+    public void queryWritten(InetSocketAddress dnsServerAddress, ChannelFuture future) {
+        this.dnsServerAddress = dnsServerAddress;
+    }
+
+    @Override
+    public void queryCancelled(int queriesRemaining) {
+        if (dnsServerAddress != null) {
+            logger.log(level, "from {} : {} cancelled with {} queries remaining", dnsServerAddress, question,
+                        queriesRemaining);
+        } else {
+            logger.log(level, "{} query never written and cancelled with {} queries remaining", question,
+                        queriesRemaining);
+        }
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryRedirected(List<InetSocketAddress> nameServers) {
+        logger.log(level, "from {} : {} redirected", dnsServerAddress, question);
+        return this;
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryCNAMEd(DnsQuestion cnameQuestion) {
+        logger.log(level, "from {} : {} CNAME question {}", dnsServerAddress, question, cnameQuestion);
+        return this;
+    }
+
+    @Override
+    public DnsQueryLifecycleObserver queryNoAnswer(DnsResponseCode code) {
+        logger.log(level, "from {} : {} no answer {}", dnsServerAddress, question, code);
+        return this;
+    }
+
+    @Override
+    public void queryFailed(Throwable cause) {
+        if (dnsServerAddress != null) {
+            logger.log(level, "from {} : {} failure", dnsServerAddress, question, cause);
+        } else {
+            logger.log(level, "{} query never written and failed", question, cause);
+        }
+    }
+
+    @Override
+    public void querySucceed() {
+    }
+}