QuickBooks-V3-DotNET-SDK icon indicating copy to clipboard operation
QuickBooks-V3-DotNET-SDK copied to clipboard

Deprecate Serilog Configuration

Open dahlbyk opened this issue 4 years ago • 6 comments

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

  1. Add AdvancedLogger.Logger to allow configuring an IAdvancedLogger without Serilog
  2. Let TraceLogger serve as an IAdvancedLogger
  3. Mark Configuration.RequestAdvancedLog and Serilog-specific Diagnostics.AdvancedLogging as [Obsolete], nudging users towards TraceLogger or 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.Logging could be an alternative implementation of your ILogger, 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
  • JsonFileConfigurationProvider uses Microsoft.Extensions.Configuration, but doesn't take advantage of its strongly-typed patterns
  • It also hard-codes how the IConfigurationRoot gets built; many apps will have alternative configuration sources
  • A ServiceProvider could have access to everything you need to access current configuration, resolve an ILoggerProvider, etc

All things being equal, I would probably target version 2.1.x due to support on .NET Framework.

dahlbyk avatar Feb 10 '22 08:02 dahlbyk

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

nimisha84 avatar Feb 01 '23 08:02 nimisha84

Please make any readme updates for changes in behavior expected with this PR

nimisha84 avatar Feb 01 '23 08:02 nimisha84

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 how OAuth2Client.AdvancedLogger is initialized.

dahlbyk avatar Feb 01 '23 13:02 dahlbyk

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

dahlbyk avatar Feb 01 '23 17:02 dahlbyk

OK, updated README and marked the old file-based RequestLog as [Obsolete] as well.

dahlbyk avatar Feb 01 '23 21:02 dahlbyk

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.

dahlbyk avatar Feb 02 '23 01:02 dahlbyk