Please remove the Microsoft.Extensions.Logging.Abstractions dependency from the next version.

[pairbit]

Right now I made a fork without the dependency Microsoft.Extensions.Logging.Abstractions https://github.com/pairbit/IT.Redis

The fact is that Microsoft.Extensions.Logging.LoggerExtensions are very bad, they are prohibited from being used in our projects.

Please create your own IRedisLogger interface without params object[]. Also replace the ILoggerFactory type with the Func<Type,IRedisLogger> delegate.

namespace StackExchange.Redis;

public interface IRedisLogger
{
    void Log(int level, Exception? exception, string? message);
}

and

namespace StackExchange.Redis.Configuration;

public class DefaultOptionsProvider
{
    public virtual Func<Type, IRedisLogger>? LoggerFactory => null;
}

and internal extension for logging

internal enum Level
{
    Trace,
    Debug,
    Information,
    Warning,
    Error,
    Critical,
    None
}

internal static class IRedisLoggerExtensions
{
    public static void LogInformation(this IRedisLogger logger, string? message)
        => logger.Log((int)Level.Information, null, message);

    public static void LogTrace(this IRedisLogger logger, string? message)
        => logger.Log((int)Level.Trace, null, message);

    public static void LogInformation(this IRedisLogger logger, Exception? exception, string? message)
        => logger.Log((int)Level.Information, exception, message);

    public static void LogError(this IRedisLogger logger, Exception? exception, string? message)
        => logger.Log((int)Level.Error, exception, message);
}

internal static class LoggerFactoryExtensions
{
    public static ILogger CreateLogger<T>(this Func<Type, IRedisLogger> factory)
        => factory(typeof(T));
}

I see that you do not use logging with params object[], but I primarily liked your library for its small number of dependencies.

Please do not close the ticket immediately. Consider my proposal. Thank you

[mgravell]

The fact is that Microsoft.Extensions.Logging.LoggerExtensions are very bad,

I'm going to need more than "are very bad". Please be specific; what objection do you have, concretely?

they are prohibited from being used in our projects.

I want to put this nicely: that's a "you" thing, not an "us" thing. Even if it is there, you can simply not use it. Removing it is itself a breaking change, so I'd need a really good justification for that.

[andrewlock]

The biggest issue for me with the Microsoft.Extensions.Logging.Abstractions dependency is it makes the nestandard 2.0 support a lie 😅

If you try to build a < netcoreapp3.1 app using version 2.7.4 of StackExchange.Redis, you'll now get a compile time error:

error : Microsoft.Extensions.Logging.Abstractions doesn't support netcoreapp3.0. Consider updating your TargetFramework to netcoreapp3.1 or later

The reason for the error is a "well known" issue, and there's reasons for Microsoft doing it, but it's pretty user-hostile IMO 😄 If you want to adopt the same policy that's obviously totally reasonable, just be aware that for most-intents and purposes you don't support .NET Standard 2.0 any more. Or I guess you support it in name only 😉

