Add ETW events for recovering syntax trees #38194
Conversation
| return RecoverRoot(stream, cancellationToken); | ||
| if (RoslynEventSource.Instance.IsEnabled(EventLevel.Informational, EventKeywords.None)) | ||
| { | ||
| RoslynEventSource.Instance.BlockStart(_containingTree.FilePath, FunctionId.Workspace_Recoverable_RecoverRootAsync, blockId: 0); |
There was a problem hiding this comment.
Is there a reason to keep using RoslynEventSource.Instance vs. capturing it in a local?
There was a problem hiding this comment.
Not especially. I just used the pattern seen in similar features for other libraries we ship.
| } | ||
| finally | ||
| { | ||
| if (RoslynEventSource.Instance.IsEnabled(EventLevel.Informational, EventKeywords.None)) |
There was a problem hiding this comment.
Seems odd at a glance to check this here. Naiively I'd say that if the check for IsEnabled(EventLevel.Informational, EventKeywords.None)) was true inside the try block then we should execute the body of this block period. Not re-check the value again. Checking the value in both places just seems to open the possibility that we'd have unmatched data points in the trace (start but no end, end but no start, etc ...)
There was a problem hiding this comment.
Mismatched events is always a possibility (sometimes events get dropped by the OS before we even have a chance to log them). The pattern used here was modeled after the pattern used in vs-threading, e.g. https://github.com/microsoft/vs-threading/blob/e213c20ec8d58d5a32e23739103ccfc22bccbc89/src/Microsoft.VisualStudio.Threading/JoinableTask.cs#L841-L891. I'm not sure what impact deviating from this would have, but I can give one example of where the current behavior might be desired: if the user stops a performance trace while one or more trees is in the process of being deserialized.
There was a problem hiding this comment.
Also on the other side, if a user starts a performance trace while the tree is being deserialized, we will see the time it took to deserialize that tree even though it started before the performance trace started capturing data.
Closes #37477
The steps for manually recording traces have already been updated to enable the provider.