-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Closed
Labels
api-approvedAPI was approved in API review, it can be implementedAPI was approved in API review, it can be implementedarea-Extensions-Loggingcode-analyzerMarks an issue that suggests a Roslyn analyzerMarks an issue that suggests a Roslyn analyzerenhancementProduct code improvement that does NOT require public API changes/additionsProduct code improvement that does NOT require public API changes/additions
Milestone
Description
Scope for analyzer
With these set of analyzers the the developers would be able to use the logging APIs at runtime in a more guided way.
For example, this is related to LoggerExtensions, APIs such as BeginScope, LogTrace, LogDebug, etc. Also APIs such as LoggerMessage Define and DefineScope.
- There is no fixer for any of these diagnostics.
List of proposed diagnostics
| Diagnostic | Severity | Category | Purpose/Message |
|---|---|---|---|
| DIAG_1 | Hidden | Naming | Use PascalCase for log message tokens |
| DIAG_2 | Hidden | Performance | For improved performance, use pre-compiled log messages instead |
| DIAG_3 | Information | Usage | Numerics should not be used in logging format string |
| DIAG_4 | Information | Usage | Logging format string should not be dynamically generated |
| DIAG_5 | Warning | Reliability | Logging format string parameter count mismatch |
Usage samples:
DIAG_1: Use PascalCase for log message tokens
logger.BeginScope("{camelCaseVsPascalCase}", "1");
// Resolution:
// logger.BeginScope("{CamelCaseVsPascalCase}", "1");DIAG_2: For improved performance, use pre-compiled log messages instead
logger.LogTrace("This is a test {Message}", "Foo");
// Resolution:
// Action<ILogger, string, Exception?> precompiledLogMessage = LoggerMessage.Define<string>(LogLevel.Trace, 42, "This is a test {Message}" ); // where 42 is its event id
// precompiledLogMessage(logger, "Foo", null);DIAG_3: Numerics should not be used in logging format string
logger.LogTrace("{0}", 1); // Produces both `DIAG_3` and `DIAG_2`
// Resolution for `DIAG_3`
// logger.LogTrace("{One}", 1);DIAG_4: Logging format string should not be dynamically generated
This would be forLogTraceand similar APIs.
logger.LogTrace("message with {argument1}" + 2);
logger.LogTrace("{string.Empty}");
// Resolution: make sure last argument is not dynamically generatedDIAG_5: Logging format string parameter count mismatch
LoggerMessage.Define<int>(LogLevel.Information, 42, "{One} {Two} {Three}");
// Resolution:
// either: LoggerMessage.Define<int>(LogLevel.Information, 42, "{One}");
// or: LoggerMessage.Define<int, int, int>(LogLevel.Information, 42, "{One} {Two} {Three}");- Why warning for
DIAG_5? when this code snippet runs we get this exception thrown at runtime:
System.ArgumentException: 'The format string '{One} {Two} {Three}' does not have the expected number of named parameters. Expected 1 parameter(s) but found 3 parameter(s).'
Link to code
The latest PR is available at dotnet/roslyn-analyzers#5244
The code was originally prototyped as preview in https://github.com/dotnet/extensions/tree/master/src/Logging/Logging.Analyzers/src but has not yet been shipped analyzers.
Original Description
Consider adding analyzers for DefineMessage
Reference analyzers from our projects
Decide how are they going to be shipped
/cc @glennc
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
api-approvedAPI was approved in API review, it can be implementedAPI was approved in API review, it can be implementedarea-Extensions-Loggingcode-analyzerMarks an issue that suggests a Roslyn analyzerMarks an issue that suggests a Roslyn analyzerenhancementProduct code improvement that does NOT require public API changes/additionsProduct code improvement that does NOT require public API changes/additions