Adjust the DNS lookup duration metric #93254

antonfirsov · 2023-10-09T19:53:22Z

This is a necessary change to ensure the metric is in sync with the spec and the OTel recommendations. An 8.0 backport is planned to avoid future breaking changes.

ghost · 2023-10-09T19:53:34Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Rename the metric to dns.lookup.duration -- Rename dns.lookupS.duration to dns.lookup.duration #92917
Report error.type -- dns.lookup.duration should report error.type #92837

This is a necessary change to ensure the metric is in sync with the spec and the OTel recommendations. An 8.0 backport is planned to avoid future breaking changes.

Author:	antonfirsov
Assignees:	antonfirsov
Labels:	`area-System.Net`
Milestone:	-

antonfirsov · 2023-10-09T20:29:19Z

src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs

+        private static string GetErrorType(Exception exception) => (exception as SocketException)?.SocketErrorCode switch
+        {
+            SocketError.HostNotFound => "host_not_found",
+            SocketError.TryAgain => "try_again",
+            SocketError.AddressFamilyNotSupported => "address_family_not_supported",
+            SocketError.NoRecovery => "no_recovery",
+
+            _ => exception.GetType().Name
+        };


I picked the codes we can realistically get from the PAL by looking at our mapping in the Unix PAL, and getaddrinfo return values on Windows.
There are some other codes that may theoretically occur, but I wouldn't expect to see them in practice. Ie. seeing InvalidArgument , TypeNotFound or SocketNotSupported means a programming error in our PAL rather than anything else if I'm not mistaken.

src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs

JamesNK

Change looks good. Details about the error attribute need to be added to dotnet/docs#37213 and/or the OTEL semantic conventions

stephentoub · 2023-10-10T12:28:21Z

src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs

                }
            }
        }

+        private static string GetErrorType(Exception exception) => (exception as SocketException)?.SocketErrorCode switch


These strings are specifically for metrics, right? Please add a comment on the method to that effect. Where do these result strings come from? Are they strings dictated by otel, or are you just snake_casing the names from the enum?

These strings are specifically for metrics, right?

Yes.

Where do these result strings come from?

They are defined by us, by mapping enum values to snake-cased strings which is an OTel convention. We do the same in HTTP metrics.

which is an OTel convention

Why don't we do the same for the _ => case then? i.e. it seems like our general goal here is to take any SocketError value or Exception name, and snake case it, and we're then just optimizing a few cases that we expect to be most common by hardcoding the transformation. In the name of robustness, shouldn't we be dynamically creating the snak_cased version of the resulting SocketError if there is one (outside of the range we've hardcoded) or for the exception type?

As noticed in #93254 (comment), this should be actually the full name of the exception, I will correct it.

Why don't we do the same for the _ => case then?

The specs don't require us to camel-case the exception names (would be very weird with full names). Tagging @lmolkova for a comprehensive explanation.

dynamically creating the snak_cased version of the resulting SocketError

The SocketError-s that can practically appear here are coming from a limited set. I picked the codes we can realistically get from the PAL by looking at our mapping in the Unix PAL, and getaddrinfo return values on Windows.

And when that mapping is augmented in the future, we're going to remember to update this random method as well?

And when that mapping is augmented in the future

That would only happen if getaddrinfo implementations would introduce new error codes which I find very unlikely. Implementing dynamic mapping here to prematurely solve that problem feels like overengineering. Users would see the value host_not_found in 99+% of the practical cases, sometimes try_again maybe.

If you think the mapping is problematic in the form I'm proposing, I would rather prefer to remove it, and just report the exception name.

it seems like our general goal here is to take any SocketError value or Exception name, and snake case it

I'd frame the goal as disambiguating the most common sources of error that users would appreciate not having grouped together when analyzing failures. I don't think it is a goal that every possible SocketError has been disambiguated, that new future SocketError values are automatically disambiguated, or that exception names are transformed into snake case. The hard-coded list Anton has here seems fine to me. If we ever changed it we'd probably want that to be an intentional choice so we can warn users that their queries might need to be updated.

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs

noahfalk

LGTM. I commented on the other issue that I do think FullName would be preferable for exception types for consistency across other .NET metrics.

antonfirsov · 2023-10-12T16:32:38Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2023-10-12T16:32:56Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2023-10-12T19:33:59Z

