Allow custom filtering logic for FakeLogger

[Demo30]

Fixes #5615

Microsoft Reviewers: Open in CodeFlow

[Demo30]

@dotnet-policy-service agree company="Microsoft"

[dotnet-comment-bot]

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Microsoft.Extensions.Caching.Hybrid	Line	86	82.77 🔻
Microsoft.Gen.MetadataExtractor	Line	98	57.35 🔻
Microsoft.Gen.MetadataExtractor	Branch	98	62.5 🔻
Microsoft.Extensions.AI.Ollama	Line	80	78.25 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project	Expected	Actual
Microsoft.Extensions.AI.Abstractions	83	84
Microsoft.Extensions.AI.OpenAI	77	78
Microsoft.Extensions.AI	88	89

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=941204&view=codecoverage-tab

[RussKie]

This setter will allow setting the property to null. Is this desirable?

[Demo30]

Since Nullable is enabled, attempt to setting it to null issues a warning. It is true that by supplying null the logic ends up crashing in runtime, which is awkward, however this seems to be the case for e.g. TimeProvider prop as well.

How about defining it as nullable, defaulting to null and having a null check at the place of usage?

[RussKie]

Since Nullable is enabled, attempt to setting it to null issues a warning.

"Nullable" only provides annotations which only work at compile-time. That is, those may result in warnings, however, they don't block the user code from assigning null (which results in NRE at runtime and may crash the app).

How about defining it as nullable, defaulting to null and having a null check at the place of usage?

It's definitely an option to expect a developer to implement null-checks, but since the API is annotated the good developer won't do that (because they trust us) and a mediocre developer won't do that (because they are mediocre). Either way, the likelihood of a user receiving an NRE is pretty high because our code isn't living up to the annotation and not handling nulls.

Null handling either implies throwing if null is assigned or setting (or just returning) an empty array (like we're doing for the init state). Which implementation better depends on usecases you're trying to address.

[Demo30]

Yes, that's want I meant, let's define the contract as nullable with null as default and handle the null gracefully on our side. This way the user will never reach NRE.

[RussKie]

@Demo30 when you're get closer to completion, please provide more details in the body of the commit message as well as in the original message explaining the nature of the changes. This is to allow future maintainers to look at the git commit and understand why the change was done instead of needing to locate the original CR.

Also, in the future please use GitHub keywords in the body of the git commit and not in the subject.

Thank you!

[RussKie]

I think this is the point of the disconnect for me: "only records for which the filter function returns <see langword="true" /> will be collected by the fake logger.".
To me if a filter function returns true, it means the record was filtered out.

I recommend inverting the meaning here, which will invert the logic in the collector implementation too.

[evgenyfedorov2]

@Demo30 what are the next steps in this PR?

[Demo30]

@Demo30 what are the next steps in this PR?

unfortunately same as 6 months ago, waiting to get scheduled for API review. At the moment, I'm closer to the top, but may change as it has already in the past. Any suggestion on how to move forward more effectively?

[Copilot]

Pull Request Overview

This PR adds support for custom filtering logic to the FakeLogger component by introducing a CustomFilter property to FakeLogCollectorOptions. This allows users to define custom logic to determine which log records should be collected, providing more flexibility beyond the existing level-based filtering.

• Added CustomFilter property to FakeLogCollectorOptions for custom filtering logic
• Implemented the custom filtering in FakeLogCollector.AddRecord method
• Added comprehensive test coverage for the new custom filtering functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
FakeLogCollectorOptions.cs	Added CustomFilter property with experimental attribute and documentation
FakeLogCollector.cs	Implemented custom filter evaluation in the AddRecord method
FakeLoggerTests.cs	Added parameterized test to verify custom filtering works with and without level filtering

[dariusclay]

API proposal shows approved, is this ready to go?

[dariusclay]

Overall, looks good. I'm on the edge between the naming of "CustomFilter" vs "FilteredRecords" though

[Demo30]

API proposal shows approved, is this ready to go?

from the API Review point of view yes, the api has been discussed on review and approved. We have some comments open here and after resolving those I think we're good to merge

[Demo30]

Overall, looks good. I'm on the edge between the naming of "CustomFilter" vs "FilteredRecords" though

I was thinking simplifying it to just Filter. I don't like FilteredRecords very much even though it seems to follow the other two filters, but in those cases: filtered levels is a set of levels, filtered categories is a list of categories -> for "filtered records" i'd expect a set of records, but this is a predicate so I personally would find that naming confusing because i'd expect to get/set some records here