Deprecate Serilog Configuration
Following on from https://github.com/intuit/QuickBooks-V3-DotNET-SDK/issues/108#issuecomment-1034273153, and seeing many Serilog-related issues, I tracked down how Serilog is currently used:
https://github.com/intuit/QuickBooks-V3-DotNET-SDK/blob/69d0962460b09f0d8f7ea1a72083afab2fae3719/IPPDotNetDevKitCSV3/Code/Intuit.Ipp.Diagnostics/AdvancedLogging.cs#L267-L270
That's it. Everything else is providing a proprietary way to configure Serilog, which does not seem like it should be this library's responsibility. For my part, I plan to stay on version 8.1.0 until the Serilog dependency is removed.
To that end, I offer this PR as a start towards Part 1 of 3:
Part 1: Deprecate Serilog Configuration
- Add
AdvancedLogger.Loggerto allow configuring anIAdvancedLoggerwithout Serilog - Let
TraceLoggerserve as anIAdvancedLogger - Mark
Configuration.RequestAdvancedLogand Serilog-specificDiagnostics.AdvancedLoggingas[Obsolete], nudging users towardsTraceLoggeror integrating with their existing logging infrastructure
A Serilog user who is using RequestAdvancedLog.CustomLogger would do something like this instead:
public class SerilogAdvancedLogger : Intuit.Ipp.Diagnostics.IAdvancedLogger
{
private Serilog.ILogger logger;
public SerilogAdvancedLogger(Serilog.ILogger logger) => this.logger = logger;
public void Log(string messageToWrite) =>
logger.Write(Serilog.Events.LogEventLevel.Verbose, messageToWrite);
}
// elsewhere
context.IppConfiguration.AdvancedLogger.Logger = new SerilogAdvancedLogger(logger);
A similar approach would be taken with IOAuthAdvancedLogger, though the required changes are more extensive due to how OAuth2Client.AdvancedLogger is initialized.
Part 2: Remove Serilog
Remove the Serilog dependencies and everything made [Obsolete] in Part 1.
You could certainly move the existing code to an Intuit.Ipp.Diagnostics.Serilog project/package to make this less of a breaking change, but I wouldn't think it's worth the effort to maintain.
Part 3: Adopt Microsoft.Extensions.*
-
Microsoft.Extensions.Loggingcould be an alternative implementation of yourILogger, or perhaps replace it altogether - logging is easier to consume with careful categorization (eliminating the need for "Advanced Logging"), which is not possible with the existing interface -
JsonFileConfigurationProviderusesMicrosoft.Extensions.Configuration, but doesn't take advantage of its strongly-typed patterns - It also hard-codes how the
IConfigurationRootgets built; many apps will have alternative configuration sources - A
ServiceProvidercould have access to everything you need to access current configuration, resolve anILoggerProvider, etc
All things being equal, I would probably target version 2.1.x due to support on .NET Framework.
Thanks for the PR. I have moved to a different team and SDK ownership will be transferred. CLosing off the pending PRs. Can you confirm if your PR is complete and handling serilog deprecation? I can review it then and get this prioritized
Please make any readme updates for changes in behavior expected with this PR
Can you confirm if your PR is complete and handling serilog deprecation?
If you're interested in the change, I can bring the PR up to date and tackle this:
A similar approach would be taken with
IOAuthAdvancedLogger,though the required changes are more extensive due to howOAuth2Client.AdvancedLoggeris initialized.
Code is ready for review. I've tried really hard to avoid breaking changes, other than compiler warnings when using the [Obsolete] logging configuration.
Working on a README update.
Part 3: Adopt Microsoft.Extensions.*
Opened a new issue for this: https://github.com/intuit/QuickBooks-V3-DotNET-SDK/issues/286
OK, updated README and marked the old file-based RequestLog as [Obsolete] as well.
Part 2: Remove Serilog
Remove the Serilog dependencies and everything made
[Obsolete]in Part 1.
Opened #289 for this, with some observations about the final state. Since my PR is from a fork I can't open that PR into this PR, so it shows all the changes. Here's just what's new in that PR: https://github.com/dahlbyk/QuickBooks-V3-DotNET-SDK/compare/deprecate-serilog...remove-serilog.