[HTTP] High cardinality metrics to observable - #131275
Conversation
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR updates System.Net.Http client metrics to mitigate high-cardinality tag fallout by switching http.client.open_connections and http.client.active_requests from UpDownCounter<long> (event-based adds) to ObservableUpDownCounter<long> (snapshot reporting via callbacks).
Changes:
- Introduces per-tag-combination trackers (
ConcurrentDictionary<..., long>) to compute current counts and feed observable instruments. - Updates
MetricsHandlerandSocketsHttpHandlerconnection metrics to increment/decrement the trackers instead of callingAdd(...). - Adjusts
System.Net.Httpmetrics functional tests to explicitly callMeterListener.RecordObservableInstruments()and to validate snapshot-style observations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs | Updates tests to collect observable instrument values and adapts expectations to snapshot semantics. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs | Converts open_connections to an observable counter and adds a tracker + tag key infrastructure. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/ConnectionMetrics.cs | Routes connection open/close/idle transitions through the new open-connections tracker. |
| src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs | Converts active_requests to an observable counter backed by an active-requests tracker keyed by request tags. |
e98159f to
02d0a60
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/SocketsHttpHandlerMetrics.cs:8
using System.Threading;appears unused in this file and can trigger analyzer warnings. Remove it to keep the using list minimal.
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.Metrics;
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:706
- Assigned
requestDatais never used; this creates an unnecessary local (and can produce warnings). Just await the read.
var requestData = await connection.ReadRequestDataAsync();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:720
- Assigned
requestDatais never used; this creates an unnecessary local (and can produce warnings). Just await the read.
var requestData = await connection.ReadRequestDataAsync();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:1533
- Assigned
requestDatais never used; this creates an unnecessary local (and can produce warnings). Just await the read.
var requestData = await connection.ReadRequestDataAsync();
02d0a60 to
877451e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:321
- Unused local
requestDatawill trigger CS0219 (assigned but never used) under TreatWarningsAsErrors, potentially breaking the test build. Just await the read without assigning it.
await server.AcceptConnectionAsync(async connection =>
{
await connection.ReadRequestDataAsync();
serverTcs.SetResult();
await clientTcs.Task.WaitAsync(TestHelper.PassingTestTimeout);
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:708
- Unused local
requestDatawill trigger CS0219 (assigned but never used) under TreatWarningsAsErrors, potentially breaking the test build. Just await the read without assigning it.
var serverTask = originalServer.AcceptConnectionAsync(async connection =>
{
var requestData = await connection.ReadRequestDataAsync();
originalServerTcs.SetResult();
await clientTcs1.Task.WaitAsync(TestHelper.PassingTestTimeout);
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:722
- Unused local
requestDatawill trigger CS0219 (assigned but never used) under TreatWarningsAsErrors, potentially breaking the test build. Just await the read without assigning it.
serverTask = redirectServer.AcceptConnectionAsync(async connection =>
{
var requestData = await connection.ReadRequestDataAsync();
redirectServerTcs.SetResult();
await clientTcs2.Task.WaitAsync(TestHelper.PassingTestTimeout);
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:1535
- Unused local
requestDatawill trigger CS0219 (assigned but never used) under TreatWarningsAsErrors, potentially breaking the test build. Just await the read without assigning it.
await server.AcceptConnectionAsync(async connection =>
{
var requestData = await connection.ReadRequestDataAsync();
serverTcs.SetResult();
await clientTcs.Task.WaitAsync(TestHelper.PassingTestTimeout);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/ConnectionMetrics.cs:82
IdleStateChangedupdates_currentlyIdlebefore updating the tracker. IfConnectionClosedruns concurrently and reads the new_currentlyIdlevalue before the trackerIncrementfor that new-state key, itsDecrementcan be dropped (missing key), leaving the new-state count stuck at 1. Locking and updating_currentlyIdleafter tracker updates avoids this race.
if (_openConnectionsEnabled && _currentlyIdle != idle)
{
_currentlyIdle = idle;
_metrics.OpenConnectionsTracker.Decrement(CreateTagKey(idle: !idle));
_metrics.OpenConnectionsTracker.Increment(CreateTagKey(idle: idle));
There was a problem hiding this comment.
A bit scary we're doing bookkeeping like this where any subtle issue (#131275 (comment)) can become a memory leak, but it's also apparently no worse than what was already happening before, just in a different component.
Did you check what this looks like in the consuming tools (e.g. do all graphs end up going to 0 when we stop reporting them)?
They do, but that was the case before as well. The problem was that the exporter would remember even the 0s with all their tags for eternity. I'm right now testing this with Aspire Dashboard, and I get a suspicion this won't be enough. The UI is still showing long time dead active_request tags 😢 I'll investigate more with console exporter to see where the problem is. In the end, we might decide not to make this change... |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:706
- Unused local
requestDataadds noise; the awaited call is only needed for its side-effect. Drop the assignment.
var requestData = await connection.ReadRequestDataAsync();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:720
- Unused local
requestDataadds noise; the awaited call is only needed for its side-effect. Drop the assignment.
var requestData = await connection.ReadRequestDataAsync();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:1533
- Unused local
requestDataadds noise; the awaited call is only needed for its side-effect. Drop the assignment.
var requestData = await connection.ReadRequestDataAsync();
|
So the console exporter looks good, after the requests are handled and connections scavenged, only remaining stuff are So there's likely another layer of the issue on the dashboard side (i.e. in Aspire). Honestly, I'm torn whether we should merge this or not. I'd appreciate any opinions on it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:706
- This local is assigned but never used. In test projects this commonly becomes a warnings-as-errors build break. Just await the call without storing the result.
var requestData = await connection.ReadRequestDataAsync();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:1533
- This local is assigned but never used. In test projects this commonly becomes a warnings-as-errors build break. Just await the call without storing the result.
var requestData = await connection.ReadRequestDataAsync();
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:314
- The test only observes
http.client.active_requestswhile the request is in-flight. If the new tracker-based implementation fails to decrement/remove the entry at request completion, this test would still pass. Add a secondRecordObservableInstruments()after the response is disposed and keep asserting a single measurement, so the test fails if the count is stuck > 0.
var response = await requestTask;
response.Dispose(); // Make sure disposal doesn't interfere with recording by enforcing early disposal.
Assert.Collection(recorder.GetMeasurements(),
m => VerifyActiveRequests(m, 1, uri));
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:732
- Similar to the non-redirect test: after the redirect flow completes, add a final
RecordObservableInstruments()and keep asserting only the two in-flight observations. This makes the test fail if the active-requests tracker doesn’t get decremented/removed at the end of either span.
await TestHelper.WhenAllCompletedOrAnyFailed(clientTask, serverTask);
await clientTask;
Assert.Collection(recorder.GetMeasurements(),
m => VerifyActiveRequests(m, 1, originalUri),
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:226
- Now that
active_requestsandopen_connectionsare observable instruments, tests that rely onInstrumentRecorder<T>must callRecordObservableInstruments()while the request/connection is active; otherwiseGetMeasurements()can be empty and assertions become vacuously true. In this file, several tests still set up recorders for these instruments but never callRecordObservableInstruments()(e.g., method-tag normalization and server.address/Host-header verification tests), which means they no longer validate those metrics.
public void RecordObservableInstruments() => _meterListener.RecordObservableInstruments();
public IReadOnlyList<Measurement<T>> GetMeasurements() => _values.ToArray();
public void Dispose() => _meterListener.Dispose();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:413
ExternalServer_DurationMetrics_Recordedrecordshttp.client.open_connectionswhile theHttpResponseMessageis still in scope (not yet disposed). Depending on when the connection is returned to the pool, the observable may report the connection asactiverather thanidle, making the later assertion for theidlestate flaky/incorrect. Dispose the response before callingRecordObservableInstruments()so the observation happens after the connection has transitioned to idle.
using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = UseVersion };
using HttpResponseMessage response = await SendAsync(client, request);
await response.Content.LoadIntoBufferAsync();
openConnectionsRecorder.RecordObservableInstruments();
await WaitForEnvironmentTicksToAdvance();
|
outerloop unrelated failures on WinHttpHandler |
rosebyte
left a comment
rosebyte
left a comment
There was a problem hiding this comment.
Just one comment, feel free to resolve as you have better context here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:1578
- Same issue as the non-default-meter variant: this
Assert.Collectiondepends on callback / observation ordering for observable instruments, which isn't a stable contract. This can lead to intermittent failures when the order of emitted measurements differs. Prefer selecting and validating the expected measurements by instrument name (andhttp.connection.stateforopen_connections) instead of asserting sequence order.
Assert.Collection(recorder.GetMeasurements(),
m => VerifyTimeInQueue(m.InstrumentName, m.Value, m.Tags, uri, version),
m => VerifyActiveRequests(m.InstrumentName, (long)m.Value, m.Tags, 1, uri),
m => VerifyOpenConnections(m.InstrumentName, m.Value, m.Tags, 1, uri, version, "active"),
m => VerifyRequestDuration(m.InstrumentName, (double)m.Value, m.Tags, uri, version, 200),
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:413
openConnectionsRecorder.RecordObservableInstruments()is called while the response is still undisposed. Depending on when the connection is returned to the pool,http.client.open_connectionsmay still be in theactivestate, and observable instruments can return multiple measurements; additionally,.First()makes the assertion depend on enumeration order. Dispose the response before recording, and assert on a single expected measurement.
using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = UseVersion };
using HttpResponseMessage response = await SendAsync(client, request);
await response.Content.LoadIntoBufferAsync();
openConnectionsRecorder.RecordObservableInstruments();
await WaitForEnvironmentTicksToAdvance();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:817
MultiInstrumentRecorder.GetMeasurements()reflects the internal ordering ofMeterListenercallbacks andConcurrentDictionaryenumeration for observable instruments, which is not guaranteed to be stable. UsingAssert.Collectionhere makes the test order-dependent and likely flaky (e.g., instrument publish order / observation order can vary, and observable measurements are appended across multipleRecordObservableInstruments()calls). Prefer order-insensitive assertions by selecting the expected measurement(s) by instrument name (+ connection state tag) and then validating them.
This issue also appears on line 1574 of the same file.
Assert.Collection(recorder.GetMeasurements(),
m => VerifyTimeInQueue(m.InstrumentName, m.Value, m.Tags, uri, UseVersion),
m => VerifyActiveRequests(m.InstrumentName, (long)m.Value, m.Tags, 1, uri),
m => VerifyOpenConnections(m.InstrumentName, m.Value, m.Tags, 1, uri, UseVersion, "active"),
m => VerifyRequestDuration(m.InstrumentName, (double)m.Value, m.Tags, uri, UseVersion, 200),
The open_connections and active_requests up down counters both use peer address in their tags and suffer from accumulation of data at the monitoring side. This PR changes them to their observable counterpart that counts the values and reports them only when asked.
This does not solve the underlying issue, this just somewhat mitigates the biggest pain points for customers. Eventually, this change should be reverted when the issue is solved on OpenTelemtry side, see open-telemetry/semantic-conventions#3279.
Unintentianal side effect is that if someone is directly using
MeterListenerthis change will be breaking for them. They also need to start usingRecordObservableInstrumentsto get the values now, see changes in our metrics tests.Fixes #122752