(FWIW, I'm only thinking about .NET Core/5+/Framework here, I realise netstandard2.0 does apply wider than that, which is also partly why this is such a mess)

[mgravell]

Now we're becoming more actionable.

I'm not at a PC to investigate, but this prompts the questions:

• does it report that message on netfx? Or just down-level netcoreapp?
• on the impacted runtimes, is it an error or a warning? if the latter, does it work, despite the warning? Is it guidance only?

If the answer to the first is "just down-level netcoreapp", then honestly that's a non-problem - that runtime is entirely out of support. I'm not going to bend things out of shape to facilitate running smoothly on obsolete runtimes. I'm entirely comfortable with doing nothing there.

Likewise, we mostly offer netstandard as a courtesy. The SLA here is exactly the same either way: "as is, with no warranty" blah blah blah. Since netstandard is a "who knows what it actually is" ephemeral target, we have always supported that less than the others, for any definition of supported. If it is causing confusion, maybe it is time to drop it!

[andrewlock]

Now we're becoming more actionable.

Just to be clear, I completely disagree with the premise of the OP 😅 Adding Microsoft.Extensions is a great option for a project like this in principle. The only problem is related to support for down-level TFMs, which is mostly due to Microsoft's approach.

Also, I forgot this, but you effectively already "dropped" netstandard support for down-level netcoreapp when you added System.Runtime.CompilerServices.Unsafe in 2.2.x, because that version of the package does a similar thing, "sabotaging" < .NET Core 3.1 builds

does it report that message on netfx? Or just down-level netcoreapp?

It's only on netcoreapp, on <.NET Core 3.1

on the impacted runtimes, is it an error or a warning? if the latter, does it work, despite the warning? Is it guidance only?

It's a build error. It's added using this in a .targets file in the package:

<Project InitialTargets="NETStandardCompatError_Microsoft_Extensions_Logging_Abstractions_netcoreapp3_1">
  <Target Name="NETStandardCompatError_Microsoft_Extensions_Logging_Abstractions_netcoreapp3_1"
          Condition="'$(SuppressTfmSupportBuildWarnings)' == ''">
    <Error Text="Microsoft.Extensions.Logging.Abstractions doesn't support $(TargetFramework). Consider updating your TargetFramework to netcoreapp3.1 or later." />
  </Target>
</Project>

As you can see from that, you can suppress the error using SuppressTfmSupportBuildWarnings

If the answer to the first is "just down-level netcoreapp", then honestly that's a non-problem - that runtime is entirely out of support. I'm not going to bend things out of shape to facilitate running smoothly on obsolete runtimes. I'm entirely comfortable with doing nothing there.

That's a perfectly reasonable policy IMO. The problem is the messaging. The package reports to support .NET Standard 2.0, which implies .NET Core 2+. You can install the package in a .NET Core 2+ project. But builds will fail with this obtuse error message that will be hard for people to understand.

To be clear, I 100% blame Microsoft for screwing this up, but the knock-on effect is that you already don't really support .NET Standard 2.0. Personally, if I were you, I would drop the netstandard2.0 target entirely. That likely simplifies a bunch of code on your side, and removes the issue entirely.

A separate but somewhat related issue is the fact that you're targeting the 6.0 package in all TFMs. While the Microsoft.Extensions package does support < net6.0, this may cause issues in practice. For example, if a user has a 3.1 ASP.NET Core app, they will be using the 3.1 packages for all of this, as well as the 3.1 implementations etc. If they add StackExchange.Redis, the abstractions package then gets dragged up to 6.0, while all other packages are 3.1. I've seen this cause gnarly NuGet resolution issues in practice.

My favored approach is to either

• Just target the lowest applicable Abstractions package in all TFMs, as package resolution will drag it up to the "correct" version in apps in practice.
• Conditionally target the "matching" package for each TFM. This is the "better" option IMO.

Luckily, the Abstractions packages basically don't change in major versions, so this is a pretty safe option.

Anyway, hope that all makes sense. If not, I tried to articulate it previous in this blog post. In summary:

• I think you should consider dropping netstandard2.0 entirely, as you basically don't work in most applicable environments anyway, and give confusing build errors.
• I think you should reconsider which packages you reference for different TMF

[mgravell]

I'll be honest: I didn't even notice the change in speaker. My bad. All good feedback, but I guess we really need to hear something from @pairbit

[mgravell]

Also: I don't disagree on netstandard being increasingly irrelevant. If it is starting to hurt us: I'm up for a cull in a major.

[pairbit]

The fact is that Microsoft.Extensions.Logging.LoggerExtensions are very bad,

I'm going to need more than "are very bad". Please be specific; what objection do you have, concretely?

they are prohibited from being used in our projects.

I want to put this nicely: that's a "you" thing, not an "us" thing. Even if it is there, you can simply not use it. Removing it is itself a breaking change, so I'd need a really good justification for that.

First, LoggerExtensions are bad because they cause memory allocation when using object parameters. I don't understand why Microsoft won't remove these options.

Secondly. The approach to string formatting is outdated. Now you can create a structured logger using string interpolation. See https://github.com/fedarovich/isle

The Microsoft.Extensions.Logging.Abstractions library does not meet my needs for writing structured logs using string interpolation.

Finally this is minor compared to the problem described by Andrew Lock.

[mgravell]

There are a ton of logging APIs available - you aren't forced to use those extension methods, and in high volume scenarios there are better structured logging approaches available that do not force objects. We're not really high volume, but we should consider using those (@NickCraver do you want me to have a stab at the generator-based logging approach?). But if you don't want to use ILogger: don't use it, fine! Them existing costs you nothing, and if the logger is null: nothing is expended. I don't see how any of that makes any difference to the library. My advice: just ignore the API you don't want to use.

[Meir017]

I believe we need to do something like dotnet/orleans#9273 here,

adding https://github.com/Meir017/github-copilot-prompts/blob/main/prompts/dotnet-logger-message-source-generator.md as a prompt-file in vscode and then opening the relevant files and just typing . in edit mode does 99% of the work :)

created #2899