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]

If you're using Ordinal comparison, then more idiomatic to write as follows:

Suggested change

	r => !r.Message.Equals(IgnoredMessage, StringComparison.Ordinal),
	r => r.Message != IgnoredMessage,

[RussKie]

Would this work?

Suggested change

	foreach (var condition in _options.CustomFilters)
	{
	if (!condition(record))
	{
	// record was filtered out by a custom filter
	return;
	}
	}
	if (_options.CustomFilters.Any(filter => !filter(record))
	{
	// record was filtered out by a custom filter
	return;
	}

[RussKie]

This looks sus - if any record hasn't been filtered (i.e., !filter(record)) then exit because "record was filtered out by a custom filter". Don't you think?

[Demo30]

ad Linq/foreach: 👍 will switch to Linq

ad !filter(): Do I understand correctly you would find a reversed semantic better? Currently the predicate evaluates whether the record should be allowed to pass through each filter, that is: "if the record fails to pass any of the filters, it is filtered out", when reversed, the predicate would evaluate whether the record should be caught by each filter, that is: "if the record is caught by any of the filters, it is filtered out" - both seem ok to me, but since the usage will more likely be to "filter this and that out", the reversed semantic might work better

[RussKie]

RE: linq vs not-linq

It's may worth checking perf switching to linq. If this is a hot path, then non-linq version may behave better. ¯\_(ツ)_/¯

the reversed semantic might work better

To me the current implementation looks confusing to me (which means it'll likely confuse others)...
I sense the predicate's logic is inverse - i.e., "is the record caught by the filter => false", whereas perhaps it should be "is the record handled/filtered by the filter => true". This will remove the inversion in the iterator, i.e., if (_options.CustomFilters.Any(filter)) return true.

WDYT?

[Piedone]

My idea under the issue was that only those records will be collected that are allowed by the filter, i.e. those that the filter returns true for (and if not filter is specified, then all records are allowed). This is following the logic of the other filtering here above: e.g., if FilteredLevels contains items, then only those records that have the level contained there will be allowed.

So, what's currently in the PR is IMO correct.

[Demo30]

The inverse was proposed in this comment: #5848 (comment)

I think both versions are possible, but I suggested going with the original proposal as that seems to be more widely used (e.g.: .Where in LINQ, .filter in javascript, stream.filter in Java) and also consistent with the behavior of the other filters in the same options type as mentioned by @Piedone.

[Demo30]

In any case, this API proposal needs to pass through API review first anyway, so both options can be mentioned during the process and considered there?

[Piedone]

I guess indeed. My concern is really only about keeping it consistent with the other filter options here, what the current implementation is.

[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]

Suggested change

	/// Gets or sets custom filters used to filter out records to be collected.
	/// Gets or sets filters which control what records would be collected by the fake logger.

[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.

[RussKie]

Suggested change

	/// <summary>
	/// Gets or sets custom filters used to filter out records to be collected.
	/// </summary>
	/// <value>Default is an empty array.</value>
	/// <remarks>
	/// Defaults to an empty array, 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>
	[Experimental(DiagnosticIds.Experiments.Telemetry)]
	public IEnumerable<Func<FakeLogRecord, bool>> CustomFilters { get; set; } = Array.Empty<Func<FakeLogRecord, bool>>();
	/// <summary>
	/// Gets or sets filters which control what records would be collected by the fake logger.
	/// </summary>
	/// <remarks>
	/// The default value is an empty array, which means all records would be collected.
	/// To filter a record out, the filter function must return <see langword="true" />.
	/// </remarks>
	[Experimental(DiagnosticIds.Experiments.Telemetry)]
	public IEnumerable<Func<FakeLogRecord, bool>> CustomFilters { get; set; } = Array.Empty<Func<FakeLogRecord, bool>>();

Suggested change

	/// <summary>
	/// Gets or sets custom filters used to filter out records to be collected.
	/// </summary>
	/// <value>Default is an empty array.</value>
	/// <remarks>
	/// Defaults to an empty array, 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>
	[Experimental(DiagnosticIds.Experiments.Telemetry)]
	public IEnumerable<Func<FakeLogRecord, bool>> CustomFilters { get; set; } = Array.Empty<Func<FakeLogRecord, bool>>();