[Analyzer] Usage of ToString in logging source generator method call sites

[stephentoub]

We should consider an analyzer that flags use of ToString() in call sites to logging methods and suggests reconsidering the shape of the logging method such that it takes a strongly-typed value. In many situations, logging is disabled or set at a log level that will result in a particular event being disabled, and doing the ToString() at the call site is then often unnecessary allocation / expense. For example, given:

[LoggerMessage(EventId = 0, Level = LogLevel.Information, Message = "Excessively large timeout `{timeout}`")]
public static partial void ExcessivelyLargeTimeout(this ILogger logger, string timeout);
...
TimeSpan timeout = ...;
logger.ExcessivelyLargeTimeout(timeout.ToString());

the analyzer would flag the .ToString() and suggest the ExcessivelyLargeTimeout method should be changed to take a TimeSpan instead of a string, with the .ToString() removed from the call site.

Performance rules Category
Severity = suggestion

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We should consider an analyzer that flags use of ToString() in call sites to logging methods and suggests reconsidering the shape of the logging method such that it takes a strongly-typed value. In many situations, logging is disabled or set at a log level that will result in a particular event being disabled, and doing the ToString() at the call site is then often unnecessary allocation / expense. For example, given:

[LoggerMessage(EventId = 0, Level = LogLevel.Information, Message = "Excessively large timeout `{timeout}`")]
public static partial void ExcessivelyLargeTimeout(this ILogger logger, string timeout);
...
TimeSpan timeout = ...;
logger.ExcessivelyLargeTimeout(timeout.ToString());

the analyzer would flag the .ToString() and suggest the ExcessivelyLargeTimeout method should be changed to take a TimeSpan instead of a string, with the .ToString() removed from the call site.

Author:	stephentoub
Assignees:	-
Labels:	code-analyzer, area-Extensions-Logging
Milestone:	-

[tarekgh]

CC @Youssef1313

[Youssef1313]

Happy to implement once the analyzer is approved.

[bartonjs]

Analyzer would detect when "work" is being done in an argument call to an ILogger logging method. This basically boils down to anything other than a literal-expression, local-expression, parameter-expression, field-expression, property-expression, or indexer-expression (potentially recursively applied).

Note that this list includes identifying any parameters being wrapped in a new array to a params method call.

The analyzer should not fire if it identifies that it is in a scope that has already checked on log levels.

A logging method is any of:

• A method decorated with [LoggerMessage]
• Any of the extension methods on Microsoft.Extensions.Logging.LoggerExtensions
• A call to Microsoft.Extensions.Logging.ILogger.Log
• Anything else that looks like a logging method that we didn't think of today

Category: Performance
Severity: Suggestion

[Youssef1313]

@buyaa-n I did a quick (not yet tested) prototype (dotnet/roslyn-analyzers#6643).

For now, I don't think I'll be able to complete it soon.

If anyone wants to pick it and continue on the code I wrote, this is totally fine.

Currently, the PR is missing:

• Tests

• The analyzer should not fire if it identifies that it is in a scope that has already checked on log levels.

• Fix any bad logic I could have wrote.

[tarekgh]

@Youssef1313 thanks for letting us know and having a draft PR we can use to build this analyzer.

[mpidash]

I'd like to finish this analyzer :)

[tarekgh]

@mpidash thanks for willing to help with that. Please ensure doing it as described in #78402 (comment).