-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust System.Net.Http metrics #89809
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAdjust System.Net.Http metrics naming and semantics according to the outcome of the discussion in lmolkova/semantic-conventions#1: Contributes to #89093.
|
[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 => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes #89451 by making the test deterministic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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
This comment was marked as resolved.
This comment was marked as resolved.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
All CI failures are unrelated. |
_ => $"HTTP/{httpVersion.Major}.{httpVersion.Minor}" | ||
(1, 0) => "1.0", | ||
(1, 1) => "1.1", | ||
(2, 0) => "2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I will open a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I was delayed with some other work, this looked good but I noticed two instruments are missing their units.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #90020 PTAL.
_currentRequests = meter.CreateUpDownCounter<long>( | ||
"http-client-current-requests", | ||
_activeRequests = meter.CreateUpDownCounter<long>( | ||
"http.client.active_requests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit should be "{request}"
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.