Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stephentoub opened this issue Nov 15, 2022 · 8 comments · May be fixed by dotnet/roslyn-analyzers#7290
Open
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Logging code-analyzer Marks an issue that suggests a Roslyn analyzer help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Nov 15, 2022

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

@stephentoub stephentoub added the code-analyzer Marks an issue that suggests a Roslyn analyzer label Nov 15, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@ghost
Copy link

ghost commented Nov 15, 2022

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 tarekgh added this to the Future milestone Nov 15, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@tarekgh
Copy link
Member

tarekgh commented Nov 15, 2022

CC @Youssef1313

@Youssef1313
Copy link
Member

Happy to implement once the analyzer is approved.

@buyaa-n buyaa-n modified the milestones: Future, 8.0.0 Nov 16, 2022
@stephentoub stephentoub added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 21, 2022
@tarekgh tarekgh added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 21, 2022
@jeffhandley jeffhandley added the partner-impact This issue impacts a partner who needs to be kept updated label Nov 21, 2022
@bartonjs
Copy link
Member

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

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 22, 2022
@buyaa-n buyaa-n removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 19, 2022
@Youssef1313
Copy link
Member

@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
Copy link
Member

tarekgh commented May 19, 2023

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

@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Jun 1, 2023
@carlossanlop carlossanlop added the in-pr There is an active PR which will close this issue when it is merged label Jun 17, 2023
@layomia layomia removed this from the 8.0.0 milestone Aug 2, 2023
@mpidash
Copy link
Contributor

mpidash commented Apr 3, 2024

I'd like to finish this analyzer :)

@tarekgh
Copy link
Member

tarekgh commented Apr 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Logging code-analyzer Marks an issue that suggests a Roslyn analyzer help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants