Reduce noise from CA1873 #51818
Reduce noise from CA1873 #51818
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
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. |
4 participants
Several improvements to the analyzer: