Skip to content

Comments

Improve Http Telemetry tests#50226

Merged
MihaZupan merged 1 commit intodotnet:mainfrom
MihaZupan:http-telem-tests
Apr 6, 2021
Merged

Improve Http Telemetry tests#50226
MihaZupan merged 1 commit intodotnet:mainfrom
MihaZupan:http-telem-tests

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Mar 25, 2021

Replace Task.Delay(hopefullyEnoughTime) with actually waiting for event counters (following what we did in Sockets telemetry tests).

While I was changing the code, I added the asserts for ActivityIds.

Fixes #46075
Fixes #46073
Fixes #41723

@MihaZupan MihaZupan added this to the 6.0.0 milestone Mar 25, 2021
@MihaZupan MihaZupan requested a review from a team March 25, 2021 04:09
@ghost ghost added the area-System.Net.Http label Mar 25, 2021
@ghost
Copy link

ghost commented Mar 25, 2021

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

Issue Details

Replace Task.Delay(hopefullyEnoughTime) with actually waiting for event counters (following what we did in Sockets telemetry tests).

While I was changing the code, I added the asserts for ActivityIds.

Fixes #46075
Fixes #46073
Fixes #41723

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http
Milestone: 6.0.0

@MihaZupan

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@MihaZupan
Copy link
Member Author

MihaZupan commented Mar 25, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 25, 2021

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP
ManickaP approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks. Hope it helps 😄

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

private static async Task WaitForEventCountersAsync(ConcurrentQueue<(EventWrittenEventArgs Event, Guid ActivityId)> events)
{
DateTime startTime = DateTime.UtcNow;
Copy link
Member

@ManickaP ManickaP Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DateTime startTime = DateTime.UtcNow;
long startTime = Environment.TickCount64;

It might not matter here since the timeout is 30s, but every time I see DateTime used like this, I'm a bit wary.

@MihaZupan
Copy link
Member Author

MihaZupan commented Mar 25, 2021 •
edited
Loading

There are 2 relevant test failures here I am still looking into
- One failure with the RequestLeftQueue test timing out
- One failure with the RequestLeftQueue ActivityIds mismatch on H2

I misread the stack trace - there was only 1 failure on osx related to ordering.
We should see fewer failures with this change.

@MihaZupan MihaZupan merged commit 8b6f3ce into dotnet:main Apr 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.