[API Proposal]: Allow custom filtering logic for FakeLogger

[Piedone]

Background and motivation

FakeLogger is useful not just in a unit, but also in an end-to-end/UI testing use case. We also started to use it in our Lombiq UI Testing Toolbox for Orchard Core project.

In this use case, one is looking to run assertions on the log entries. However, when testing a complete app, the log can include entries that we can safely ignore, but these make assertions more complicated. E.g. instead of "the log should be empty" we need to assert "the log should be empty except for entries with the ignored message message".

The best would be to not log these messages in the first place, but in our experience, this is frequently not possible even after applying the existing log filter rules (e.g. filtering out complete classes). We'd need to target specific category-logger-log level-message combinations.

I'd like to propose the ability to optionally configure a custom filter delegate for this. It would work together with the existing filters, AND-ing with them.

API Proposal

public class FakeLogCollectorOptions
{
    /// <summary>
    /// Gets or sets custom filter for which records are collected.
    /// </summary>
    /// <value>The default is <see langword="null" />.</value>
    /// <remarks>
    /// Defaults to <see langword="null" /> which doesn't apply any additional filter to the records.
    /// If not empty, only records for which the filter function returns <see langword="true" /> will be collected by the fake logger.
    /// </remarks>
    public Func<FakeLogRecord, bool>? CustomFilter { get; set; }
}

API Usage

ConfigureLogging(loggingBuilder => loggingBuilder.AddFakeLogging(options =>
{
    options.CustomFilter = record => record.Message != "ignored message"
}))

Alternative Designs

Non-nullable type with default 'pass-through' filter

public Func<FakeLogRecord, bool> CustomFilter { get; set; } = _ => true;

This is more consistent with other existing properties, but may result in NRE when supplied null by the user.

Inversed semantics of the boolean value returned by the filter function

true returned by the predicate function would change meaning from "let the record pass" to "catch the record by the filter and don't let it pass". However, the current semantics is in line with other filtering properties and is familiar filtering approach in e.g. other programming languages such as javascripts .filter().

Workarounds

While not an alternative design, a workaround is to adjust the log levels of entries, so they can be filtered on that. However, in our case, due to the log entries coming from external projects that we can't all feasibly change, this is not a suitable solution.

Risks

This is a non-breaking change.

A custom filter predicate, if implemented inefficiently, can potentially slow logging down a lot. Since we're talking only about specific testing scenarios where other filtering is not suitable, I'd consider this acceptable.

[Demo30]

Hello @Piedone, it should be possible to filter the log records after the fact for the purpose of the assertion:

var fakeLogger = new FakeLogger<object>();

fakeLogger.LogInformation("ignored message");
fakeLogger.LogInformation("Message I'm interested in");

IReadOnlyList<FakeLogRecord> logs = fakeLogger.Collector.GetSnapshot();

var logsForAssert = logs.Where(log => log.Message != "ignored message");
Assert.Single(logsForAssert);

Is this approach not feasible for your case? If not, could you please elaborate on what challenges you are facing that this enhancement would help tackle?

[Piedone]

Thanks for your reply. As elaborated in the issue, yes, this works, and is the workaround we apply currently. However, it would be nicer to not log irrelevant entries in the first place, since that adds noise to assertions and debugging.

Note that while this is a new API, it's not a new idea, since it only expands on the already existing, but coarser, filtering options.

[evgenyfedorov2]

what about a Func<> like this

public Func<FakeLogRecord, bool> FilteredRecords { get; set; } = (_) => true;

as more modern looking?

[Demo30]

I suggest having a collection of Func<> to support better dynamic composability of the conditions.

public IEnumerable<Func<FakeLogRecord, bool>> CustomFilters { get; set; } = Array.Empty<Func<FakeLogRecord, bool>>();

[Piedone]

I'd be happy with either of these.

[Demo30]

Hello @Piedone, thank you for your proposal and the time spent so far!

We want to proceed with this proposal. It needs to first pass through api review. Will be added to the api review backlog automatically when api-ready-for-review tag is added. Do you wish to drive this effort further through the review yourself or would you prefer us to handle it?

Regarding the api proposal itself, please consider modifying it based on the discussion here and also in this related draft PR. That is, consider the following points:

• using Func<> instead of Predicate as it is more modern
• having a collection of filters instead of single filter to allow for a better dynamic composability of the conditions
• having the semantic of the filter reversed so that "true" would mean the record should get filtered out instead of allowed to pass. Since the usage is expected to be to filter this or that out and to potentially stack these constraints using multiple filters, then it might be more idiomatic to use as: filters.Add(r => r.Message == "Ignore message 1"); and filters.Add(r => r.Message == "Ignore message 2");
• declaring the property as nullable with null value as default

