Add logging in RedisCache

[adityamandaleeka]

We don't have the ability to log anything in RedisCache today. It would be nice to be able to. One example of a thing worth logging is our set script logic (we fall back on using a deprecated command in some cases, such as when we can't determine the server version).

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[martincostello]

I've pushed up a draft to resolve this in #41769 - if it looks reasonable I'll open an issue for API review for a new constructor for the ILogger<T>.

[adityamandaleeka]

@martincostello Thanks!

It might be better to match what MemoryCache does and take an ILoggerFactory in the constructor:

https://github.com/dotnet/runtime/blob/8626da3cc62d940513141b7c58c3495a87bb2bba/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs#L46

Can you please make that change and then just add a comment to this thread for the API review?

[halter73]

If we do any public API changes, it needs to go through the API proposal process which involves filling out the API proposal issue template. We could just update this issue to fill in the required sections like "Proposed API", "Usage Examples", "Alternative Designs", etc..., but we should be doing that for any PR that touch a PublicAPI.*.txt file.

Even some PRs that don't touch these files should go through this process if it's effectively public API (for example minimal APIs special-casing existing parameter types and attributes it didn't before), but that's a bit more ambiguous.

When we discussed this in triage, I wavered on whether we should add a public constructor for this. I suggested doing something similar to KestrelServer and KestrelServerImpl where the internal Impl type is what gets the new constructor and gets registered to DI. During the meeting, I got swayed to supporting a new public constructor to align with MemoryCache.

But now that I've had even more time to consider this, I do not think we should add a public RedisCache(IOptions<RedisCacheOptions>, ILoggerFactory) ctor or a public RedisCache(IOptions<RedisCacheOptions>, ILogger<RedisCache>) constructor. I think the public ILoggerFactory constructor overload on MemoryCache was a mistake that we made because it was more convenient to implement it that way since we had to make it work with DI.

If we really think RedisCache deserves a public constructor that takes a logger, it should probably be public RedisCache(IOptions<RedisCacheOptions>, ILogger) instead. That way the person newing up the RedisCache can chose the category name. For example:

var highFrequencyCache = new RedisCache(highFrequencyOptions, myLoggerFactory.CreateLogger("HighFrequencyRedisCache"));
var lowFrequencyCache = new RedisCache(lowFrequencyOptions, myLoggerFactory.CreateLogger("LowFrequencyRedisCache"));

Maybe an ILoggerFactory also makes sense if you just want the default name, but it's less flexible. Of course, this overload in addition to an ILogger overload could confuse DI if someone registered an ILogger for some reason. For DI scenarios where the category name has to be controlled by the service itself and there's only one instance per host anyway, the ILogger<RedisCache> and/or ILoggerFactory overloads make much more sense, but they don't need to be on a public type.

Because of this, I think we should do the RedisCache and RedisCacheImpl split either way. And then we can decide on the merits if a public ILogger and ILoggerFactory constructor overload make sense for the public RedisCache type that we no longer register with DI based on the merits of the API and not just convenience.

[martincostello]

Thanks for the detailed feedback @halter73 - I've updated the draft to remove the changes to the public API surface area and added an *Impl type as suggested.