Conversation
Several improvements to the analyzer: - Property accesses are common in logging calls, and property accesses are supposed to be cheap. Avoid raising diagnostics for property accesses. - GetType() is frequently used in logging calls; special-case it to avoid diagnostics for it. - The main reason this rule exists is to eliminate cost on hot paths. Generally such hot paths aren't raising warning/error/critical diagnostics, such that the more rare warning/errors don't need as much attention to overheads. As such, I've changed the checks to only kick in by default for information and below, with a configuration switch that can be used to override to what levels it applies.
There was a problem hiding this comment.
Pull Request Overview
This PR reduces noise from analyzer CA1873 (AvoidPotentiallyExpensiveCallWhenLogging) by adding three key improvements: (1) exempting property accesses since they should be cheap, (2) special-casing common trivial methods like GetType/GetHashCode/GetTimestamp that are frequently used in logging, and (3) introducing log level filtering to only analyze logs at Information level and below by default, with a configurable threshold via EditorConfig.
Key Changes
- Property accesses (without arguments) are now considered cheap and won't trigger diagnostics, though indexer arguments are still validated
- Trivial methods (object.GetType, object.GetHashCode, Stopwatch.GetTimestamp) are exempted from analysis as they're common in logging and sufficiently cheap
- By default, only log levels up to Information are analyzed; Warning/Error/Critical levels are skipped unless configured otherwise via
dotnet_code_quality.CA1873.max_log_level
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| AvoidPotentiallyExpensiveCallWhenLogging.cs | Core analyzer implementation: adds IsTrivialInvocation method to exempt common cheap methods, updates IsPotentiallyExpensive to treat simple property accesses as cheap, adds log level threshold checking with EditorConfig support, refactors RequiredSymbols class to use primary constructor and Dictionary instead of ImmutableDictionary |
| EditorConfigOptionNames.cs | Adds MaxLogLevel constant for the new max_log_level EditorConfig option with proper XML documentation |
| AvoidPotentiallyExpensiveCallWhenLoggingTests.cs | Updates existing tests to reflect new log level behavior (Warning/Error no longer flagged by default), adds comprehensive tests for trivial method exemptions (GetType, GetHashCode, GetTimestamp), adds tests for cast operations, adds extensive tests for log level configuration via EditorConfig, updates test helper methods to support EditorConfig testing |
|
@jeffhandley, you good with this? |
|
This is targeted for 11 because of what |
|
I'm fine with either. |
jeffhandley
left a comment
There was a problem hiding this comment.
This is great; thanks, @stephentoub. And I agree we should backport into 10.0 servicing. I can shepherd that through.
@baronfel, what branch is that? |
|
/backport to release/10.0.1xx That one :) |
|
Started backporting to |
|
@baronfel backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128 |
|
@stephentoub that didn't apply cleanly - can you do the manual backport to release/10.0.1xx? |
Several improvements to the analyzer: - Property accesses are common in logging calls, and property accesses are supposed to be cheap. Avoid raising diagnostics for property accesses. - GetType/GetHashCode/GetTimestamp are used reasonably-frequently in logging calls; special-case them to avoid diagnostics for them. - The main reason this rule exists is to eliminate cost on hot paths. Generally such hot paths aren't raising warning/error/critical diagnostics, such that the more rare warning/errors don't need as much attention to overheads. As such, I've changed the checks to only kick in by default for information and below, with a configuration switch that can be used to override to what levels it applies.
|
@akoeplinger, are you aware of any issues with the backport action? A few times now it's failed to backport a change to a 10.0 release branch, but doing the cherry-pick locally has worked fine with zero conflicts. |
|
Was this fix backported to 10.0.103? I'm still seeing the analyzer light up for logging calls that reference simple property accesses. |
Is it firing not because of property access but because there's an array allocation? |
|
No array allocation, just a string property, like _logger.LogInformation("Parsing category {CategoryName}", category.Name);
Is the hint still there by design? |
|
But there is an array allocation, you just don't see it in the source. The signature of that overload is: |
|
Oh that signature 😁Got it. Every overload takes a Edit: Am I just not understanding what this was supposed to address?
|
Guard the call with an IsEnabled check, or use the source generator.
The analyzer is only a suggestion by default. You must be raising its level somewhere? The purpose of the analyzer is to flag work that has non-trivial cost that you're paying for even when logging is disabled. Allocation is such a thing, so it's doing its job. But if you don't want it, don't enable it :)
That's accurate, and if you were using the source generator, previously passing in a property access would have warned and now it won't. You're not getting a warning because of the property access, you're getting it because of the array allocation. |
|
That makes sense. Thanks for the explanation @stephentoub, much appreciated! |
@stephentoub looks like I missed the earlier ping but I think I just fixed this with dotnet/arcade#16500, let me know if you still see it. |
Several improvements to the analyzer: