Remove AsyncCausalityTracer by logging into TplEventSource directly #38313
Conversation
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/AsyncCausalityTracer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/AsyncCausalityTracer.cs
Outdated
Show resolved
Hide resolved
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); |
There was a problem hiding this comment.
What exceptions might get thrown? I'm not clear on why the try/catch is needed here and elsewhere.
There was a problem hiding this comment.
All these may be a left-over from the original WinRT AsyncCausalityTracer implementation and may not be needed anymore.
| } | ||
|
|
||
| internal static bool LoggingOn => listenerCnt > 0; | ||
| private static int listenerCnt = 0; |
|
|
||
| namespace System.Threading.Tasks | ||
| { | ||
| internal static class AsyncCausalityTracer |
There was a problem hiding this comment.
Do we need the AsyncCausalityTracer type at all at this point? Could call sites just be updated to use TplEventSource directly, as with all of the other call sites to it?
There was a problem hiding this comment.
We don't really need it since the WinRT dependent component from this class is removed - we could just update all the call sites to use TplEventSource directly. Would that be something you'd prefer over adding AsyncCausalityTracer back?
There was a problem hiding this comment.
Would that be something you'd prefer over adding AsyncCausalityTracer back?
Yes, that would be better - especially once the other cruft is deleted and these methods become trivial forwarders.
There was a problem hiding this comment.
Agreed. We can look at TplEventSource if we want to understand the full list of events that can be emitted. I'd only argue for keeping AsyncCausalityTracer if it were holding state, but that doesn't appear to be the case.
| // we should catch and log exceptions but never propagate them. | ||
| private static void LogAndDisable(Exception ex) | ||
| { | ||
| Disable(); |
There was a problem hiding this comment.
I'm not clear why this is the right thing to do. If an exception occurs (and per my previous comment, I'm not sure why/when that would happen), we decrement the ref count, but we don't actually unregister anything. Doesn't that suggest that the count could just keep decreasing and decreasing? That would seem to a) mean you couldn't re-enable it eventually, and b) in an extreme case, it could wrap around and we'd end up seeing it as enabled again?
There was a problem hiding this comment.
An exception can be thrown when TplEventSource tries to write events into ETW and an error occurs (i.e. not enough memory). That being said, I'm not entirely sure why we should catch that exception and disable... Like Jan said, it is a remnant from the old WinRT AsyncCausalityTracer. I suppose we can remove this unless @noahfalk has any other thoughts?
There was a problem hiding this comment.
I think it would be fine to remove these exception catches. I don't see any reason why an exception that was acceptable to throw from TplEventSource would be unacceptable to throw here.
sywhang
changed the title
ghost
locked as resolved and limited conversation to collaborators
For #37945
This PR adds a new cross-platform version of the old AsyncCausalityTracer which was removed in #36715 .The cross-platform AsyncCausalityTracer is a subset of the old AsyncCausalityTracer and logs events through TplEventSource, and not the Windows AsyncCausalityTracer, removing the need for COM-Interop/WinRT support.[Edit] After discussion, the PR has been modified to remove AsyncCausalityTracer and change all the call sites to call TplEventSource directly.
I've verified the VS/PerfView scenarios that depend on this feature works again with this fix. I've also verified that TplEventSource/TraceOperation/Start and TplEventSource/TraceOperation/Stop events are now getting emitted on Linux and macOS.
cc @noahfalk @stephentoub @brianrob