-
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 event to RuntimeEventSource for AppContext switches #57303
Changes from all commits
e0a1602
c6503d3
c52f044
8002ac9
451ee4b
6dd83dc
bc59e00
737492b
4facebf
b0235f8
58a87f5
5d4060c
1044a0f
f352689
6a62a27
fad8256
8989c1b
01288ae
0da1677
740a750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,11 @@ internal sealed partial class RuntimeEventSource : EventSource | |
{ | ||
internal const string EventSourceName = "System.Runtime"; | ||
|
||
public static class Keywords | ||
{ | ||
public const EventKeywords AppContext = (EventKeywords)0x1; | ||
} | ||
|
||
private static RuntimeEventSource? s_RuntimeEventSource; | ||
private PollingCounter? _gcHeapSizeCounter; | ||
private IncrementingPollingCounter? _gen0GCCounter; | ||
|
@@ -50,6 +55,17 @@ public static void Initialize() | |
// as you can't make a constructor partial. | ||
private RuntimeEventSource(int _) { } | ||
|
||
private enum EventId : int | ||
{ | ||
AppContextSwitch = 1 | ||
} | ||
|
||
[Event((int)EventId.AppContextSwitch, Level = EventLevel.Informational, Keywords = Keywords.AppContext)] | ||
internal void LogAppContextSwitch(string switchName, int value) | ||
agocke marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
base.WriteEvent((int)EventId.AppContextSwitch, switchName, value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
protected override void OnEventCommand(EventCommandEventArgs command) | ||
{ | ||
if (command.Command == EventCommand.Enable) | ||
agocke marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -87,6 +103,8 @@ protected override void OnEventCommand(EventCommandEventArgs command) | |
_ilBytesJittedCounter ??= new PollingCounter("il-bytes-jitted", this, () => System.Runtime.JitInfo.GetCompiledILBytes()) { DisplayName = "IL Bytes Jitted", DisplayUnits = "B" }; | ||
_methodsJittedCounter ??= new PollingCounter("methods-jitted-count", this, () => System.Runtime.JitInfo.GetCompiledMethodCount()) { DisplayName = "Number of Methods Jitted" }; | ||
_jitTimeCounter ??= new IncrementingPollingCounter("time-in-jit", this, () => System.Runtime.JitInfo.GetCompilationTime().TotalMilliseconds) { DisplayName = "Time spent in JIT", DisplayUnits = "ms", DisplayRateTimeScale = new TimeSpan(0, 0, 1) }; | ||
|
||
AppContext.LogSwitchValues(this); | ||
josalem marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just logging values at a specific point in time (when the event source is enabled) - is that the desire? I'm thinking of the possibility of scenarios like:
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think most of those are edge cases. The 90% usage is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely agree that those should not be the common case, but wanted to call it out.
I think part of my question is what does this end up looking like when tracing startup (where I'd expect the event source is enabled before the app's entry is is run)? Could it end up being the first scenario I had above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, @brianrob @josalem do event sources OnCommand run before Main executes when you attach the event source at startup (which not many do). @elinor-fung These are great questions BTW 👍🏾 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They definitely can, but not all will. If the EventSource comes into existence before Main is called and it is told to turn on (e.g. EventPipe, ETW, or some EventListener tells it to), then it will trigger before Main. By design, there isn't anything that requires that Main run before events can be emitted. A good example here is FrameworkEventSource which often comes into existence before Main. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To add to this, if you want to ensure that these can be emitted early, make sure that the EventSource that emits them comes into existence early. That may already be the case, but if not, make sure it gets instantiated early, even if it doesn't log any events when instantiated. Just the instantiation will allow it to be enabled immediately (at least by ETW, but shortly by EventPipe once it comes up). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elinor-fung, just read your first scenario - I think that the right way to handle a race between logging the current state and changes to switches is to dump the set of switches at the beginning of the tracing session as this code does. Additionally, any time a switch is configured, it should be logged immediately. This ensures that if switches change value or get set after the session is created, we see the change. This, combined with instantiating the EventSource early will ensure that we don't miss any data. |
||
} | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.IO; | ||
using System.Diagnostics.Tracing; | ||
using System.Runtime.CompilerServices; | ||
using System.Threading; | ||
using Tracing.Tests.Common; | ||
|
||
namespace Tracing.Tests | ||
{ | ||
public sealed class NativeRuntimeEventSourceTest | ||
{ | ||
static int Main(string[] args) | ||
{ | ||
SimpleEventListener.EnableKeywords = (EventKeywords)0; | ||
using (SimpleEventListener noEventsListener = new SimpleEventListener("NoEvents")) | ||
{ | ||
// Create an EventListener. | ||
SimpleEventListener.EnableKeywords = (EventKeywords)0x4c14fccbd; | ||
using (SimpleEventListener listener = new SimpleEventListener("Simple")) | ||
{ | ||
// Trigger the allocator task. | ||
System.Threading.Tasks.Task.Run(new Action(Allocator)); | ||
|
||
// Wait for events. | ||
Thread.Sleep(1000); | ||
|
||
// Generate some GC events. | ||
GC.Collect(2, GCCollectionMode.Forced); | ||
|
||
// Wait for more events. | ||
Thread.Sleep(1000); | ||
|
||
// Ensure that we've seen some events. | ||
Assert.True("listener.EventCount > 0", listener.EventCount > 0); | ||
} | ||
|
||
// Generate some more GC events. | ||
GC.Collect(2, GCCollectionMode.Forced); | ||
|
||
// Ensure that we've seen no events. | ||
Assert.True("noEventsListener.EventCount == 0", noEventsListener.EventCount == 0); | ||
} | ||
|
||
return 100; | ||
} | ||
|
||
private static void Allocator() | ||
{ | ||
while (true) | ||
{ | ||
for(int i=0; i<1000; i++) | ||
GC.KeepAlive(new object()); | ||
|
||
Thread.Sleep(10); | ||
} | ||
} | ||
} | ||
|
||
internal sealed class SimpleEventListener : EventListener | ||
{ | ||
private string m_name; | ||
|
||
// Keep track of the set of keywords to be enabled. | ||
public static EventKeywords EnableKeywords | ||
{ | ||
get; | ||
set; | ||
} | ||
|
||
public SimpleEventListener(string name) | ||
{ | ||
m_name = name; | ||
} | ||
|
||
public int EventCount { get; private set; } = 0; | ||
|
||
protected override void OnEventSourceCreated(EventSource eventSource) | ||
{ | ||
if (eventSource.Name.Equals("Microsoft-Windows-DotNETRuntime")) | ||
{ | ||
if (EnableKeywords != 0) | ||
{ | ||
// Enable events. | ||
EnableEvents(eventSource, EventLevel.Verbose, EnableKeywords); | ||
} | ||
else | ||
{ | ||
// Enable the provider, but not any keywords, so we should get no events as long as no rundown occurs. | ||
EnableEvents(eventSource, EventLevel.Critical, EnableKeywords); | ||
} | ||
} | ||
} | ||
|
||
protected override void OnEventWritten(EventWrittenEventArgs eventData) | ||
{ | ||
Console.WriteLine($"[{m_name}] ThreadID = {eventData.OSThreadId} ID = {eventData.EventId} Name = {eventData.EventName}"); | ||
Console.WriteLine($"TimeStamp: {eventData.TimeStamp.ToLocalTime()}"); | ||
Console.WriteLine($"LocalTime: {DateTime.Now}"); | ||
Console.WriteLine($"Difference: {DateTime.UtcNow - eventData.TimeStamp}"); | ||
Assert.True("eventData.TimeStamp <= DateTime.UtcNow", eventData.TimeStamp <= DateTime.UtcNow); | ||
for (int i = 0; i < eventData.Payload.Count; i++) | ||
{ | ||
string payloadString = eventData.Payload[i] != null ? eventData.Payload[i].ToString() : string.Empty; | ||
Console.WriteLine($"\tName = \"{eventData.PayloadNames[i]}\" Value = \"{payloadString}\""); | ||
} | ||
Console.WriteLine("\n"); | ||
|
||
EventCount++; | ||
} | ||
} | ||
} |
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.
@brianrob Would a single event with all of the data be better for something like this?
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 think one event per switch is good. It makes it easy to consume the data, and I can't imagine we'll have so many that the cost of this event will be painful. If/when we choose to create events were we dump data in bulk, it makes both the event production and event consumption code more complex. On the production side, you have to chunk the data to ensure that you don't run up against size limitations (e.g. ETW has a 64K size limit per event). On the consumption side, you have to be more sophisticated in how you parse the event. There are definitely places were it's valuable - for example BulkType events in the runtime when we log heap snapshots. There are just tons of types, and so emitting each one in its own event is very costly.