dns.lookup.duration should report error.type

[lmolkova]

Description

Currently dns.lookup.duration only reports duration of he successful lookups, but failed lookups are quite interesting as well (count, rate, duration, failure reason).

Having error.type attribute on the dns.lookup.duration metric and reporting it when DNS lookup fails would provide all the details.

Adding an attribute later is considered a breaking change on OTel, since it increases the number of time series in the metric and breaks alerts and dashboards (so creating it as a bug).

Reproduction Steps

N/A

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

Line 24 in e89daba

s_lookupDuration.Record(duration.TotalSeconds, KeyValuePair.Create("dns.question.name", (object?)hostName));

Expected behavior

The metric should have error.type attribute nad report duration of both, failed and successfully lookups.

Actual behavior

Metric only reports successful lookups

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[ghost]

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

Issue Details

---

Description

Currently dns.lookup.duration only reports duration of he successful lookups, but failed lookups are quite interesting as well (count, rate, duration, failure reason).

Having error.type attribute on the dns.lookup.duration metric and reporting it when DNS lookup fails would provide all the details.

Adding an attribute later is considered a breaking change on OTel, since it increases the number of time series in the metric and breaks alerts and dashboards (so creating it as a bug).

Reproduction Steps

N/A

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

Line 24 in e89daba

s_lookupDuration.Record(duration.TotalSeconds, KeyValuePair.Create("dns.question.name", (object?)hostName));

Expected behavior

The metric should have error.type attribute nad report duration of both, failed and successfully lookups.

Actual behavior

Metric only reports successful lookups

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	lmolkova
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[MihaZupan]

Currently dns.lookup.duration only reports duration of he successful lookups

Where are you seeing that? We're using the same code for both success & failure cases, and have tests for the failure case here

runtime/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs

Line 45 in d1972fe

public static async Task ResolveInvalidHostName_MetricsRecorded()

Having error.type attribute on the dns.lookup.duration metric and reporting it when DNS lookup fails would provide all the details.

What sort of information would you expect to see there (we may have a limited amount of info from the OS)?

[lmolkova]

Where are you seeing that? We're using the same code for both success & failure cases, and have tests for the failure case here

Thanks for the clarification! So it records all durations regardless of the result - I didn't realize that.

In this case, users are just not able to separate successful attempts from failed ones.
I'd like to see an exception type captured in error.type (or, if there is any enum-like list of DNS errors this would also be a great option).

[antonfirsov]

@karelz after discussing with @noahfalk and @lmolkova, we think the addition of error.type is something we should get into 8.0:

• Attribute additions are breaking changes according to otel, making the negative impact higher if we add it in future releases.
• It's easy / low risk to implement

[antonfirsov]

Linking the related aspnet issue: dotnet/aspnetcore#51029

[MihaZupan]

Attribute additions are breaking changes according to otel, making the negative impact higher if we add it in future releases

To what degree are changes to emitted values for existing tags considered breaking?
E.g. if we had a managed implementation of DNS, would we be free to report different information, or would we want to mimic the values we'll end up using here?

[antonfirsov]

Adding attributes may break existing Prometheus queries. I hope changes in the query result because of different values in the attributes are not considered to be breaking. @lmolkova do you have a comprehensive answer to this?

[lmolkova]

Imagine someone writes the following query:

avg(rate(dns_query_duration_count{question="foo.io"}[1m]))

At first, there was no error.type attribute and then this query returned an average number of DNS lookups per minute to foo.io.

Let's say we do ~100 of them: 90 are fine and 10 fail.

Now, we add error.type attribute.
From what used to be 1 time series (mind other attributes), we now have two of them:

• dns_query_duration_count{error_type="fail"} has avg rate of 10
• dns_query_duration_count{error_type=""} (or no error_type) has avg rate of 90

The average of 2 of them is 50.

I.e. we went from an average 100 to an average 50 by adding a new attribute. If someone set up an alert on this, then this alert is no longer valid. Users would also be surprised about a new range of this value.

Here's how the graph looks like (drop corresponds to adding new attribute)

and this is individual time series

So, certain queries and operators are sensitive to new attribute addition, but in some cases we can tolerate/justify it.
E.g. if we now only reported successful lookups, adding error.type would probably not be breaking since we'd start to report additional time series about things that didn't happen before.

Instead of justifying this change and inevitably shuffling things around in the future, we'd have much easier time updating it now.

[lmolkova]

To what degree are changes to emitted values for existing tags considered breaking?

TL;DR
New/different values of tags are mostly fine (unless you change units or do other drastic changes), but

1. adding new tags (i.e. new time series) is definitely a problem
2. breaking down tag value into multiple ones or grouping multiple values into one would be similar to adding a new tag (changes a number of time series).
3. renaming tag value can be breaking (e.g. if someone filters by this specific value).
4. adding a new tag value that has never been reported before (a new class of errors that never happened) should be fine

So we should in general avoid 1-3, but 2-3 should be less impactful than 1.

[antonfirsov]

Fixed by #93414 in release/8.0, and by #93254 in main (9.0).