add more specific metrics and logging for DNS lookup failures #2179

bjlaub · 2024-02-27T21:56:52Z

Before this PR

DNS lookups that result in an UnknownHostException are logged, but the exception message is logged unsafely which can make it hard to determine what the actual failure was. Under the hood, the error returned by getaddrinfo() is converted to a string via gai_strerror and used as the message for the generated UnknownHostException, which can be useful in determining what type of failure actually triggered the exception.

After this PR

Try to match strings generated by gai_strerror against the exception message, and log only the EAI_* error types and corresponding messages from the operating system safely. Additionally we report a meter metric with the error type (e.g. EAI_NONAME for when a DNS lookup fails because the name is not actually known to the nameserver).

==COMMIT_MSG==
add more specific metrics and logging for DNS lookup failures
==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-02-27T21:56:55Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

try to log error messages when getaddrinfo fails

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2024-02-27T22:03:16Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+            log.warn(
+                    "Unknown host '{}'",
+                    SafeArg.of("gaiErrorMessage", gaiError.getErrorMessage()),
+                    SafeArg.of("gaiErrorType", gaiError.getErrorType()),


Thoughts on adding a meter metric tagged with the errorType so we can keep an eye on things more broadly?

yeah i think that makes sense, will add

carterkozak · 2024-02-27T22:14:55Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+
+    private static ExtractedGaiError extractGaiErrorString(UnknownHostException exception) {
+        try {
+            for (Map.Entry<String, String> entry : EXPECTED_GAI_ERROR_STRINGS.entrySet()) {


Should we account for cached failures with an initial check that the message doesn't exactly match the input hostname value? (Perhaps return ImmutableExtractedGaiError.of("cached", "cached"); in that case)

carterkozak · 2024-02-27T23:58:41Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+            StackTraceElement[] trace = exception.getStackTrace();
+            if (trace.length > 0) {
+                StackTraceElement top = trace[0];
+                if ("java.net.InetAddress$CachedLookup".equals(top.getClassName())) {
+                    return ImmutableExtractedGaiError.of("cached", "cached");
+                }


I think the cached case failures can be checked most easily using Objects.equals(requestedHostname, exception.getMessage()) -- either way we should add a test for this.

carterkozak · 2024-02-27T23:59:18Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+                    return ImmutableExtractedGaiError.of("cached", "cached");
+                }
+
+                for (Map.Entry<String, String> entry : EXPECTED_GAI_ERROR_STRINGS.entrySet()) {


I don't think this loop needs to be predicated on if (trace.length > 0) {

carterkozak · 2024-02-27T23:59:51Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+        if (exception == null) {
+            return ImmutableExtractedGaiError.of("null", "null");
+        }


We probably don't need to handle null here since we're calling from our own code on catch (UnknownHostException

carterkozak · 2024-02-28T13:49:02Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+    @Value.Immutable
+    interface ExtractedGaiError {


I think we can combine the map and type here into a single enum e.g.

enum DNS_ERROR { EAI_ADDRFAMILY("Address family for hostname not supported"), EAI_AGAIN("Temporary failure in name resolution"), /*etc*/ CACHED(), // explicitly avoid setting a substring matcher for 'cached' and 'unknown' special cases UNKNOWN() }

carterkozak · 2024-02-28T17:51:33Z

dialogue-clients/metrics.md

@@ -68,6 +68,8 @@ Dialogue DNS metrics.
    - `success`: DNS resolution succeeded using `InetAddress.getAllByName`.
    - `fallback`: DNS resolution using the primary mechanism failed, however addresses were available in the fallback cache.
    - `failure`: No addresses could be resolved for the given hostname.
+- `client.dns.lookupError` (meter): DNS resolver query failures.


let's match the failure wording above, perhaps naming this client.dns.failure

carterkozak · 2024-02-28T17:52:52Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

+        }
+    }
+
+    private static GaiError extractGaiErrorString(UnknownHostException exception) {


perhaps extractGaiError since this returns GaiError, not String

carterkozak · 2024-02-28T17:55:57Z

...ogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java

+        TaggedMetricRegistry registry = new DefaultTaggedMetricRegistry();
+        DialogueDnsResolver resolver = new DefaultDialogueDnsResolver(registry);
+
+        String badHost = "alksdjflajsdlkfjalksjflkadjsf.com";


Anyone could pay ~$10 to break this test.

Let's use something like:

String badHost = UUID.randomUUID() + ".palantir.com";

I think that only went into one of the two tests using this pattern, we need to update both

…ached negative lookup

carterkozak · 2024-02-28T17:59:51Z

...ogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java

+        ImmutableSet<InetAddress> result2 = resolver.resolve(badHost);
+        assertThat(result2).isEmpty();
+        ClientDnsMetrics metrics = ClientDnsMetrics.of(registry);
+        assertThat(metrics.lookupError("CACHED").getCount()).isGreaterThan(0);


carterkozak · 2024-02-28T18:00:25Z

...ogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java

+        String badHost = "alksdjflajsdlkfjalksjflkadjsf.com";
+        ImmutableSet<InetAddress> result = resolver.resolve(badHost);
+
+        assertThat(result).isEmpty();


let's add assertThat(metrics.lookupError("EAI_NONAME").getCount()).isEqualTo(1); in addition to this assert

carterkozak · 2024-02-28T18:05:46Z

...ogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java

+        // should resolve from cache
+        ImmutableSet<InetAddress> result2 = resolver.resolve(badHost);
+        assertThat(result2).isEmpty();
+        assertThat(metrics.failure("CACHED").getCount()).isGreaterThan(0);


.isEqualTo(1);

carterkozak · 2024-02-28T18:08:34Z

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolver.java

-            log.warn("Unknown host '{}'", UnsafeArg.of("hostname", hostname), e);
+            GaiError gaiError = extractGaiError(e, hostname);
+            log.warn(
+                    "Unknown host '{}'",


Suggested change

"Unknown host '{}'",

"Unknown host '{}'. {}: {}",

svc-autorelease · 2024-02-28T18:48:49Z

Released 3.117.0

bjlaub requested a review from carterkozak February 27, 2024 21:56

carterkozak reviewed Feb 27, 2024

View reviewed changes

carterkozak reviewed Feb 28, 2024

View reviewed changes

Brian Laub added 7 commits February 28, 2024 11:06

try to log error messages when getaddrinfo fails

8b07beb

treat UnknownHostException from cache separately

9fa92ee

use an enum for GaiError

36f6502

remove unnecessary null check

92a2d79

refactor a bit

79056c4

more changes

377e143

add metrics and tests

ca9e483

bjlaub force-pushed the blaub/log-getaddrinfo-error-strings branch from 292e6a9 to ca9e483 Compare February 28, 2024 17:43

carterkozak reviewed Feb 28, 2024

View reviewed changes

Brian Laub added 2 commits February 28, 2024 12:56

add missing change

4ecfd6c

match hostname directly against exception message when checking for c…

e1895df

…ached negative lookup

carterkozak reviewed Feb 28, 2024

View reviewed changes

fixes

8fb4963

bjlaub changed the title ~~try to log error messages when getaddrinfo fails~~ add metrics and logging for DNS lookup failures Feb 28, 2024

bjlaub changed the title ~~add metrics and logging for DNS lookup failures~~ add more specific metrics and logging for DNS lookup failures Feb 28, 2024

carterkozak reviewed Feb 28, 2024

View reviewed changes

Brian Laub added 2 commits February 28, 2024 13:06

fix test

57fa8d8

fix test again

b7590d2

carterkozak reviewed Feb 28, 2024

View reviewed changes

Brian Laub and others added 2 commits February 28, 2024 13:11

fix log formatting

71c251f

Add generated changelog entries

c48c3d7

carterkozak approved these changes Feb 28, 2024

View reviewed changes

carterkozak added merge when ready autorelease labels Feb 28, 2024

bulldozer-bot bot merged commit 409307a into develop Feb 28, 2024
6 checks passed

bulldozer-bot bot deleted the blaub/log-getaddrinfo-error-strings branch February 28, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more specific metrics and logging for DNS lookup failures #2179

add more specific metrics and logging for DNS lookup failures #2179

bjlaub commented Feb 27, 2024 •

edited

Loading

changelog-app bot commented Feb 27, 2024 •

edited by carterkozak

Loading

carterkozak Feb 27, 2024

bjlaub Feb 27, 2024

carterkozak Feb 27, 2024 •

edited

Loading

carterkozak Feb 27, 2024

carterkozak Feb 27, 2024

carterkozak Feb 27, 2024

carterkozak Feb 28, 2024

carterkozak Feb 28, 2024

bjlaub Feb 28, 2024

carterkozak Feb 28, 2024

bjlaub Feb 28, 2024

carterkozak Feb 28, 2024

bjlaub Feb 28, 2024

carterkozak Feb 28, 2024

carterkozak Feb 28, 2024

bjlaub Feb 28, 2024

carterkozak Feb 28, 2024

bjlaub Feb 28, 2024

carterkozak Feb 28, 2024

carterkozak Feb 28, 2024

svc-autorelease commented Feb 28, 2024

add more specific metrics and logging for DNS lookup failures #2179

add more specific metrics and logging for DNS lookup failures #2179

Conversation

bjlaub commented Feb 27, 2024 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Feb 27, 2024 • edited by carterkozak Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-autorelease commented Feb 28, 2024

bjlaub commented Feb 27, 2024 •

edited

Loading

changelog-app bot commented Feb 27, 2024 •

edited by carterkozak

Loading

Generate changelog in `changelog/@unreleased`

carterkozak Feb 27, 2024 •

edited

Loading