Skip to content
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

Deprecate Serilog Configuration #221

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dahlbyk
Copy link
Contributor

@dahlbyk dahlbyk commented Feb 10, 2022

Following on from #108 (comment), and seeing many Serilog-related issues, I tracked down how Serilog is currently used:

public void Log(string messageToWrite)
{
logger.Write(LogEventLevel.Verbose, messageToWrite);
}

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.

@nimisha84
Copy link
Collaborator

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
Copy link
Collaborator

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

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Feb 1, 2023

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
Copy link
Contributor Author

dahlbyk commented Feb 1, 2023

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: #286

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Feb 1, 2023

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

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Feb 2, 2023

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: dahlbyk/QuickBooks-V3-DotNET-SDK@deprecate-serilog...remove-serilog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants