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

Adopt Microsoft.Extensions.* #286

Open
dahlbyk opened this issue Feb 1, 2023 · 3 comments
Open

Adopt Microsoft.Extensions.* #286

dahlbyk opened this issue Feb 1, 2023 · 3 comments

Comments

@dahlbyk
Copy link
Contributor

dahlbyk commented Feb 1, 2023

Pullling out of #221 for discussion:

  • Microsoft.Extensions.Logging should replace Intuit.Ipp.Diagnostics.ILogger
    • Logging is easier to consume with careful categorization (eliminating the need for "Advanced Logging"), which is not possible with the existing interfaces.
  • 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 (especially all ASP.NET Core) will have existing config
  • A ServiceProvider with Microsoft.Extensions.DependencyInjection 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 for these dependencies due to support on .NET Framework.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Feb 2, 2023

https://learn.microsoft.com/en-us/dotnet/core/extensions/logging has a good overview of Microsoft.Extensions.Logging, including the intent of each log level. Aside from Serilog, a pain point with the current logging is it's almost all at the same level. Would help to promote some things to warnings, have basic request URL logged as info, and make sure everything with sensitive data (auth codes/headers, request/response bodies) uses Trace (for example).

I think the right approach is to give IppConfiguration an ILoggerFactory so different components can get loggers with an appropriate category for filtering.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented May 7, 2023

Haven't looked at OAuth yet, but for the main package it seems like we'd want something like services.AddQuickBooksOnlineClient() that registers:

  • Implementations of the QBO logging abstractions that depend ILoggerFactory, if configured
  • An Intuit.Ipp.Core.IConfigurationProvider implementation that builds IppConfiguration from DI, including logging
    • Could save the ILoggerFactory on IppConfiguration (default to NullLoggerFactory) for future logging needs.
    • Could probably reuse the sections/keys from JsonFileConfigurationProvider, but they shouldn't be top-level. Could update that to check for a QBO section to read from before reading from root (and log a deprecation warning)? Should also bind to strongly-typed options.
  • IServiceContextProvider with an implementation that constructs ServiceContexts with a DI-provided IConfigurationProvider
  • Could also make providers for Intuit.Ipp.DataService.DataService and such (need RealmId), but creating from ServiceContext is probably fine initially

Any other thoughts from your experiments, @MatthewSteeples?

@drusellers
Copy link

@dahlbyk speaks the truth! I would love to see some of this integrated.

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

No branches or pull requests

2 participants