Fix MetricsEventSource tests by noahfalk · Pull Request #56382 · dotnet/runtime

noahfalk · 2021-07-27T13:05:37Z

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.

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)
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:

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.
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 · 2021-07-27T13:05:43Z

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.

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)
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:

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.
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 · 2021-07-27T13:06:06Z

@tarekgh @hoyosjs

stephentoub · 2021-07-27T13:13:15Z

src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs

Nit:

Suggested change

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

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

stephentoub · 2021-07-27T13:13:33Z

/azp list

azure-pipelines · 2021-07-27T13:13:38Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo coreclr-gc-regions

stephentoub · 2021-07-27T13:13:49Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-07-27T13:14:04Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk · 2021-07-27T20:12:44Z

I confirmed metrics tests are passing in the failing outerloop legs

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.

…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) ...

* 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 added the area-System.Diagnostics.Tracing label Jul 27, 2021

noahfalk mentioned this pull request Jul 27, 2021

System.Diagnostics.Metrics.Tests.MetricEventSourceTests failing from event waiting timeouts #56280

Closed

stephentoub reviewed Jul 27, 2021

View reviewed changes

stephentoub approved these changes Jul 27, 2021

View reviewed changes

tarekgh approved these changes Jul 27, 2021

View reviewed changes

noahfalk force-pushed the fix_metrics_tests branch from 0062718 to 738668e Compare July 27, 2021 20:17

noahfalk merged commit 392ff31 into dotnet:main Jul 27, 2021

ghost locked as resolved and limited conversation to collaborators Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MetricsEventSource tests#56382

Fix MetricsEventSource tests#56382
noahfalk merged 1 commit intodotnet:mainfrom
noahfalk:fix_metrics_tests

noahfalk commented Jul 27, 2021

Uh oh!

ghost commented Jul 27, 2021

Uh oh!

noahfalk commented Jul 27, 2021

Uh oh!

stephentoub Jul 27, 2021

Uh oh!

stephentoub commented Jul 27, 2021

Uh oh!

azure-pipelines bot commented Jul 27, 2021

Uh oh!

stephentoub commented Jul 27, 2021

Uh oh!

azure-pipelines bot commented Jul 27, 2021

Uh oh!

noahfalk commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Conversation

noahfalk commented Jul 27, 2021

Uh oh!

ghost commented Jul 27, 2021

Uh oh!

noahfalk commented Jul 27, 2021

Uh oh!

stephentoub Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jul 27, 2021

Uh oh!

azure-pipelines bot commented Jul 27, 2021

Uh oh!

stephentoub commented Jul 27, 2021

Uh oh!

azure-pipelines bot commented Jul 27, 2021

Uh oh!

noahfalk commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants