Finalize logging analyzers

[pakrym]

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 for LogTrace and similar APIs.

logger.LogTrace("message with {argument1}" + 2);
logger.LogTrace("{string.Empty}");
// Resolution: make sure last argument is not dynamically generated

---

• DIAG_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

[paulomorgado]

Where are these analyzers published?

[pakrym]

Right now they are published to https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json feed. https://dotnet.myget.org/feed/aspnetcore-ci-dev/package/nuget/Microsoft.Extensions.Logging.Analyzers

[paulomorgado]

Will they work on VS2015?

[paulomorgado]

@pakrym, still no plans to release this?

[pakrym]

No, not in 2.1

[paulomorgado]

😢

[Eilon]

@pakrym - are we shipping them in "experimental" form so that people can easily try them out? I thought I heard someone say that the other day.

[pakrym]

Yes, we would continue to ship them in the same form they are now, but wouldn't do the same thing we did with MVC and EF analyzers (reference from packages themselves)

[Genbox]

I think these analyzers should be a little more extensive than they are now. It would also be great if they supported structured logging templates. The structured message analyzer rule can be 'none' severity by default and only enabled if users want them.

[penenkel]

any updates on this?

[JanEggers]

@pakrym any updates on this one? can you please point me to the current nuget feed used for nightly builds? I didnt find any info on this in the readme and the link above does not work (vs shows no packages) and the link to the package asks for credentials.

[pakrym]

@anurse might have more information on this.

[analogrelay]

I believe the packages should be on the dotnet-core nightly feed: https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json

I don't have specific updates on the progress here, but it will be considered as part of 5.0 planning.

[analogrelay]

Could this tie in to the analyzer effort you're working on @bartonjs @jeffhandley ? These are analyzers currently produced in extensions (though the logging stuff is now owned by the runtime) that we don't currently ship (they're marked as non-shipping packages) but would be useful for anyone using Microsoft.Extensions.Logging.

[jeffhandley]

Hey @anurse - sorry for the delayed response. Our review process is currently fairly narrow as we were only planning to ship a small set of analyzers for now and dial it up later, and I don't expect we'll add any more analyzers into the .NET Libraries team's backlog for 5.0. Were you just thinking you want to get these into the backlog, or did you want to contribute them into a productized shipping vehicle?

[jons-aura]

Would this issue be at all related to Suchiman/SerilogAnalyzer#15 ?

[poke]

Hey, is there any chance that we could publish a preview version of the logging analyzers on NuGet? I’m totally fine with the current feature set and would just like to consume them, ideally without having to rely on a separate nightly feed.

[maryamariyan]

I tested out the logging analyzer, and seems like it mostly has Information level diagnostics (3 of them I think), and the one warning level diagnostics is checking for mismatch in arguments provided in the message template. This is a kind of analytics already provided by the source generator we are working on in issue #36022. So it makes sense to defer shipping this analyzer to after until we first have the logging generator in.

[maryamariyan]

@shirhatti should we close this given that we have logging generator now? or is this something you are still interested to include as part of 6.0?

UPDATE:

Since there are use cases in here not addressed by the source generator work, makes sense to keep this issue open but we reduced its priority.

[Kesmy]

The source generator (if used) solves zero problems I encounter in logging, but the analyzers (if used) would solve... pretty much every problem I encounter.

Nearly 100% of the libraries we use get logging wrong, because the people writing the logging in those libraries have no idea that log templates are supposed to be constant.

Why not solve the actual problem in all the existing dotnet versions via simple analyzers, and then work on whatever issue the code generation is supposed to solve for the future?

Nothing we use is going to be touching .NET 6, I haven't even seen any code using .NET 5, so abandoning improvements on the existing developer experience to aid developers that don't yet exist seems way out of touch here.

[davidfowl]

@Kesmy That sounds like a +1 for the logging analyzers. Thanks for letting us know this is still very important to you.

@ericstj @maryamariyan lets discuss to see if we can come up with a plan.

[Kesmy]

I want to add that this is also a case where "good enough" is probably going to be good enough.

There exists a SerilogAnalyzers package which performs this same analysis over Serilog templates, and while it doesn't cover every case (and can sometimes be a little overzealous, albeit with good reason), it very effectively achieves the goal of alerting developers that they've just written a bad logging statement.

I know there was an early post that suggested that structured logging analyzers should be none by default, but they should really be enabled; I know most orgs don't have my specific problems, but I lose an exceptional amount of storage to these messages, thus lose a lot of insight on production failures to log rolling.

[maryamariyan]

Thanks for your feedback, keeping this issue open to track adding an analyzer for logging as well.

[bartonjs]

Video

API Review notes

• Diagnostic 1
  • A fixer seems doable here.
  • A level higher than Disabled seems warranted. (Hidden?)
  • Category: Naming
• Diagnostic 2
  • A fixer seems possible here, but the resolution should create a static field for the defined message, not a local variable.
  • Should be Hidden, not Disabled.
  • Category: Performance
• Diagnostic 3
  • The example should be updated. logger.LogTrace("{0}", value) => logger.LogTrace("{Value}", value)
  • Information / Usage
• Diagnostic 4
  • This should not apply to LoggerMessage.Define, just LogTrace (and friends).
  • Information / Usage
• Diagnostic 5
  • Warning does seems appropriate when/since you can prove it's a guaranteed exception.
  • Since it's about an exception, let's use Reliability instead of Usage.
  • Warning / Reliability

[davidfowl]

Are we doing a fixer from LoggerMessage.Define to the new source generator approach? That would be great 😄

[maryamariyan]

Are we doing a fixer from LoggerMessage.Define to the new source generator approach? That would be great 😄

we're adding the analyzer as an analyzer in the SDK so my assumption is that the source generator wont be available in all cases that this analyzer suit is available for, and the fixer wouldn't be able to use the source gen. But would be good to use the source generator for adding the fixer.

cc: @ericstj

[maryamariyan]

We'll add the fixers in the 7.0 timeframe.

[poke]

Does that mean that the analyzers are on track for 6.0? That's great news! We've been hit by the issue from diagnostic 5 a few times in production so this will help a lot. Thanks for following up with this @maryamariyan!

[maryamariyan]

Does that mean that the analyzers are on track for 6.0?

Yes :) Here's a link to the merged PR dotnet/roslyn-analyzers#5244

[maryamariyan]

Closing this issue as fixed and opened dotnet/roslyn-analyzers#7241 to track adding the fixers.

[Kesmy]

Are these analyzers available prior to .NET 6? If not, we're probably not going to be able to use them for another decade.

[ericstj]

They have been added to the SDK so you can use them when targeting any framework and running on the latest SDK. The analyzers are also available out-of-band in https://www.nuget.org/packages/Microsoft.CodeAnalysis.NetAnalyzers. You can even try out a nightly build of those today: https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet6&package=Microsoft.CodeAnalysis.NetAnalyzers&protocolType=NuGet&version=6.0.0-preview7.21378.5&view=overview
Please let us know if you face any problems by filing new issues here.