-
Notifications
You must be signed in to change notification settings - Fork 827
[Logging] Fixes LogProperties and LogPropertyIgnore attributes to work if an object being logged resides in a different assembly than the logging method #6600
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
Conversation
Removes Conditional attribute from the definition of LogProperties and LogPropertyIgnore attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes attribute visibility for LogProperties
and LogPropertyIgnore
by removing their Conditional
annotations, ensuring metadata is emitted across assemblies, and adds a cross-assembly test scenario.
- Removed
[Conditional("CODE_GENERATION_ATTRIBUTES")]
from both attribute definitions to retain attribute metadata in compiled assemblies. - Introduced a HelperLibrary (
ObjectToLog
/FieldToLog
) and updated unit test projects to validate logging objects from a different assembly. - Enhanced existing emitter tests and test classes to include a
LogObjectFromAnotherAssembly
scenario.
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
test/Generators/Microsoft.Gen.Logging/Unit/Microsoft.Gen.Logging.Unit.Tests.csproj | Added project reference to HelperLibrary for cross-assembly testing. |
test/Generators/Microsoft.Gen.Logging/Unit/EmitterTests.cs | Included ObjectToLog assembly in generator input assemblies. |
test/Generators/Microsoft.Gen.Logging/TestClasses/LogPropertiesExtensions.cs | Added LogObjectFromAnotherAssembly test logging method. |
test/Generators/Microsoft.Gen.Logging/HelperLibrary/ObjectToLog.cs | Created helper class with LogPropertyIgnore and nested LogProperties . |
test/Generators/Microsoft.Gen.Logging/HelperLibrary/Microsoft.Gen.Logging.HelperLibrary.csproj | Defined the HelperLibrary project configuration. |
test/Generators/Microsoft.Gen.Logging/HelperLibrary/FieldToLog.cs | Created helper FieldToLog class. |
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Logging/LogPropertyIgnoreAttribute.cs | Removed unused using System.Diagnostics; and Conditional attribute. |
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Logging/LogPropertiesAttribute.cs | Removed unused using System.Diagnostics; and Conditional attribute. |
Comments suppressed due to low confidence (3)
test/Generators/Microsoft.Gen.Logging/TestClasses/LogPropertiesExtensions.cs:250
- A new test method
LogObjectFromAnotherAssembly
was added, but there’s no corresponding expected output or assertions for its generated code. Please update the expected generated output or add assertions to cover this test scenario.
public static partial void LogObjectFromAnotherAssembly(ILogger logger, [LogProperties] ObjectToLog logObject);
test/Generators/Microsoft.Gen.Logging/Unit/Microsoft.Gen.Logging.Unit.Tests.csproj:23
- [nitpick] The indentation of this
<ProjectReference>
line does not match the surrounding entries. Align indentation with existing project references for consistency.
<ProjectReference Include="..\HelperLibrary\Microsoft.Gen.Logging.HelperLibrary.csproj" />
test/Generators/Microsoft.Gen.Logging/TestClasses/LogPropertiesExtensions.cs:8
- [nitpick] This
using
directive is not sorted with the other using statements. Consider grouping and ordering system usings first, then external dependencies, with a blank line between groups.
using Microsoft.Gen.Logging.Test;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if similar behavior also affects to TagNameAttribute
and TagProviderAttribute
.
Yes, it is similar. For the |
Fixes #6598 by removing the
Conditional
attribute from the definition of theLogProperties
andLogPropertyIgnore
attributes.The
Conditional
attribute is the root cause of the bug. When a property is marked with theLogPropertyIgnore
attribute, the associated metadata is excluded from the compiled assembly. As a result, the logging method’s code generator, which relies on reflection to determine whether a property should be logged, lacks the necessary information and ends up emitting the property regardless.When the logging method and the object being logged are located in the same assembly, the behavior differs. In this case, the code generator does not rely on the compiled assembly and can access the metadata associated with the
LogPropertyIgnore
attribute directly. As a result, it correctly identifies which properties should be excluded from logging.While there is a workaround, i.e. defining the
CODE_GENERATION_ATTRIBUTES
compilation symbol required by theConditional
attribute, it is better to fix the bug by removing theConditional
attribute. If a user forget to apply the workaround he/she risks leaking privacy or security sensitive data which could be stored in properties marked asLogPropertyIgnore
.Microsoft Reviewers: Open in CodeFlow