-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Add log level and keyword tests for NativeRuntimeEventSource #89816
Add log level and keyword tests for NativeRuntimeEventSource #89816
Conversation
Test failures seem to be due to a deadlock/hang in the test with the following callstacks. @davmason is this related to dotnet/diagnostics#4825?
|
Possible workaround in the test - #86370 |
Yeah, the previous failure is #86838. Now that the workaround is in we shouldn't see it anymore. There are two current failures
|
src/tests/tracing/runtimeeventsource/NativeRuntimeLevelKeywords.cs
Outdated
Show resolved
Hide resolved
src/tests/tracing/runtimeeventsource/NativeRuntimeLevelKeywords.cs
Outdated
Show resolved
Hide resolved
@n77y, it looks like you're iterating on the test so I haven't looked at your recent changes. When you would like me to take a look please let me know |
@davmason, feel free to take a look when you get a chance. I have been running into issues with even targeting one or two events. In some cases, all the listeners will pick up no events. This appears to coincide with specific builds. My latest commit should only fail when there is some incongruency with what event levels the listeners receive. For example, the test will fail if the LogAlways listener is missing any events found by the Verbose listener, or if the Informational listener picked up any Verbose events that would also be a big no no. |
Build windows-x64 Checked NativeAOT failed with null reference. |
|
||
Console.WriteLine($"\n{nameof(stopwatch.Elapsed.TotalSeconds)} {stopwatch.Elapsed.TotalSeconds}"); | ||
|
||
WarningIfNoEventsAtLevel(informational); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No one will read the output of the tests unless there is a failure, so a warning is effectively the same as doing nothing. It seems like we should fail the test if there are no events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few platforms are failing with no OnEventWritten()
's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to add some logic to see if its an issue with the ConcurrentDictionary
.
} | ||
GC.Collect(); | ||
|
||
if (informational.Contains("GCStart_V2") && verbose.Contains("GCAllocationTick_V4")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should pick some events that is supported by Mono runtime? The GCStart_V2 is supported when requesting full GC dumps (but currently won't be emitted if you just use event mask 1) and GCAllocationTick_V4 is not supported at all on Mono runtime. https://github.com/dotnet/runtime/blob/main/src/mono/mono/eventpipe/gen-eventing-event-inc.lst lists the native runtime event currently supported by Mono runtime. Mono runtime also supports all managed events emitted through EventSource implementations in BCL (and custom defined EventSource classes). If you don't find any suitable events around supported once, you will need to exclude this test on Mono runtime altogether, but would be really good to test it on Mono runtime as well. One option could be to request a GC dump and collect generated events from that, see https://github.com/dotnet/runtime/blob/main/src/tests/tracing/eventpipe/gcdump/gcdump.cs, that test runs on Mono desktop platforms. You can also look in issues.targets for that test and make sure it disabled on the same set of platforms to make sure it won't break runtime extra platforms lane on CI.
@n77y thanks for the contribution on this PR! Please let us know if you are planning on addressing the feedback in the short term. Otherwise we will plan on setting the PR status to "draft". |
@n77y I’ve changed this PR to draft mode. Please feel free to move change the status once work resumes on it. |
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
Tests are designed to catch possible regressions for
EventLevel.LogAlways
&EventKeywords.None
after #89326.EventKeywords.None
sould be equivalent toEventKeywords.All
.EventLevel.LogAlways
sould be equivalent to or greater thenEventLevel.Verbose
.