Analyzer proposal: EventSource log argument guarding

[stephentoub]

Background and Motivation

The typical pattern with EventSource is to have an EventSource-derived type that exposes one method per event; arguments to these method calls are then generally included in some form as part of the event payload. With the goal of logging being a nop / as cheap as possible when it's not enabled, any expensive work should be guarded by checks for whether the EventSource is actually enabled, e.g.

if (MyEventSource.Log.IsEnabled())
{
    MyEventSource.Log.SomethingInterestingHappened(ComputeArgument());
}

but it's easy to accidentally forget such a guard. dotnet/aspnetcore#27956 has examples of cases in ASP.NET that we shipped in .NET 5, where unguarded log calls were doing fairly expensive work, e.g.

Log.DescribeFoundCertificates(ToCertificateDescription(matchingCertificates));

Proposed Analyzer

We won't be able to catch all such uses perfectly, but we should be able to flag many, with a reasonably low false positive rate, e.g. find calls to SomeEventSource.Log.SomeMethod, and if any argument isn't a field/local or some property access off of one (making the assumption that properties are cheap), then find if there's a SomeEventSource.Log.IsEnabled() check guarding it, and if there isn't, warn there should be. An auto-fixer could add one.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

The typical pattern with EventSource is to have an EventSource-derived type that exposes one method per event; arguments to these method calls are then generally included in some form as part of the event payload. With the goal of logging being a nop / as cheap as possible when it's not enabled, any expensive work should be guarded by checks for whether the EventSource is actually enabled, e.g.

if (MyEventSource.Log.IsEnabled())
{
    MyEventSource.Log.SomethingInterestingHappened(ComputeArgument());
}

but it's easy to accidentally forget such a guard. dotnet/aspnetcore#27956 has examples of cases in ASP.NET that we shipped in .NET 5, where unguarded log calls were doing fairly expensive work, e.g.

Log.DescribeFoundCertificates(ToCertificateDescription(matchingCertificates));

Proposed Analyzer

We won't be able to catch all such uses perfectly, but we should be able to flag many, with a reasonably low false positive rate, e.g. find calls to SomeEventSource.Log.SomeMethod, and if any argument isn't a field/local or some property access off of one (making the assumption that properties are cheap), then find if there's a SomeEventSource.Log.IsEnabled() check guarding it, and if there isn't, warn there should be. An auto-fixer could add one.

Author:	stephentoub
Assignees:	-
Labels:	api-suggestion, area-System.Diagnostics.Tracing, code-analyzer, code-fixer
Milestone:	6.0.0

[tommcdon]

@sywhang