From 6e2e0205d704b660e27b109d6eee24153b78a664 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 4 Jul 2024 04:10:41 +0200 Subject: [PATCH 1/5] swap MetricsHandler and DiagnosticsHandler --- .../Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index a20b9c56485410..170060b5da2728 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -529,16 +529,15 @@ private HttpMessageHandlerStage SetupHandlerChain() handler = new HttpAuthenticatedConnectionHandler(poolManager); } + handler = new MetricsHandler(handler, settings._meterFactory, out Meter meter); + settings._metrics = new SocketsHttpHandlerMetrics(meter); + // DiagnosticsHandler is inserted before RedirectHandler so that trace propagation is done on redirects as well if (DiagnosticsHandler.IsGloballyEnabled() && settings._activityHeadersPropagator is DistributedContextPropagator propagator) { handler = new DiagnosticsHandler(handler, propagator, settings._allowAutoRedirect); } - handler = new MetricsHandler(handler, settings._meterFactory, out Meter meter); - - settings._metrics = new SocketsHttpHandlerMetrics(meter); - if (settings._allowAutoRedirect) { // Just as with WinHttpHandler, for security reasons, we do not support authentication on redirects From 24e5164f395ae449abecbd0b765ecc9ad01eea7e Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 5 Jul 2024 00:51:16 +0200 Subject: [PATCH 2/5] Swap MetricsHandler and DiagnosticsHandler --- ...ntHandler.AnyMobile.InvokeNativeHandler.cs | 2 +- .../Net/Http/HttpClientHandler.AnyMobile.cs | 21 +++---- .../src/System/Net/Http/HttpClientHandler.cs | 22 +++++--- .../SocketsHttpHandler/SocketsHttpHandler.cs | 2 + .../tests/FunctionalTests/MetricsTest.cs | 55 ++++++++++++++++++- 5 files changed, 80 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs index 8f8c101b9e4a52..183ab043cbea66 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs @@ -176,7 +176,7 @@ private object InvokeNativeHandlerMethod(Func getMethod, object?[]? try { - return method!.Invoke(_nativeHandler, parameters)!; + return method!.Invoke(_nativeUnderlyingHandler, parameters)!; } catch (TargetInvocationException e) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs index 0d647385f996ca..a8f7613b7e0834 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs @@ -22,9 +22,9 @@ public partial class HttpClientHandler : HttpMessageHandler { private static readonly ConcurrentDictionary s_cachedMethods = new(); - private readonly HttpMessageHandler? _nativeHandler; + private readonly HttpMessageHandler? _nativeUnderlyingHandler; private IMeterFactory? _nativeMeterFactory; - private MetricsHandler? _nativeMetricsHandler; + private HttpMessageHandler? _nativeFirstHandler; // DiagnosticsHandler or MetricsHandler, depending on global configuration. private readonly SocketsHttpHandler? _socketHandler; @@ -38,23 +38,24 @@ private HttpMessageHandler Handler { if (IsNativeHandlerEnabled) { - if (_nativeMetricsHandler is null) + if (_nativeFirstHandler is null) { // We only setup these handlers for the native handler. SocketsHttpHandler already does this internally. - HttpMessageHandler handler = _nativeHandler!; + HttpMessageHandler handler = _nativeUnderlyingHandler!; + // MetricsHandler should be descendant of DiagnosticsHandler in the handler chain to make sure the 'http.request.duration' + // metric is recorded before stopping the request Activity. This is needed to make sure that our telemetry supports Exemplars. + handler = new MetricsHandler(handler, _nativeMeterFactory, out _); if (DiagnosticsHandler.IsGloballyEnabled()) { handler = new DiagnosticsHandler(handler, DistributedContextPropagator.Current); } - MetricsHandler metricsHandler = new MetricsHandler(handler, _nativeMeterFactory, out _); - // Ensure a single handler is used for all requests. - Interlocked.CompareExchange(ref _nativeMetricsHandler, metricsHandler, null); + Interlocked.CompareExchange(ref _nativeFirstHandler, handler, null); } - return _nativeMetricsHandler; + return _nativeFirstHandler; } else { @@ -67,7 +68,7 @@ public HttpClientHandler() { if (IsNativeHandlerEnabled) { - _nativeHandler = CreateNativeHandler(); + _nativeUnderlyingHandler = CreateNativeHandler(); } else { @@ -115,7 +116,7 @@ public IMeterFactory? MeterFactory if (IsNativeHandlerEnabled) { - if (_nativeMetricsHandler is not null) + if (_nativeFirstHandler is not null) { throw new InvalidOperationException(SR.net_http_operation_started); } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs index 7a6edd71053bb6..5a34491ab4c9f4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs @@ -26,30 +26,34 @@ public partial class HttpClientHandler : HttpMessageHandler #if TARGET_BROWSER private IMeterFactory? _meterFactory; - private MetricsHandler? _metricsHandler; + private HttpMessageHandler? _firstHandler; // DiagnosticsHandler or MetricsHandler, depending on global configuration. - private MetricsHandler Handler + private HttpMessageHandler Handler { get { - if (_metricsHandler != null) + if (_firstHandler != null) { - return _metricsHandler; + return _firstHandler; } HttpMessageHandler handler = _underlyingHandler; + + // MetricsHandler should be descendant of DiagnosticsHandler in the handler chain to make sure the 'http.request.duration' + // metric is recorded before stopping the request Activity. This is needed to make sure that our telemetry supports Exemplars. + handler = new MetricsHandler(handler, _meterFactory, out _); if (DiagnosticsHandler.IsGloballyEnabled()) { handler = new DiagnosticsHandler(handler, DistributedContextPropagator.Current); } - MetricsHandler metricsHandler = new MetricsHandler(handler, _meterFactory, out _); // Ensure a single handler is used for all requests. - if (Interlocked.CompareExchange(ref _metricsHandler, metricsHandler, null) != null) + if (Interlocked.CompareExchange(ref _firstHandler, handler, null) != null) { - metricsHandler.Dispose(); + handler.Dispose(); } - return _metricsHandler; + + return _firstHandler; } } #else @@ -95,7 +99,7 @@ public IMeterFactory? MeterFactory set { ObjectDisposedException.ThrowIf(_disposed, this); - if (_metricsHandler != null) + if (_firstHandler != null) { throw new InvalidOperationException(SR.net_http_operation_started); } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 170060b5da2728..44acad742e7c21 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -529,6 +529,8 @@ private HttpMessageHandlerStage SetupHandlerChain() handler = new HttpAuthenticatedConnectionHandler(poolManager); } + // MetricsHandler should be descendant of DiagnosticsHandler in the handler chain to make sure the 'http.request.duration' + // metric is recorded before stopping the request Activity. This is needed to make sure that our telemetry supports Exemplars. handler = new MetricsHandler(handler, settings._meterFactory, out Meter meter); settings._metrics = new SocketsHttpHandlerMetrics(meter); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs index e6c74540a9053f..5f74bcfef3a814 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs @@ -160,6 +160,8 @@ protected sealed class InstrumentRecorder : IDisposable where T : struct private readonly ConcurrentQueue> _values = new(); private Meter? _meter; + public Action? MeasurementRecorded; + public InstrumentRecorder(string instrumentName) { _meterListener.InstrumentPublished = (instrument, listener) => @@ -187,7 +189,12 @@ public InstrumentRecorder(IMeterFactory meterFactory, string instrumentName) _meterListener.Start(); } - private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) => _values.Enqueue(new Measurement(measurement, tags)); + private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) + { + _values.Enqueue(new Measurement(measurement, tags)); + MeasurementRecorded?.Invoke(); + } + public IReadOnlyList> GetMeasurements() => _values.ToArray(); public void Dispose() => _meterListener.Dispose(); } @@ -334,6 +341,51 @@ public Task RequestDuration_Success_Recorded(string method, HttpStatusCode statu }); } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public async Task RequestDuration_HttpTracingEnabled_RecordedWhileRequestActivityRunning() + { + await RemoteExecutor.Invoke(static testClass => + { + HttpMetricsTest test = (HttpMetricsTest)Activator.CreateInstance(Type.GetType(testClass), (ITestOutputHelper)null); + + return test.LoopbackServerFactory.CreateClientAndServerAsync(async uri => + { + using HttpMessageInvoker client = test.CreateHttpMessageInvoker(); + using InstrumentRecorder recorder = test.SetupInstrumentRecorder(InstrumentNames.RequestDuration); + + Activity? activity = null; + bool stopped = false; + + ActivitySource.AddActivityListener(new ActivityListener + { + ShouldListenTo = s => s.Name is "System.Net.Http", + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.AllData, + ActivityStarted = created => activity = created, + ActivityStopped = _ => stopped = true + }); + + recorder.MeasurementRecorded = () => + { + Assert.NotNull(activity); + Assert.False(stopped); + Assert.Same(activity, Activity.Current); + }; + + using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = test.UseVersion }; + using HttpResponseMessage response = await test.SendAsync(client, request); + + Assert.NotNull(activity); + + Measurement m = Assert.Single(recorder.GetMeasurements()); + VerifyRequestDuration(m, uri, test.UseVersion, 200, "GET"); + + }, async server => + { + await server.AcceptConnectionSendResponseAndCloseAsync(); + }); + }, GetType().FullName).DisposeAsync(); + } + [Fact] public Task RequestDuration_CustomTags_Recorded() { @@ -363,7 +415,6 @@ public Task RequestDuration_CustomTags_Recorded() [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [InlineData("System.Net.Http.HttpRequestOut.Start")] [InlineData("System.Net.Http.Request")] - [InlineData("System.Net.Http.HttpRequestOut.Stop")] public async Task RequestDuration_CustomTags_DiagnosticListener_Recorded(string eventName) { await RemoteExecutor.Invoke(static async (testClassName, eventNameInner) => From 5be1f2b0ef3a1cd7449d60a860b2660cd7e1a3f3 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Mon, 8 Jul 2024 19:29:23 +0200 Subject: [PATCH 3/5] wip --- .../System.Net.Http/src/System.Net.Http.csproj | 1 + .../src/System/Net/Http/DiagnosticsHelper.cs | 15 +++++++++++++++ .../src/System/Net/Http/Metrics/MetricsHandler.cs | 3 ++- 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs diff --git a/src/libraries/System.Net.Http/src/System.Net.Http.csproj b/src/libraries/System.Net.Http/src/System.Net.Http.csproj index dea20f206cdc9d..8317359183f138 100644 --- a/src/libraries/System.Net.Http/src/System.Net.Http.csproj +++ b/src/libraries/System.Net.Http/src/System.Net.Http.csproj @@ -38,6 +38,7 @@ + diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs new file mode 100644 index 00000000000000..40016b28b2afca --- /dev/null +++ b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.Metrics; + +namespace System.Net.Http +{ + internal static class DiagnosticsHelper + { + public static InstrumentAdvice ShortHistogramAdvice { get; } = new() + { + HistogramBucketBoundaries = [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] + }; + } +} diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs index 942cd6ceaed36b..6f45c3ddb2b055 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs @@ -29,7 +29,8 @@ public MetricsHandler(HttpMessageHandler innerHandler, IMeterFactory? meterFacto _requestsDuration = meter.CreateHistogram( "http.client.request.duration", unit: "s", - description: "Duration of HTTP client requests."); + description: "Duration of HTTP client requests.", + advice: DiagnosticsHelper.ShortHistogramAdvice); } internal override ValueTask SendAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) From 520a3937391f98b6d6d8adf88d071f04e005fea3 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Mon, 8 Jul 2024 23:04:18 +0200 Subject: [PATCH 4/5] Add bucket size hints for HTTP & DNS histograms --- .../src/System/Net/Http/DiagnosticsHelper.cs | 3 ++ .../Metrics/SocketsHttpHandlerMetrics.cs | 10 ++++- .../tests/FunctionalTests/MetricsTest.cs | 44 +++++++++++++++++++ .../src/System/Net/NameResolutionMetrics.cs | 9 +++- .../tests/FunctionalTests/MetricsTest.cs | 31 ++++++++++++- 5 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs index 40016b28b2afca..8fbd42c5364276 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs @@ -7,6 +7,9 @@ namespace System.Net.Http { internal static class DiagnosticsHelper { + // OTel bucket boundary recommendation for 'http.request.duration': + // https://github.com/open-telemetry/semantic-conventions/blob/release/v1.23.x/docs/http/http-metrics.md#metric-httpclientrequestduration + // We are using these boundaries for all network requests that are expected to be short. public static InstrumentAdvice ShortHistogramAdvice { get; } = new() { HistogramBucketBoundaries = [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs index bd37b3f66b8051..aaf55f9b5ae891 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs @@ -16,12 +16,18 @@ internal sealed class SocketsHttpHandlerMetrics(Meter meter) public readonly Histogram ConnectionDuration = meter.CreateHistogram( name: "http.client.connection.duration", unit: "s", - description: "The duration of successfully established outbound HTTP connections."); + description: "The duration of successfully established outbound HTTP connections.", + advice: new InstrumentAdvice() + { + // These values are not based on a standard and may change in the future. + HistogramBucketBoundaries = [0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300] + }); public readonly Histogram RequestsQueueDuration = meter.CreateHistogram( name: "http.client.request.time_in_queue", unit: "s", - description: "The amount of time requests spent on a queue waiting for an available connection."); + description: "The amount of time requests spent on a queue waiting for an available connection.", + advice: DiagnosticsHelper.ShortHistogramAdvice); public void RequestLeftQueue(HttpRequestMessage request, HttpConnectionPool pool, TimeSpan duration, int versionMajor) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs index 5f74bcfef3a814..341c42e5a3c9c6 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs @@ -161,6 +161,8 @@ protected sealed class InstrumentRecorder : IDisposable where T : struct private Meter? _meter; public Action? MeasurementRecorded; + public Action> VerifyHistogramBucketBoundaries; + public int MeasurementCount => _values.Count; public InstrumentRecorder(string instrumentName) { @@ -193,6 +195,13 @@ private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnl { _values.Enqueue(new Measurement(measurement, tags)); MeasurementRecorded?.Invoke(); + if (VerifyHistogramBucketBoundaries is not null) + { + Histogram histogram = (Histogram)instrument; + IReadOnlyList boundaries = histogram.Advice.HistogramBucketBoundaries; + Assert.NotNull(boundaries); + VerifyHistogramBucketBoundaries(boundaries); + } } public IReadOnlyList> GetMeasurements() => _values.ToArray(); @@ -328,6 +337,7 @@ public Task RequestDuration_Success_Recorded(string method, HttpStatusCode statu { using HttpMessageInvoker client = CreateHttpMessageInvoker(); using InstrumentRecorder recorder = SetupInstrumentRecorder(InstrumentNames.RequestDuration); + using HttpRequestMessage request = new(HttpMethod.Parse(method), uri) { Version = UseVersion }; using HttpResponseMessage response = await SendAsync(client, request); @@ -924,6 +934,40 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => } }); } + + [Fact] + public Task DurationHistograms_HaveBucketSizeHints() + { + return LoopbackServerFactory.CreateClientAndServerAsync(async uri => + { + using HttpMessageInvoker client = CreateHttpMessageInvoker(); + using InstrumentRecorder requestDurationRecorder = SetupInstrumentRecorder(InstrumentNames.RequestDuration); + using InstrumentRecorder timeInQueueRecorder = SetupInstrumentRecorder(InstrumentNames.TimeInQueue); + using InstrumentRecorder connectionDurationRecorder = SetupInstrumentRecorder(InstrumentNames.ConnectionDuration); + + requestDurationRecorder.VerifyHistogramBucketBoundaries = b => + { + // Verify first and last value of the boundaries defined in + // https://github.com/open-telemetry/semantic-conventions/blob/release/v1.23.x/docs/http/http-metrics.md#metric-httpserverrequestduration + Assert.Equal(0.005, b.First()); + Assert.Equal(10, b.Last()); + }; + timeInQueueRecorder.VerifyHistogramBucketBoundaries = requestDurationRecorder.VerifyHistogramBucketBoundaries; + connectionDurationRecorder.VerifyHistogramBucketBoundaries = + b => Assert.True(b.Last() > 180); // At least 3 minutes for the highest bucket. + + using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = UseVersion }; + using HttpResponseMessage response = await SendAsync(client, request); + + Assert.Equal(1, requestDurationRecorder.MeasurementCount); + Assert.Equal(1, timeInQueueRecorder.MeasurementCount); + client.Dispose(); // terminate the connection + Assert.Equal(1, connectionDurationRecorder.MeasurementCount); + }, async server => + { + await server.AcceptConnectionSendResponseAndCloseAsync(); + }); + } } [ActiveIssue("https://github.com/dotnet/runtime/issues/93754", TestPlatforms.Browser)] 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 fe1048e90b22de..580254289f9f7c 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionMetrics.cs @@ -15,7 +15,14 @@ internal static class NameResolutionMetrics private static readonly Histogram s_lookupDuration = s_meter.CreateHistogram( name: "dns.lookup.duration", unit: "s", - description: "Measures the time taken to perform a DNS lookup."); + description: "Measures the time taken to perform a DNS lookup.", + advice: new InstrumentAdvice() + { + // OTel bucket boundary recommendation for 'http.request.duration': + // https://github.com/open-telemetry/semantic-conventions/blob/release/v1.23.x/docs/http/http-metrics.md#metric-httpclientrequestduration + // We are using these boundaries for all network requests that are expected to be short. + HistogramBucketBoundaries = [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] + }); public static bool IsEnabled() => s_lookupDuration.Enabled; diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs index e72a7f2940a13a..ef50997547dc28 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/MetricsTest.cs @@ -41,6 +41,22 @@ await RemoteExecutor.Invoke(async () => }).DisposeAsync(); } + [Fact] + public static async Task DurationHistogram_HasBucketSizeHints() + { + await RemoteExecutor.Invoke(async () => + { + const string ValidHostName = "localhost"; + + using var recorder = new InstrumentRecorder(DnsLookupDuration); + recorder.VerifyHistogramBucketBoundaries = b => Assert.True(b.Count > 2); + + await Dns.GetHostEntryAsync(ValidHostName); + + Assert.Equal(1, recorder.MeasurementCount); + }).DisposeAsync(); + } + [Fact] public static async Task ResolveInvalidHostName_MetricsRecorded() { @@ -86,6 +102,9 @@ private sealed class InstrumentRecorder : IDisposable where T : struct private readonly MeterListener _meterListener = new(); private readonly ConcurrentQueue> _values = new(); + public Action> VerifyHistogramBucketBoundaries; + public int MeasurementCount => _values.Count; + public InstrumentRecorder(string instrumentName) { _meterListener.InstrumentPublished = (instrument, listener) => @@ -99,7 +118,17 @@ public InstrumentRecorder(string instrumentName) _meterListener.Start(); } - private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) => _values.Enqueue(new Measurement(measurement, tags)); + private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan> tags, object? state) + { + _values.Enqueue(new Measurement(measurement, tags)); + if (VerifyHistogramBucketBoundaries is not null) + { + Histogram histogram = (Histogram)instrument; + IReadOnlyList boundaries = histogram.Advice.HistogramBucketBoundaries; + Assert.NotNull(boundaries); + VerifyHistogramBucketBoundaries(boundaries); + } + } public IReadOnlyList> GetMeasurements() => _values.ToArray(); public void Dispose() => _meterListener.Dispose(); } From f16fe7739f962eb8796a2f2c4914304a383d098e Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Wed, 10 Jul 2024 01:03:42 +0200 Subject: [PATCH 5/5] Update src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs --- .../System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs index 8fbd42c5364276..445020f86eb770 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs @@ -9,7 +9,7 @@ internal static class DiagnosticsHelper { // OTel bucket boundary recommendation for 'http.request.duration': // https://github.com/open-telemetry/semantic-conventions/blob/release/v1.23.x/docs/http/http-metrics.md#metric-httpclientrequestduration - // We are using these boundaries for all network requests that are expected to be short. + // We are using the same boundaries for durations which are not expected to be longer than an HTTP request. public static InstrumentAdvice ShortHistogramAdvice { get; } = new() { HistogramBucketBoundaries = [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]