-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Please remove the Microsoft.Extensions.Logging.Abstractions dependency from the next version. #2589
Comments
I'm going to need more than "are very bad". Please be specific; what objection do you have, concretely?
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. |
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 <
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) |
Now we're becoming more actionable. I'm not at a PC to investigate, but this prompts the questions:
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! |
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
It's only on
It's a build error. It's added using this in a <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
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
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'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 |
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. |
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. |
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. |
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.
and
and internal extension for logging
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
The text was updated successfully, but these errors were encountered: