Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix MetricsEventSource tests #56382

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Fix MetricsEventSource tests #56382

merged 1 commit into from
Jul 27, 2021

Conversation

noahfalk
Copy link
Member

Last week I added a change to the error handling behavior when MetricEventSource
is enabled by multiple listeners and didn't properly update the tests.

  1. The major issue was timout failures which were being caused because there is
    a bug in EventListener where it doesn't notify EventSources that the source has
    been disabled when the listener is disposed. This in turn caused every tests after
    the first to reject the new EventListener because the EventSource believed it was
    still in use by the first EventListener.
    (EventListener bug is tracked Disposing EventListener doesn't send disable command #56378)
  2. A secondary issue is that I didn't update the test which was explicitly verifying
    the behavior where the EventSource emits an error in response to having two
    listeners and I had changed the product behavior there.

Fixes:

  1. I worked around the EventListener bug by explicitly calling DisableEvents().
    I also updated OnEventWritten to log the MultipleSessionsNotSupportedError
    so that any future error that is similar has more obvious diagnostic logging.
  2. I updated the test with new expectation that the 1st listener continues normal
    operation and the 2nd listener is the one that gets rejected.

I also did a little refactoring and added a 2nd tests of multiple listeners to confirm
it works if the first one is disabled before the 2nd one starts.

@ghost
Copy link

ghost commented Jul 27, 2021

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Last week I added a change to the error handling behavior when MetricEventSource
is enabled by multiple listeners and didn't properly update the tests.

  1. The major issue was timout failures which were being caused because there is
    a bug in EventListener where it doesn't notify EventSources that the source has
    been disabled when the listener is disposed. This in turn caused every tests after
    the first to reject the new EventListener because the EventSource believed it was
    still in use by the first EventListener.
    (EventListener bug is tracked Disposing EventListener doesn't send disable command #56378)
  2. A secondary issue is that I didn't update the test which was explicitly verifying
    the behavior where the EventSource emits an error in response to having two
    listeners and I had changed the product behavior there.

Fixes:

  1. I worked around the EventListener bug by explicitly calling DisableEvents().
    I also updated OnEventWritten to log the MultipleSessionsNotSupportedError
    so that any future error that is similar has more obvious diagnostic logging.
  2. I updated the test with new expectation that the 1st listener continues normal
    operation and the 2nd listener is the one that gets rejected.

I also did a little refactoring and added a 2nd tests of multiple listeners to confirm
it works if the first one is disabled before the 2nd one starts.

Author: noahfalk
Assignees: -
Labels:

area-System.Diagnostics.Tracing
Milestone: -

@noahfalk
Copy link
Member Author

@tarekgh @hoyosjs

@noahfalk noahfalk mentioned this pull request Jul 27, 2021
Closed
stephentoub
stephentoub reviewed Jul 27, 2021
View reviewed changes
src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs Outdated
protected override void OnEventWritten(EventWrittenEventArgs eventData)
{
string sessionId = eventData.Payload[0].ToString();
if(sessionId != "" && sessionId != _sessionId)
if(eventData.EventName != "MultipleSessionsNotSupportedError" && sessionId != "" && sessionId != _sessionId)
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
if(eventData.EventName != "MultipleSessionsNotSupportedError" && sessionId != "" && sessionId != _sessionId)
if (eventData.EventName != "MultipleSessionsNotSupportedError" && sessionId != "" && sessionId != _sessionId)

@stephentoub
Copy link
Member

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@stephentoub
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@noahfalk
Copy link
Member Author

I confirmed metrics tests are passing in the failing outerloop legs

@noahfalk
Fix MetricsEventSource tests
738668e 
Last week I added a change to the error handling behavior when MetricEventSource
is enabled by multiple listeners and didn't properly update the tests.
1. The major issue was timout failures which were being caused because there is
a bug in EventListener where it doesn't notify EventSources that the source has
been disabled when the listener is disposed. This in turn caused every tests after
the first to reject the new EventListener because the EventSource believed it was
still in use by the first EventListener.
(EventListener bug is tracked dotnet#56378)
2. A secondary issue is that I didn't update the test which was explicitly verifying
the behavior where the EventSource emits an error in response to having two
listeners and I had changed the product behavior there.

Fixes:
1. I worked around the EventListener bug by explicitly calling DisableEvents().
I also updated OnEventWritten to log the MultipleSessionsNotSupportedError
so that any future error that is similar has more obvious diagnostic logging.
2. I updated the test with new expectation that the 1st listener continues normal
operation and the 2nd listener is the one that gets rejected.

I also did a little refactoring and added a 2nd tests of multiple listeners to confirm
it works if the first one is disabled before the 2nd one starts.
@noahfalk noahfalk merged commit 392ff31 into dotnet:main Jul 27, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into improve_hot_reload_de…
1930315 
…bug_tests

* origin/main: (274 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_js_tests
3f83a08 
* origin/main: (95 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tarekgh tarekgh tarekgh approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Diagnostics.Tracing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@noahfalk @stephentoub @tarekgh