CI failures are unrelated (eg. #93413).

This reverts commit 946c524.

lewing · 2023-10-12T21:47:50Z

I opened #93418 to revert this because it broke the build

karelz · 2023-10-13T12:08:39Z

Looks like it didn't break the build after all - see explanation in #93418 (comment)

…istency (#93414) * Rename `server.socket.address` to `network.peer.address` (#93255) * Report Exception.FullName for http.client.request.duration/error.type (#93322) * Adjust the DNS lookup duration metric (#93254)

antonfirsov added 2 commits October 5, 2023 16:41

rename duration metric

3915c95

introduce error.type

3915202

antonfirsov requested a review from MihaZupan October 9, 2023 19:53

ghost assigned antonfirsov Oct 9, 2023

dotnet-issue-labeler bot added the area-System.Net label Oct 9, 2023

antonfirsov commented Oct 9, 2023

View reviewed changes

MihaZupan approved these changes Oct 9, 2023

View reviewed changes

src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs Outdated Show resolved Hide resolved

JamesNK approved these changes Oct 10, 2023

View reviewed changes

stephentoub reviewed Oct 10, 2023

View reviewed changes

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs Outdated Show resolved Hide resolved

Move error type mapping to NameResolutionMetrics

5648adb

karelz added this to the 9.0.0 milestone Oct 10, 2023

JamesNK reviewed Oct 10, 2023

View reviewed changes

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs Outdated Show resolved Hide resolved

karelz approved these changes Oct 10, 2023

View reviewed changes

build-analysis bot mentioned this pull request Oct 10, 2023

illink - Task host node exited prematurely #92951

Closed

This was referenced Oct 10, 2023

[Metrics] error.type doesn't report Exception.FullName #93302

Closed

Add a reference for new .NET 8 metrics dotnet/docs#37213

Merged

noahfalk approved these changes Oct 10, 2023

View reviewed changes

antonfirsov added 2 commits October 11, 2023 03:06

make error.type=Exception.FullName in the general case

473af1b

fix indentation

9a84581

antonfirsov merged commit 946c524 into dotnet:main Oct 12, 2023

build-analysis bot mentioned this pull request Oct 12, 2023

System.ComponentModel.Win32Exception : An error occurred trying to start process 'notepad.exe' with working directory 'C:\h\w\9F8D0903\w\B7E309C0\e'. Access is denied #90406

Open

3 tasks

antonfirsov added a commit to antonfirsov/runtime that referenced this pull request Oct 12, 2023

Adjust the DNS lookup duration metric (dotnet#93254)

97c03ae

antonfirsov mentioned this pull request Oct 12, 2023

[release/8.0] Adjust System.Net metrics for OTel conformance and consistency #93414

Merged

lewing added a commit that referenced this pull request Oct 12, 2023

Revert "Adjust the DNS lookup duration metric (#93254)"

eb17eb1

This reverts commit 946c524.

lewing mentioned this pull request Oct 12, 2023

Revert "Adjust the DNS lookup duration metric (#93254)" #93418

Closed

This was referenced Oct 16, 2023

Rename dns.lookupS.duration to dns.lookup.duration #92917

Closed

dns.lookup.duration should report error.type #92837

Closed

ghost locked as resolved and limited conversation to collaborators Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust the DNS lookup duration metric #93254

Adjust the DNS lookup duration metric #93254

antonfirsov commented Oct 9, 2023 •

edited

Loading

ghost commented Oct 9, 2023

antonfirsov Oct 9, 2023 •

edited

Loading

JamesNK left a comment

stephentoub Oct 10, 2023

antonfirsov Oct 10, 2023

stephentoub Oct 10, 2023

antonfirsov Oct 10, 2023 •

edited

Loading

stephentoub Oct 10, 2023

antonfirsov Oct 10, 2023 •

edited

Loading

noahfalk Oct 10, 2023

noahfalk left a comment

antonfirsov commented Oct 12, 2023

azure-pipelines bot commented Oct 12, 2023

antonfirsov commented Oct 12, 2023

lewing commented Oct 12, 2023

karelz commented Oct 13, 2023

Adjust the DNS lookup duration metric #93254

Adjust the DNS lookup duration metric #93254

Conversation

antonfirsov commented Oct 9, 2023 • edited Loading

ghost commented Oct 9, 2023

antonfirsov Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

JamesNK left a comment

Choose a reason for hiding this comment

stephentoub Oct 10, 2023

Choose a reason for hiding this comment

antonfirsov Oct 10, 2023

Choose a reason for hiding this comment

stephentoub Oct 10, 2023

Choose a reason for hiding this comment

antonfirsov Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

stephentoub Oct 10, 2023

Choose a reason for hiding this comment

antonfirsov Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

noahfalk Oct 10, 2023

Choose a reason for hiding this comment

noahfalk left a comment

Choose a reason for hiding this comment

antonfirsov commented Oct 12, 2023

azure-pipelines bot commented Oct 12, 2023

antonfirsov commented Oct 12, 2023

lewing commented Oct 12, 2023

karelz commented Oct 13, 2023

antonfirsov commented Oct 9, 2023 •

edited

Loading

antonfirsov Oct 9, 2023 •

edited

Loading

antonfirsov Oct 10, 2023 •

edited

Loading

antonfirsov Oct 10, 2023 •

edited

Loading