Skip to content

Conversation

iliar-turdushev
Copy link
Contributor

@iliar-turdushev iliar-turdushev commented Jul 11, 2025

Fixes #6598 by removing the Conditional attribute from the definition of the LogProperties and LogPropertyIgnore attributes.

The Conditional attribute is the root cause of the bug. When a property is marked with the LogPropertyIgnore 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 the Conditional attribute, it is better to fix the bug by removing the Conditional 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 as LogPropertyIgnore.

Microsoft Reviewers: Open in CodeFlow

Removes Conditional attribute from the definition of LogProperties and LogPropertyIgnore attributes
@Copilot Copilot AI review requested due to automatic review settings July 11, 2025 08:46
@iliar-turdushev iliar-turdushev requested a review from a team as a code owner July 11, 2025 08:46
Copy link
Contributor

@Copilot Copilot AI left a 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;

@eduherminio eduherminio self-requested a review July 11, 2025 10:46
Copy link
Member

@eduherminio eduherminio left a 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.

@evgenyfedorov2 evgenyfedorov2 merged commit cc0d010 into dotnet:main Jul 11, 2025
7 checks passed
@iliar-turdushev
Copy link
Contributor Author

iliar-turdushev commented Jul 11, 2025

I wonder if similar behavior also affects to TagNameAttribute and TagProviderAttribute.

Yes, it is similar. For the TagName and TagProvider I'm going to create a separate issue, because they currently have a bug which doesn't allow to use them with properties. At the moment, they can be used only with log method parameters. We'll fix both bugs in the separate issue.

@iliar-turdushev iliar-turdushev deleted the logpropertyignore-fix branch July 11, 2025 14:13
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logging] LogPropertyIgnore and LogProperties attributes don't work if an object being logged resides in a different assembly than the logging method
4 participants