[NOMERGE] [HTTP] Metrics logging - #132116
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 adds extra diagnostics to the System.Net.Http metrics functional tests to help investigate intermittent failures in HttpMetricsTest_Http20.TimeInQueue_RecordedForNewConnectionsOnly (#122522).
Changes:
- Augments
InstrumentRecorder<T>to attach stack traces to recorded measurements and adds aClear()helper. - Modifies
TimeInQueue_RecordedForNewConnectionsOnlyto repeatedly run the scenario and dump stack traces when unexpected extra measurements are recorded. - Adds a new
using TestUtilities;(currently only referenced by a commented-out line).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:216
InstrumentRecorder<T>now unconditionally capturesEnvironment.StackTraceand adds it as a tag for every recorded measurement. This is very allocation-heavy (full stack trace string per measurement) and changes the tag set for all metrics tests in this file, which can significantly slow / flake the suite. Consider gating stack capture behind an opt-in switch/environment variable so the default path preserves the original lightweight behavior.
private void OnMeasurementRecorded(Instrument instrument, T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags, object? state)
{
KeyValuePair<string, object?>[] newTags = [..tags, new ("stack", Environment.StackTrace)];
_values.Enqueue(new Measurement<T>(measurement, newTags));
MeasurementRecorded?.Invoke();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:941
- Dumping full stack traces via
Console.WriteLinewhen the assertion is about to fail can produce extremely large test output and slow down Helix runs. Prefer writing toITestOutputHelperand gating the diagnostic output behind an opt-in switch so normal CI runs stay quiet.
if (timeInQueueRecorder.MeasurementCount > 1)
{
foreach (var v in timeInQueueRecorder.GetMeasurements())
{
foreach (var t in v.Tags)
{
if (t.Key == "stack")
{
Console.WriteLine(t.Value);
}
}
}
}
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:918
- The test now runs an inner loop of 3,000 client/server sessions and 5 requests each. This is likely to cause long runtimes/timeouts in CI and makes the test behavior depend on load/timing. If this is only for diagnostics, please gate the iteration count behind an environment variable (defaulting to 1) and fix the brace/indentation style.
using HttpMessageInvoker client = CreateHttpMessageInvoker();
using InstrumentRecorder<double> timeInQueueRecorder = SetupInstrumentRecorder<double>(InstrumentNames.TimeInQueue);
for (int ii = 0; ii < 3_000; ii++) {
const int RequestCount = 5;
timeInQueueRecorder.Clear();
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:285
- Applying
[Collection(nameof(DisableParallelization))]to the sharedHttpMetricsTestbase class forces all derived metrics test classes in this file to run serially, which can substantially increase CI wall time and may hide concurrency-related issues. If only specific tests are flaky, it’s better to scope DisableParallelization to those specific test classes.
[Collection(nameof(DisableParallelization))]
Additional logging to investigate #122522