Let us know and we'll move on by adding the api-ready-for-review tag.

fyi @evgenyfedorov2

[Piedone]

Awesome!

I'd be happy to take part in the review process, but I don't really see (also after reading https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md) what I'd do. I'll reply to comments here and under the PR, of course, and the latter I've also checked out already.

I updated the proposal above. However, thinking a bit more, I think if we go with a collection of filters, then instead of IEnumerable<T> it should be mutable type that allows adding items. This is because with just IEnumerable<T>, you'll always need to add a collection of Funcs, even when you add a single one (which I suspect would be the majority of cases) which feels awkward:

options.CustomFilters = [ record => record.Message != "ignored message" ];

But more importantly, if you want to allow other code to contribute, you'd need to use some kind of collection concatenation, which also feels awkward to me:

options.CustomFilters = options.CustomFilters.Union([ record => record.Message != "ignored message" ]);

Instead, we could use ICollection<T>, and allow what you also mention:

options.CustomFilters.Add(record => record.Message != "ignored message");

In this case, by default the property should should have e.g. a List<T> assigned, though. And at that point, maybe allowing null and the ability to set the property is detrimental too (since it opens up the possibility of NREs). So then, perhaps?

public class FakeLogCollectorOptions
{
    /// <summary>
    /// Gets or sets custom filters for which records are collected.
    /// </summary>
    /// <value>The default is an empty collection.</value>
    /// <remarks>
    /// Defaults to an empty List, which doesn't filter any records.
    /// If not empty, only records for which the filter function returns <see langword="true" /> will be collected by the fake logger.
    /// </remarks>
    public ICollection<Func<FakeLogRecord, bool>> CustomFilters { get; } = new List<Func<FakeLogRecord, bool>>();
}

ConfigureLogging(loggingBuilder => loggingBuilder.AddFakeLogging(options =>
{
    options.CustomFilters.Add(record => record.Message != "ignored message" ]);
}))

Regarding the positive/negative filtering logic, I commented here: https://github.com/dotnet/extensions/pull/5848/files#r1949072508.

[Demo30]

Hello @Piedone,

ad API review process and ownership

• Either you or someone from the internal team (me) will be assigned as owner of this issue. You can decide. Once the discussion here reaches conclusion, the issue can be tagged with api-ready-for-review and the issue will be added to backlog for the actual api review. When it gets scheduled, the owner would present the proposal on a meeting (see step 5)

ad API

• We need to balance the ease of use, performance and having the API future-proof. I agree with your arguments and the proposal makes sense to me. See the benchmarks below to compare different solutions. Each measurement was made on 1_000_000 records with 3 filters in place for each. To accommodate both performance and ease of use, either List or HashSet seem to work best. But those also lock us to these types, while ICollection gives us some freedom to potentially change the implementation in the future. The ICollection with List is up to 10 times less performant, but I think that possibly 9ms of difference is not relevant in the scenario of 1mil records filtered. @evgenyfedorov2 What do you think?

IEnumerable<Func<>> = List assigned
IList<Func<>> = List assigned
ICollection<Func<>> = List assigned

| Method  | Mean      | Error     | StdDev    |
|-------- |----------:|----------:|----------:|
| LinqAny | 21.568 ms | 0.4300 ms | 0.8386 ms |
| Foreach |  9.749 ms | 0.1923 ms | 0.5066 ms |

IList<Func<>> = Array assigned
IEnumerable<Func<>> = Array assigned

| Method  | Mean     | Error    | StdDev   |
|-------- |---------:|---------:|---------:|
| LinqAny | 13.81 ms | 0.273 ms | 0.383 ms |
| Foreach | 10.03 ms | 0.200 ms | 0.246 ms |

List<Func<>> = List

| Method  | Mean      | Error     | StdDev    |
|-------- |----------:|----------:|----------:|
| LinqAny | 20.411 ms | 0.4063 ms | 0.5283 ms |
| Foreach |  2.027 ms | 0.0367 ms | 0.0343 ms |

HashSet<Func<>> = HashSet assigned
Func<>[] = Array assigned

| Method  | Mean        | Error     | StdDev    | Median      |
|-------- |------------:|----------:|----------:|------------:|
| LinqAny | 14,287.6 us | 309.94 us | 899.20 us | 14,044.1 us |
| Foreach |    907.5 us |  18.01 us |  16.84 us |    901.4 us |

