From f447532b85af1d64889fe5406c218aa5feb69eb9 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Thu, 3 Aug 2023 21:02:33 +0200 Subject: [PATCH] Adjust DNS metrics (#89813) Adjust DNS metrics according to https://github.com/lmolkova/semantic-conventions/blob/dotnet8-metrics/docs/dotnet/dotnet-dns-metrics.md --- .../src/System/Net/Dns.cs | 22 ++++----- .../src/System/Net/NameResolutionMetrics.cs | 40 +++-------------- .../src/System/Net/NameResolutionTelemetry.cs | 45 +++++++++++++++---- .../tests/FunctionalTests/MetricsTest.cs | 18 ++++---- 4 files changed, 63 insertions(+), 62 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs b/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs index 46c1eff907955..dee9056aa3525 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs @@ -24,13 +24,13 @@ public static string GetHostName() { name = NameResolutionPal.GetHostName(); } - catch when (LogFailure(startingTimestamp)) + catch when (LogFailure(string.Empty, startingTimestamp)) { Debug.Fail("LogFailure should return false"); throw; } - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: true); + NameResolutionTelemetry.Log.AfterResolution(string.Empty, startingTimestamp, successful: true); if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, name); return name; @@ -395,13 +395,13 @@ private static object GetHostEntryOrAddressesCore(string hostName, bool justAddr Aliases = aliases }; } - catch when (LogFailure(startingTimestamp)) + catch when (LogFailure(hostName, startingTimestamp)) { Debug.Fail("LogFailure should return false"); throw; } - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: true); + NameResolutionTelemetry.Log.AfterResolution(hostName, startingTimestamp, successful: true); return result; } @@ -435,13 +435,13 @@ private static object GetHostEntryOrAddressesCore(IPAddress address, bool justAd } Debug.Assert(name != null); } - catch when (LogFailure(startingTimestamp)) + catch when (LogFailure(address, startingTimestamp)) { Debug.Fail("LogFailure should return false"); throw; } - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: true); + NameResolutionTelemetry.Log.AfterResolution(address, startingTimestamp, successful: true); // Do the forward lookup to get the IPs for that host name startingTimestamp = NameResolutionTelemetry.Log.BeforeResolution(name); @@ -465,13 +465,13 @@ private static object GetHostEntryOrAddressesCore(IPAddress address, bool justAd AddressList = addresses }; } - catch when (LogFailure(startingTimestamp)) + catch when (LogFailure(name, startingTimestamp)) { Debug.Fail("LogFailure should return false"); throw; } - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: true); + NameResolutionTelemetry.Log.AfterResolution(name, startingTimestamp, successful: true); // One of three things happened: // 1. Success. @@ -603,7 +603,7 @@ static async Task CompleteAsync(Task task, string hostName, long startingTime } finally { - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: result is not null); + NameResolutionTelemetry.Log.AfterResolution(hostName, startingTimestamp, successful: result is not null); } } } @@ -628,9 +628,9 @@ private static void ValidateHostName(string hostName) } } - private static bool LogFailure(long? startingTimestamp) + private static bool LogFailure(object hostNameOrAddress, long? startingTimestamp) { - NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: false); + NameResolutionTelemetry.Log.AfterResolution(hostNameOrAddress, startingTimestamp, successful: false); return false; } diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs index df1f5a2967421..180f492b3408e 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs @@ -12,42 +12,16 @@ internal static class NameResolutionMetrics { private static readonly Meter s_meter = new("System.Net.NameResolution"); - private static readonly Counter s_lookupsRequestedCounter = s_meter.CreateCounter( - name: "dns-lookups-requested", - description: "Number of DNS lookups requested."); + private static readonly Histogram s_lookupDuration = s_meter.CreateHistogram( + name: "dns.lookups.duration", + unit: "s", + description: "Measures the time taken to perform a DNS lookup."); - public static bool IsEnabled() => s_lookupsRequestedCounter.Enabled; + public static bool IsEnabled() => s_lookupDuration.Enabled; - public static void BeforeResolution(object hostNameOrAddress, out string? host) + public static void AfterResolution(TimeSpan duration, string hostName) { - if (s_lookupsRequestedCounter.Enabled) - { - host = GetHostnameFromStateObject(hostNameOrAddress); - - s_lookupsRequestedCounter.Add(1, KeyValuePair.Create("hostname", (object?)host)); - } - else - { - host = null; - } - } - - public static string GetHostnameFromStateObject(object hostNameOrAddress) - { - Debug.Assert(hostNameOrAddress is not null); - - string host = hostNameOrAddress switch - { - string h => h, - KeyValuePair t => t.Key, - IPAddress a => a.ToString(), - KeyValuePair t => t.Key.ToString(), - _ => null! - }; - - Debug.Assert(host is not null, $"Unknown hostNameOrAddress type: {hostNameOrAddress.GetType().Name}"); - - return host; + s_lookupDuration.Record(duration.TotalSeconds, KeyValuePair.Create("dns.question.name", (object?)hostName)); } } } diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs index 893659aea837b..ef43b59d15a13 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs @@ -1,8 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.Tracing; +using System.Net.Sockets; using System.Threading; namespace System.Net @@ -60,10 +62,6 @@ protected override void OnEventCommand(EventCommandEventArgs command) [NonEvent] public long BeforeResolution(object hostNameOrAddress) { - // System.Diagnostics.Metrics part - NameResolutionMetrics.BeforeResolution(hostNameOrAddress, out string? host); - - // System.Diagnostics.Tracing part if (IsEnabled()) { Interlocked.Increment(ref _lookupsRequested); @@ -71,7 +69,7 @@ public long BeforeResolution(object hostNameOrAddress) if (IsEnabled(EventLevel.Informational, EventKeywords.None)) { - host ??= NameResolutionMetrics.GetHostnameFromStateObject(hostNameOrAddress); + string host = GetHostnameFromStateObject(hostNameOrAddress); ResolutionStart(host); } @@ -79,19 +77,25 @@ public long BeforeResolution(object hostNameOrAddress) return Stopwatch.GetTimestamp(); } - return 0; + return NameResolutionMetrics.IsEnabled() ? Stopwatch.GetTimestamp() : 0; } [NonEvent] - public void AfterResolution(long? startingTimestamp, bool successful) + public void AfterResolution(object hostNameOrAddress, long? startingTimestamp, bool successful) { Debug.Assert(startingTimestamp.HasValue); + if (startingTimestamp == 0) + { + return; + } + + TimeSpan duration = Stopwatch.GetElapsedTime(startingTimestamp.Value); - if (startingTimestamp != 0) + if (IsEnabled()) { Interlocked.Decrement(ref _currentLookups); - _lookupsDuration?.WriteMetric(Stopwatch.GetElapsedTime(startingTimestamp.Value).TotalMilliseconds); + _lookupsDuration?.WriteMetric(duration.TotalMilliseconds); if (IsEnabled(EventLevel.Informational, EventKeywords.None)) { @@ -103,6 +107,29 @@ public void AfterResolution(long? startingTimestamp, bool successful) ResolutionStop(); } } + + if (NameResolutionMetrics.IsEnabled()) + { + NameResolutionMetrics.AfterResolution(duration, GetHostnameFromStateObject(hostNameOrAddress)); + } + } + + private static string GetHostnameFromStateObject(object hostNameOrAddress) + { + Debug.Assert(hostNameOrAddress is not null); + + string host = hostNameOrAddress switch + { + string h => h, + KeyValuePair t => t.Key, + IPAddress a => a.ToString(), + KeyValuePair t => t.Key.ToString(), + _ => null! + }; + + Debug.Assert(host is not null, $"Unknown hostNameOrAddress type: {hostNameOrAddress.GetType().Name}"); + + return host; } } } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs index 262626ce3108a..d3c0990dbf9c4 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs @@ -14,7 +14,7 @@ namespace System.Net.NameResolution.Tests { public class MetricsTest { - private const string DnsLookupsRequested = "dns-lookups-requested"; + private const string DnsLookupDuration = "dns.lookups.duration"; [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public static void ResolveValidHostName_MetricsRecorded() @@ -23,7 +23,7 @@ public static void ResolveValidHostName_MetricsRecorded() { const string ValidHostName = "localhost"; - using var recorder = new InstrumentRecorder(DnsLookupsRequested); + using var recorder = new InstrumentRecorder(DnsLookupDuration); await Dns.GetHostEntryAsync(ValidHostName); await Dns.GetHostAddressesAsync(ValidHostName); @@ -34,10 +34,10 @@ public static void ResolveValidHostName_MetricsRecorded() Dns.EndGetHostEntry(Dns.BeginGetHostEntry(ValidHostName, null, null)); Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(ValidHostName, null, null)); - long[] measurements = GetMeasurementsForHostname(recorder, ValidHostName); + double[] measurements = GetMeasurementsForHostname(recorder, ValidHostName); Assert.Equal(6, measurements.Length); - Assert.All(measurements, m => Assert.Equal(1, m)); + Assert.All(measurements, m => Assert.True(m > double.Epsilon)); }).Dispose(); } @@ -46,7 +46,7 @@ public static async Task ResolveInvalidHostName_MetricsRecorded() { const string InvalidHostName = $"invalid...example.com...{nameof(ResolveInvalidHostName_MetricsRecorded)}"; - using var recorder = new InstrumentRecorder(DnsLookupsRequested); + using var recorder = new InstrumentRecorder(DnsLookupDuration); await Assert.ThrowsAnyAsync(async () => await Dns.GetHostEntryAsync(InvalidHostName)); await Assert.ThrowsAnyAsync(async () => await Dns.GetHostAddressesAsync(InvalidHostName)); @@ -57,17 +57,17 @@ public static async Task ResolveInvalidHostName_MetricsRecorded() Assert.ThrowsAny(() => Dns.EndGetHostEntry(Dns.BeginGetHostEntry(InvalidHostName, null, null))); Assert.ThrowsAny(() => Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(InvalidHostName, null, null))); - long[] measurements = GetMeasurementsForHostname(recorder, InvalidHostName); + double[] measurements = GetMeasurementsForHostname(recorder, InvalidHostName); Assert.Equal(6, measurements.Length); - Assert.All(measurements, m => Assert.Equal(1, m)); + Assert.All(measurements, m => Assert.True(m > double.Epsilon)); } - private static long[] GetMeasurementsForHostname(InstrumentRecorder recorder, string hostname) + private static double[] GetMeasurementsForHostname(InstrumentRecorder recorder, string hostname) { return recorder .GetMeasurements() - .Where(m => m.Tags.ToArray().Any(t => t.Key == "hostname" && t.Value is string hostnameTag && hostnameTag == hostname)) + .Where(m => m.Tags.ToArray().Any(t => t.Key == "dns.question.name" && t.Value is string hostnameTag && hostnameTag == hostname)) .Select(m => m.Value) .ToArray(); }