Skip to content

Address flaky hosting event counters tests - #57269

Merged
JamesNK merged 5 commits into
mainfrom
jamesnk/hosting-logging
Aug 13, 2024
Merged

JamesNK merged 5 commits into
mainfrom
jamesnk/hosting-logging

Conversation

@JamesNK

@JamesNK JamesNK commented Aug 11, 2024 •

Copy link
Copy Markdown
Member

Addresses #57259

Counter tests are flaky. Possible cause is a timing change for reporting counter values in dotnet/runtime.

PR changes:

  • Adds logging to help find out what is going on.
  • Sums changes from event counter events. Makes tests more resilient to counter events spread across intervals.
  • Ensures test event listener is only listening to values from the current test.

@JamesNK

JamesNK commented Aug 12, 2024

Copy link
Copy Markdown
Member Author

I noticed when I added logging that TestCounterListener (an event listener) was getting events from event sources in different tests. This was surprising to me because the listener's EnableEvents method has the event source as an argument, so I would have expected it to only listen to events from that event source.

I fixed the issue by also passing the unique event source name to the listener and filtering event data.

Is this expected? @noahfalk @tarekgh

@JamesNK
JamesNK force-pushed the jamesnk/hosting-logging branch from 0113703 to bd62721 Compare August 12, 2024 00:22
@JamesNK JamesNK changed the title Additional logging for EventCountersAndMetricsValues Address flaky hosting event counters tests Aug 12, 2024
@noahfalk

Copy link
Copy Markdown
Member

I noticed when I added logging that TestCounterListener (an event listener) was getting events from event sources in different tests. This was surprising to me because the listener's EnableEvents method has the event source as an argument, so I would have expected it to only listen to events from that event source.

I fixed the issue by also passing the unique event source name to the listener and filtering event data.

Is this expected? @noahfalk @tarekgh

That sounds like you hit dotnet/runtime#31927. Its expected only in the sense that it is a known bug that has been around a long time. I'm encouraging that we fix it.

Comment thread src/Hosting/Hosting/test/Internal/AsyncEnumerableExtensions.cs Outdated
Comment thread src/Shared/EventSource.Testing/TestCounterListener.cs

public override void Dispose()
{
base.Dispose();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Naively, I would have expected the base call to go at the end, though it doesn't appear to matter in this case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Usually, I put base call at the beginning unless there is a reason not to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I generally agree, I think Dispose is a mirror of the constructor and, for similar reasons, it makes sense to tear down the derived class before tearing down the base class (which it may depend upon).

Comment thread src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs Outdated
Co-authored-by: Andrew Casey <amcasey@users.noreply.github.com>
@JamesNK
JamesNK enabled auto-merge (squash) August 12, 2024 23:26
@JamesNK

JamesNK commented Aug 13, 2024

Copy link
Copy Markdown
Member Author

That sounds like you hit dotnet/runtime#31927. Its expected only in the sense that it is a known bug that has been around a long time. I'm encouraging that we fix it.

Ok. These tests have been flawed forever 😬

@JamesNK

JamesNK commented Aug 13, 2024

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@JamesNK
JamesNK merged commit a03b790 into main Aug 13, 2024
@JamesNK
JamesNK deleted the jamesnk/hosting-logging branch August 13, 2024 16:08
@dotnet-policy-service dotnet-policy-service Bot added this to the 9.0-rc1 milestone Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hosting Includes Hosting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants