dotnet/runtime

Analyzer proposal: EventSource log argument guarding

Open

#45,215 opened on Nov 25, 2020

View on GitHub
 (2 comments) (0 reactions) (0 assignees)C# (5,445 forks)batch import
api-suggestionarea-System.Diagnostics.Tracingcode-analyzercode-fixerfeature-requesthelp wanted

Repository metrics

Stars
 (17,886 stars)
PR merge metrics
 (Avg merge 12d 11h) (661 merged PRs in 30d)

Description

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. https://github.com/dotnet/aspnetcore/pull/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.

Contributor guide