-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Flow EventSources to EventPipe on Windows #18217
Flow EventSources to EventPipe on Windows #18217
Conversation
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
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.
This looks OK. Can you add some testing where both EventPipe and ETW are turned on at the same time (and are subscribing to overlapping sets of events and are fiddling with their controls (and showing that it does not affect the other).
#endif // FEATURE_MANAGED_ETW | ||
|
||
if (m_Dispatchers != null && m_eventData[eventId].EnabledForAnyListener) | ||
if (m_Dispatchers != null && m_eventData[eventId].EnabledForAnyListener) |
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.
nit, this indentation is off (tabs vs spaces?)
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.
Fixed.
if (m_provider != null) | ||
m_eventData[eventId].EnabledForETW = value; | ||
if (m_etwProvider != null) | ||
m_eventData[eventId].EnabledForBlobSerializedListeners = value; |
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 don't see where we set EnabledForBlobSerializedListeners to true for the EventPipe case.
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.
Per our offline conversation, I have fixed this by having a boolean for each source (EventPipe and ETW) so that disabling one does not disable the other.
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've also added a test to cover this case.
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.
LGTM!
{ | ||
ThrowEventSourceException(eventName); | ||
} | ||
else |
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.
nit: could be just else if
(not an extra if nested inside else block)
Thanks @adamsitnik and @vancem for the reviews. Note, I have also validated EventSource tests in CoreFX are passing with these changes. |
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Fixes #15494.
Modifies EventSource to be able to concurrently write to both ETW and EventPipe. This is done by creating two EventProvider objects - one for ETW and the other for EventPipe and using them both when writing events.
NOTE: The change to EventSourceTrace.cs is just to put the EventSource in a using block so that we test disposal of the EventSource. The diff on GitHub shows much more of a change than was actually made due to whitespace differences.