[evgenyfedorov2]

FakeLogger is used in test code only, hence benchmarks are not that relevant, probably?

Also, I still think one delegate is enough. If you need to have multiple conditions, you can have them all in the same delegate. What is the scenario when this is not enough and you really need multiple delegates within the same unit test? This will be discussed at API review anyway, though.

[Demo30]

ad multiple conditions

• The idea was to ease the potential need for composability of the filters (having some base general filtering predefined and then extend it based on specific scenarios), but the user can achieve the same results by just wrapping the conditions into single condition:
  • Func<FakeLogRecord, bool> baseCondition = r => r.Message != IgnoredMessage1;
  • Func<FakeLogRecord, bool> composed = record => baseCondition(record) && record.Message != IgnoredMessage2;

[Piedone]

ad API review process and ownership

Thank you for explaining. I'd happy to be the owner of this issue in this sense. However, I'm not sure I have the project permissions necessary, though (can't change the labels here, for example).

ad API and multiple conditions

TBH I don't think multiple conditions are important enough to support with a collection (hence my original proposal didn't include it). In our UI testing framework, we have multiple such delegates configurable from the consuming test suites, and composing them works fine; I don't think using a collection is more convenient.

All this keeping in mind that while if you develop a testing framework like us, then this is a more interesting use case, but I'd think (but I have no hard data to back this up) that if you're in the more frequent "I just want a test project" use case, then you'll at most set this delegate once for a given FakeLogger instance. Thus, I'd focus on making this as simple and as hard to misuse as possible.

[RussKie]

Assigning @evgenyfedorov2 and @Demo30 as champions for this API proposal.

[Demo30]

ad API and multiple conditions

Sure, perfectly fine with me. Thank you for iterating through the suggestion! To summarize:

• we'll have a single Func<> filter
• the meaning of the returned true would remain the "The record should pass", but the proposed inverse variant can be mentioned during the review
• nullability of the type Func<> can also be discussed during the review. Both approaches have their pros and cons. Personally, I'd go with nullable, default null and handling it in the implementation without risk of reaching NRE, but other existing properties already use the defaults and don't mind the potential NRE (e.g. FilteredLevels and other).

Regarding further steps:

• I'll keep you posted

[Demo30]

Hello @Piedone, I have modified the proposal slightly to match the current state of the discussion. I have assigned new tags, and the proposal is now listed in the API review backlog! The next review online meeting is scheduled for next week's Tuesday 18th (see the schedule and look for BCL sessions). The proposals are discussed by working through the backlog queue and I believe this proposal is now a 3rd to go, meaning it may come up during the next session, but also may not, depending on how long the other topics take. These meetings are held publicly, and you can watch them on the .NET Foundation YouTube channel.

I have added you as one of the ticket's owners so hopefully you will receive an invitation to that meeting in time. The plan is we both attend and if you wish you can lead the presentation. In case no invitation arrives, we'll sync here on the Monday 17th the latest about further steps possibly ending with me presenting the proposal.

Thank you for all the time and effort!

[Piedone]

Thank you! I'd be happy to attend the meeting, so hopefully I'll receive an invite.

Appreciate the thoroughness that goes into this!

[Demo30]

Hello @Piedone, no invitation on my end, so looks like the topic will be scheduled for another date. I'll let you know once I receive my invitation.

[Piedone]

OK, thank you!

[joperezr]

Adding @samsp-msft @noahfalk @tarekgh as some of our logging and telemetry experts. Any thoughts on this proposal? Do we already have anything in the framework that can already be used for this purpose?

[tarekgh]

I am wondering, can't this achieve using the new sampling APIs #5123 (comment)?

CC @geeknoid

[geeknoid]

I definitely like the feature idea and would have probably stuck it in FakeLogger in the first place if I'd thought of it.

@tarekgh You could indeed implement this with the new sampling capabilities, but I think it might not be to best UX since you'd have to enable and configure the full sampling infrastructure. Since the point of this is a utility feature to save some lines of code, making it super convenient seems like a better forward IMO.

[Demo30]

Is the proper full api review still needed, or can we fast-forward this one by discussing here as the API change is rather minimal?

[tarekgh]

This change is in Microsoft.Extensions.Diagnostics.Testing which did not go through design review if I am not mistaken. So, the owner of the library can decide if want to design review it. In general, I would recommend design review public interfaces anyway. Usually in design review you can get some good feedback.

I am good with the proposal too.