Adjust System.Net.Http metrics by antonfirsov · Pull Request #89809 · dotnet/runtime

antonfirsov · 2023-08-01T20:30:13Z

Adjust System.Net.Http metrics naming and semantics according to the outcome of the discussion in lmolkova/semantic-conventions#1:

https://github.com/lmolkova/semantic-conventions/blob/dotnet8-metrics/docs/dotnet/dotnet-http-metrics.md

Contributes to #89093.
Fixes #89451.

ghost · 2023-08-01T20:30:27Z

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

Issue Details

Adjust System.Net.Http metrics naming and semantics according to the outcome of the discussion in lmolkova/semantic-conventions#1:

https://github.com/lmolkova/semantic-conventions/blob/dotnet8-metrics/docs/dotnet/dotnet-http-metrics.md

Contributes to #89093.
Fixes #89451.

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

antonfirsov · 2023-08-01T20:40:44Z

src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs

+        [ConditionalFact(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))]
+        public async Task ActiveRequests_InstrumentEnabledAfterSending_NotRecorded()
        {
-            SemaphoreSlim instrumentEnabledSemaphore = new SemaphoreSlim(0);
+            if (UseVersion == HttpVersion.Version30)
+            {
+                return; // This test depends on ConnectCallback.
+            }
+
+            TaskCompletionSource connectionStarted = new TaskCompletionSource();

            await LoopbackServerFactory.CreateClientAndServerAsync(async uri =>
            {
                using HttpMessageInvoker client = CreateHttpMessageInvoker();
+                GetUnderlyingSocketsHttpHandler(Handler).ConnectCallback = async (ctx, cancellationToken) =>
+                {
+                    connectionStarted.SetResult();
+                    Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
+                    try
+                    {
+                        await socket.ConnectAsync(ctx.DnsEndPoint, cancellationToken);
+                        return new NetworkStream(socket, ownsSocket: true);
+                    }
+                    catch
+                    {
+                        socket.Dispose();
+                        throw;
+                    }
+                };

                // Enable recording request-duration to test the path with metrics enabled.
                using InstrumentRecorder<double> unrelatedRecorder = SetupInstrumentRecorder<double>(InstrumentNames.RequestDuration);

                using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = UseVersion };
-                Task<HttpResponseMessage> clientTask = SendAsync(client, request);
-                await Task.Delay(100);
-                using InstrumentRecorder<long> recorder = new(Handler.MeterFactory, InstrumentNames.CurrentRequests);
-                instrumentEnabledSemaphore.Release();
-
+                Task<HttpResponseMessage> clientTask = Task.Run(() => SendAsync(client, request));
+                await connectionStarted.Task;
+                using InstrumentRecorder<long> recorder = new(Handler.MeterFactory, InstrumentNames.ActiveRequests);
                using HttpResponseMessage response = await clientTask;

                Assert.Empty(recorder.GetMeasurements());
            }, async server =>


This fixes #89451 by making the test deterministic.

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

antonfirsov · 2023-08-02T21:45:36Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-08-02T21:45:52Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2023-08-03T13:52:34Z

/azp run runtime

azure-pipelines · 2023-08-03T13:53:04Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2023-08-03T17:06:16Z

All CI failures are unrelated.

lmolkova · 2023-08-03T17:58:49Z

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

-            _ => $"HTTP/{httpVersion.Major}.{httpVersion.Minor}"
+            (1, 0) => "1.0",
+            (1, 1) => "1.1",
+            (2, 0) => "2.0",


I think @JamesNK uses "2" and "3" for HTTP/2 and HTTP/3 for asp.net core and kestrel, which, I believe, matches their RFCs (HTTP/2, HTTP/3).

can we still unify?

Yup I will open a follow-up PR.

Addressing #89809 (comment)

noahfalk

Sorry I was delayed with some other work, this looked good but I noticed two instruments are missing their units.

noahfalk · 2023-08-04T06:48:56Z

.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs

-            name: "http-client-current-idle-connections",
-            description: "Number of outbound HTTP connections that are currently idle on the client.");
+        public readonly UpDownCounter<long> OpenConnections = meter.CreateUpDownCounter<long>(
+            name: "http.client.open_connections",


The unit should be set to "{connection}"

@lmolkova - confirming that is correct and not just some formatting thing in the spec that wasn't intended to be used literally?

Opened #90020 PTAL.

noahfalk · 2023-08-04T06:50:29Z

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

-            _currentRequests = meter.CreateUpDownCounter<long>(
-                "http-client-current-requests",
+            _activeRequests = meter.CreateUpDownCounter<long>(
+                "http.client.active_requests",


unit should be "{request}"

adjust System.Net.Http metrics to the new spec

eee946f

ghost added the area-System.Net.Http label Aug 1, 2023

ghost assigned antonfirsov Aug 1, 2023

antonfirsov requested a review from MihaZupan August 1, 2023 20:30

antonfirsov requested review from a team and noahfalk August 1, 2023 20:30

antonfirsov added this to the 8.0.0 milestone Aug 1, 2023

Merge branch 'main' into adjust-http-metrics-02

709d058

antonfirsov commented Aug 1, 2023

View reviewed changes

antonfirsov added 2 commits August 1, 2023 22:51

also adjust descriptions

df18ba5

simpler comment

8f43983

This comment was marked as resolved.

Sign in to view

_idleSinceTickCount init should not depend on metrics

cea486e

This comment was marked as resolved.

Sign in to view

antonfirsov added 3 commits August 2, 2023 15:03

PATCH RequestMetrics_EmitNormalizedMethodTags for wasm

1260105

fix typo

29ded04

Merge branch 'main' into adjust-http-metrics-02

5687758

MihaZupan approved these changes Aug 2, 2023

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs Show resolved Hide resolved

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs Show resolved Hide resolved

antonfirsov mentioned this pull request Aug 2, 2023

Dotnet8 metrics in otel format lmolkova/semantic-conventions#1

Closed

add an assertion to guard the HttpRequestError mapping

ce3eb0c

This comment was marked as resolved.

Sign in to view

disable RequestMetrics_EmitNormalizedMethodTags on browser completely

653c6e7

kotlarmilos mentioned this pull request Aug 3, 2023

[mono][iossimulator] Undefined globalization symbols for iossimulator-arm64 #89925

Closed

antonfirsov merged commit 82934fd into dotnet:main Aug 3, 2023

lmolkova reviewed Aug 3, 2023

View reviewed changes

antonfirsov mentioned this pull request Aug 3, 2023

[HTTP Metrics] Shorter version tags #89959

Merged

antonfirsov added a commit that referenced this pull request Aug 3, 2023

[HTTP Metrics] Shorter version tags (#89959)

9d32dd6

Addressing #89809 (comment)

This was linked to issues Aug 4, 2023

Adjust Networking metrics to follow OpenTelemetry semantic conventions #89093

Closed

[Design] .NET metrics naming standard #88515

Closed

noahfalk reviewed Aug 4, 2023

View reviewed changes

antonfirsov mentioned this pull request Aug 4, 2023

Add units to http.client.active_requests & http.client.open_connections #90020

Merged

ghost locked as resolved and limited conversation to collaborators Sep 3, 2023

Conversation

antonfirsov commented Aug 1, 2023

Uh oh!

ghost commented Aug 1, 2023

Uh oh!

antonfirsov Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

This comment was marked as resolved.

antonfirsov commented Aug 2, 2023

Uh oh!

azure-pipelines bot commented Aug 2, 2023

Uh oh!

antonfirsov commented Aug 3, 2023

Uh oh!

azure-pipelines bot commented Aug 3, 2023

Uh oh!

antonfirsov commented Aug 3, 2023

Uh oh!

lmolkova Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

antonfirsov Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

noahfalk left a comment

Choose a reason for hiding this comment

Uh oh!

noahfalk Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

antonfirsov Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

noahfalk Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antonfirsov Aug 1, 2023 •

edited

Loading