From b94dd5a36d945b0ea4cc39f704a9a102373b350d Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 12 Jul 2023 21:06:30 +0200 Subject: [PATCH 1/5] Add System.Net.NameResolution metrics --- .../src/System.Net.NameResolution.csproj | 2 + .../src/System/Net/Dns.cs | 3 +- .../src/System/Net/NameResolutionMetrics.cs | 53 +++++++++++ .../src/System/Net/NameResolutionTelemetry.cs | 22 +---- .../tests/FunctionalTests/MetricsTest.cs | 94 +++++++++++++++++++ ...Net.NameResolution.Functional.Tests.csproj | 1 + .../tests/FunctionalTests/TelemetryTest.cs | 2 - 7 files changed, 156 insertions(+), 21 deletions(-) create mode 100644 src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs create mode 100644 src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs diff --git a/src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj b/src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj index 254be8e59c0f5..e064468578801 100644 --- a/src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj +++ b/src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj @@ -13,6 +13,7 @@ + + 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 0ee39a39e7402..d64896a91c8f3 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs @@ -535,12 +535,11 @@ private static Task GetHostEntryOrAddressesCoreAsync(string hostName, bool justR ValidateHostName(hostName); Task? t; - if (NameResolutionTelemetry.Log.IsEnabled()) + if (NameResolutionTelemetry.Log.IsEnabled() || NameResolutionMetrics.IsEnabled()) { t = justAddresses ? GetAddrInfoWithTelemetryAsync(hostName, justAddresses, family, cancellationToken) : GetAddrInfoWithTelemetryAsync(hostName, justAddresses, family, cancellationToken); - } else { diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs new file mode 100644 index 0000000000000..df1f5a2967421 --- /dev/null +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs @@ -0,0 +1,53 @@ +// 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.Metrics; +using System.Net.Sockets; + +namespace System.Net +{ + 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."); + + public static bool IsEnabled() => s_lookupsRequestedCounter.Enabled; + + public static void BeforeResolution(object hostNameOrAddress, out string? host) + { + 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; + } + } +} 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 eef4b0af8a869..8f17241acd25d 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs @@ -1,10 +1,8 @@ // 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 @@ -62,14 +60,10 @@ protected override void OnEventCommand(EventCommandEventArgs command) [NonEvent] public long BeforeResolution(object hostNameOrAddress) { - Debug.Assert(hostNameOrAddress != null); - Debug.Assert( - hostNameOrAddress is string || - hostNameOrAddress is IPAddress || - hostNameOrAddress is KeyValuePair || - hostNameOrAddress is KeyValuePair, - $"Unknown hostNameOrAddress type: {hostNameOrAddress.GetType().Name}"); + // System.Diagnostics.Metrics part + NameResolutionMetrics.BeforeResolution(hostNameOrAddress, out string? host); + // System.Diagnostics.Tracing part if (IsEnabled()) { Interlocked.Increment(ref _lookupsRequested); @@ -77,14 +71,8 @@ hostNameOrAddress is KeyValuePair || if (IsEnabled(EventLevel.Informational, EventKeywords.None)) { - string host = hostNameOrAddress switch - { - string h => h, - KeyValuePair t => t.Key, - IPAddress a => a.ToString(), - KeyValuePair t => t.Key.ToString(), - _ => null! - }; + host ??= NameResolutionMetrics.GetHostnameFromStateObject(hostNameOrAddress); + ResolutionStart(host); } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs new file mode 100644 index 0000000000000..746f61fcd6710 --- /dev/null +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs @@ -0,0 +1,94 @@ +// 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.Metrics; +using System.Linq; +using System.Net.Sockets; +using System.Threading.Tasks; +using Xunit; + +namespace System.Net.NameResolution.Tests +{ + public class MetricsTest + { + private const string DnsLookupsRequested = "dns-lookups-requested"; + + [Fact] + public static async Task ResolveValidHostName_MetricsRecorded() + { + const string ValidHostName = "localhost"; + + using var recorder = new InstrumentRecorder(DnsLookupsRequested); + + await Dns.GetHostEntryAsync(ValidHostName); + await Dns.GetHostAddressesAsync(ValidHostName); + + Dns.GetHostEntry(ValidHostName); + Dns.GetHostAddresses(ValidHostName); + + Dns.EndGetHostEntry(Dns.BeginGetHostEntry(ValidHostName, null, null)); + Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(ValidHostName, null, null)); + + long[] measurements = GetMeasurementsForHostname(recorder, ValidHostName); + + // >= 6 because other tests running in parallel may have also resolved the same host. + Assert.True(measurements.Length >= 6, $"Was {measurements.Length}"); + Assert.All(measurements, m => Assert.Equal(1, m)); + } + + [Fact] + public static async Task ResolveInvalidHostName_MetricsRecorded() + { + const string InvalidHostName = $"invalid...example.com...{nameof(ResolveInvalidHostName_MetricsRecorded)}"; + + using var recorder = new InstrumentRecorder(DnsLookupsRequested); + + await Assert.ThrowsAnyAsync(async () => await Dns.GetHostEntryAsync(InvalidHostName)); + await Assert.ThrowsAnyAsync(async () => await Dns.GetHostAddressesAsync(InvalidHostName)); + + Assert.ThrowsAny(() => Dns.GetHostEntry(InvalidHostName)); + Assert.ThrowsAny(() => Dns.GetHostAddresses(InvalidHostName)); + + Assert.ThrowsAny(() => Dns.EndGetHostEntry(Dns.BeginGetHostEntry(InvalidHostName, null, null))); + Assert.ThrowsAny(() => Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(InvalidHostName, null, null))); + + long[] measurements = GetMeasurementsForHostname(recorder, InvalidHostName); + + Assert.Equal(6, measurements.Length); + Assert.All(measurements, m => Assert.Equal(1, m)); + } + + private static long[] 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)) + .Select(m => m.Value) + .ToArray(); + } + + private sealed class InstrumentRecorder : IDisposable where T : struct + { + private readonly MeterListener _meterListener = new(); + private readonly List> _values = new(); + + public InstrumentRecorder(string instrumentName) + { + _meterListener.InstrumentPublished = (instrument, listener) => + { + if (instrument.Meter.Name == "System.Net.NameResolution" && instrument.Name == instrumentName) + { + listener.EnableMeasurementEvents(instrument); + } + }; + _meterListener.SetMeasurementEventCallback(OnMeasurementRecorded); + _meterListener.Start(); + } + + private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) => _values.Add(new Measurement(measurement, tags)); + public IReadOnlyList> GetMeasurements() => _values.ToArray(); + public void Dispose() => _meterListener.Dispose(); + } + } +} diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj index b70d26344c89b..402868113d600 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj @@ -10,6 +10,7 @@ + diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/TelemetryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/TelemetryTest.cs index e42b3d9a7129f..c11359154514c 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/TelemetryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/TelemetryTest.cs @@ -8,9 +8,7 @@ using System.Net.Sockets; using System.Threading.Tasks; using Microsoft.DotNet.RemoteExecutor; -using Microsoft.DotNet.XUnitExtensions; using Xunit; -using Xunit.Sdk; namespace System.Net.NameResolution.Tests { From c46040a5cd5f2b8cb0ac411d56d9fa9a4b953f09 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Thu, 13 Jul 2023 13:35:39 +0200 Subject: [PATCH 2/5] Avoid potential race condition in test --- .../tests/FunctionalTests/MetricsTest.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs index 746f61fcd6710..78645b312d116 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs @@ -1,6 +1,7 @@ // 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.Concurrent; using System.Collections.Generic; using System.Diagnostics.Metrics; using System.Linq; @@ -71,7 +72,7 @@ private static long[] GetMeasurementsForHostname(InstrumentRecorder record private sealed class InstrumentRecorder : IDisposable where T : struct { private readonly MeterListener _meterListener = new(); - private readonly List> _values = new(); + private readonly ConcurrentQueue> _values = new(); public InstrumentRecorder(string instrumentName) { @@ -86,7 +87,7 @@ public InstrumentRecorder(string instrumentName) _meterListener.Start(); } - private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) => _values.Add(new Measurement(measurement, tags)); + private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) => _values.Enqueue(new Measurement(measurement, tags)); public IReadOnlyList> GetMeasurements() => _values.ToArray(); public void Dispose() => _meterListener.Dispose(); } From 5ae7ed587d3c91ccfa7b7b6e497e1822899e98cf Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Thu, 13 Jul 2023 15:02:51 +0200 Subject: [PATCH 3/5] Fix metrics counter firing twice when called from RunAsync without telemetry --- .../src/System/Net/Dns.cs | 32 ++++++++----------- .../src/System/Net/NameResolutionTelemetry.cs | 6 ++-- 2 files changed, 17 insertions(+), 21 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 d64896a91c8f3..46c1eff907955 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs @@ -156,8 +156,8 @@ public static Task GetHostEntryAsync(IPAddress address) throw new ArgumentException(SR.net_invalid_ip_addr, nameof(address)); } - return RunAsync(static (s, stopwatch) => { - IPHostEntry ipHostEntry = GetHostEntryCore((IPAddress)s, AddressFamily.Unspecified, stopwatch); + return RunAsync(static (s, startingTimestamp) => { + IPHostEntry ipHostEntry = GetHostEntryCore((IPAddress)s, AddressFamily.Unspecified, startingTimestamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Info((IPAddress)s, $"{ipHostEntry} with {ipHostEntry.AddressList.Length} entries"); return ipHostEntry; }, address, CancellationToken.None); @@ -362,20 +362,18 @@ public static IPHostEntry EndResolve(IAsyncResult asyncResult) return ipHostEntry; } - private static IPHostEntry GetHostEntryCore(string hostName, AddressFamily addressFamily, long startingTimestamp = 0) => + private static IPHostEntry GetHostEntryCore(string hostName, AddressFamily addressFamily, long? startingTimestamp = null) => (IPHostEntry)GetHostEntryOrAddressesCore(hostName, justAddresses: false, addressFamily, startingTimestamp); - private static IPAddress[] GetHostAddressesCore(string hostName, AddressFamily addressFamily, long startingTimestamp = 0) => + private static IPAddress[] GetHostAddressesCore(string hostName, AddressFamily addressFamily, long? startingTimestamp = null) => (IPAddress[])GetHostEntryOrAddressesCore(hostName, justAddresses: true, addressFamily, startingTimestamp); - private static object GetHostEntryOrAddressesCore(string hostName, bool justAddresses, AddressFamily addressFamily, long startingTimestamp = 0) + private static object GetHostEntryOrAddressesCore(string hostName, bool justAddresses, AddressFamily addressFamily, long? startingTimestamp = null) { ValidateHostName(hostName); - if (startingTimestamp == 0) - { - startingTimestamp = NameResolutionTelemetry.Log.BeforeResolution(hostName); - } + // startingTimestamp may have already been set if we're being called from RunAsync. + startingTimestamp ??= NameResolutionTelemetry.Log.BeforeResolution(hostName); object result; try @@ -408,24 +406,22 @@ private static object GetHostEntryOrAddressesCore(string hostName, bool justAddr return result; } - private static IPHostEntry GetHostEntryCore(IPAddress address, AddressFamily addressFamily, long startingTimestamp = 0) => + private static IPHostEntry GetHostEntryCore(IPAddress address, AddressFamily addressFamily, long? startingTimestamp = null) => (IPHostEntry)GetHostEntryOrAddressesCore(address, justAddresses: false, addressFamily, startingTimestamp); - private static IPAddress[] GetHostAddressesCore(IPAddress address, AddressFamily addressFamily, long startingTimestamp) => + private static IPAddress[] GetHostAddressesCore(IPAddress address, AddressFamily addressFamily, long? startingTimestamp = null) => (IPAddress[])GetHostEntryOrAddressesCore(address, justAddresses: true, addressFamily, startingTimestamp); // Does internal IPAddress reverse and then forward lookups (for Legacy and current public methods). - private static object GetHostEntryOrAddressesCore(IPAddress address, bool justAddresses, AddressFamily addressFamily, long startingTimestamp) + private static object GetHostEntryOrAddressesCore(IPAddress address, bool justAddresses, AddressFamily addressFamily, long? startingTimestamp = null) { // Try to get the data for the host from its address. // We need to call getnameinfo first, because getaddrinfo w/ the ipaddress string // will only return that address and not the full list. // Do a reverse lookup to get the host name. - if (startingTimestamp == 0) - { - startingTimestamp = NameResolutionTelemetry.Log.BeforeResolution(address); - } + // startingTimestamp may have already been set if we're being called from RunAsync. + startingTimestamp ??= NameResolutionTelemetry.Log.BeforeResolution(address); SocketError errorCode; string? name; @@ -598,7 +594,7 @@ private static Task GetHostEntryOrAddressesCoreAsync(string hostName, bool justR static async Task CompleteAsync(Task task, string hostName, long startingTimestamp) { - _ = NameResolutionTelemetry.Log.BeforeResolution(hostName); + _ = NameResolutionTelemetry.Log.BeforeResolution(hostName); T? result = null; try { @@ -632,7 +628,7 @@ private static void ValidateHostName(string hostName) } } - private static bool LogFailure(long startingTimestamp) + private static bool LogFailure(long? startingTimestamp) { NameResolutionTelemetry.Log.AfterResolution(startingTimestamp, successful: false); return false; 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 8f17241acd25d..21903d0e28716 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs @@ -83,13 +83,13 @@ public long BeforeResolution(object hostNameOrAddress) } [NonEvent] - public void AfterResolution(long startingTimestamp, bool successful) + public void AfterResolution(long? startingTimestamp, bool successful) { - if (startingTimestamp != 0) + if (startingTimestamp.HasValue && startingTimestamp != 0) { Interlocked.Decrement(ref _currentLookups); - _lookupsDuration?.WriteMetric(Stopwatch.GetElapsedTime(startingTimestamp).TotalMilliseconds); + _lookupsDuration?.WriteMetric(Stopwatch.GetElapsedTime(startingTimestamp.Value).TotalMilliseconds); if (IsEnabled(EventLevel.Informational, EventKeywords.None)) { From 60981e01fb0478d5816b4f45b01a0e3412fa4a46 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Thu, 13 Jul 2023 15:48:10 +0200 Subject: [PATCH 4/5] Add an extra assert --- .../src/System/Net/NameResolutionTelemetry.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 21903d0e28716..893659aea837b 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs @@ -85,7 +85,9 @@ public long BeforeResolution(object hostNameOrAddress) [NonEvent] public void AfterResolution(long? startingTimestamp, bool successful) { - if (startingTimestamp.HasValue && startingTimestamp != 0) + Debug.Assert(startingTimestamp.HasValue); + + if (startingTimestamp != 0) { Interlocked.Decrement(ref _currentLookups); From 9875241f4885d04675acc2552c5bcb025a1ab89f Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Sat, 15 Jul 2023 01:06:43 +0200 Subject: [PATCH 5/5] More RemoteExecutor --- .../tests/FunctionalTests/MetricsTest.cs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs index 78645b312d116..262626ce3108a 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs @@ -7,6 +7,7 @@ using System.Linq; using System.Net.Sockets; using System.Threading.Tasks; +using Microsoft.DotNet.RemoteExecutor; using Xunit; namespace System.Net.NameResolution.Tests @@ -15,27 +16,29 @@ public class MetricsTest { private const string DnsLookupsRequested = "dns-lookups-requested"; - [Fact] - public static async Task ResolveValidHostName_MetricsRecorded() + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public static void ResolveValidHostName_MetricsRecorded() { - const string ValidHostName = "localhost"; + RemoteExecutor.Invoke(async () => + { + const string ValidHostName = "localhost"; - using var recorder = new InstrumentRecorder(DnsLookupsRequested); + using var recorder = new InstrumentRecorder(DnsLookupsRequested); - await Dns.GetHostEntryAsync(ValidHostName); - await Dns.GetHostAddressesAsync(ValidHostName); + await Dns.GetHostEntryAsync(ValidHostName); + await Dns.GetHostAddressesAsync(ValidHostName); - Dns.GetHostEntry(ValidHostName); - Dns.GetHostAddresses(ValidHostName); + Dns.GetHostEntry(ValidHostName); + Dns.GetHostAddresses(ValidHostName); - Dns.EndGetHostEntry(Dns.BeginGetHostEntry(ValidHostName, null, null)); - Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(ValidHostName, null, null)); + Dns.EndGetHostEntry(Dns.BeginGetHostEntry(ValidHostName, null, null)); + Dns.EndGetHostAddresses(Dns.BeginGetHostAddresses(ValidHostName, null, null)); - long[] measurements = GetMeasurementsForHostname(recorder, ValidHostName); + long[] measurements = GetMeasurementsForHostname(recorder, ValidHostName); - // >= 6 because other tests running in parallel may have also resolved the same host. - Assert.True(measurements.Length >= 6, $"Was {measurements.Length}"); - Assert.All(measurements, m => Assert.Equal(1, m)); + Assert.Equal(6, measurements.Length); + Assert.All(measurements, m => Assert.Equal(1, m)); + }).Dispose(); } [Fact]