Open dahlbyk opened 1 year ago
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.
Haven't looked at OAuth yet, but for the main package it seems like we'd want something like services.AddQuickBooksOnlineClient()
that registers:
ILoggerFactory
, if configuredIntuit.Ipp.Core.IConfigurationProvider
implementation that builds IppConfiguration
from DI, including logging
ILoggerFactory
on IppConfiguration
(default to NullLoggerFactory
) for future logging needs. 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 ServiceContext
s with a DI-provided IConfigurationProvider
Intuit.Ipp.DataService.DataService
and such (need RealmId
), but creating from ServiceContext
is probably fine initiallyAny other thoughts from your experiments, @MatthewSteeples?
@dahlbyk speaks the truth! I would love to see some of this integrated.
Pullling out of #221 for discussion:
Microsoft.Extensions.Logging
should replaceIntuit.Ipp.Diagnostics.ILogger
JsonFileConfigurationProvider
usesMicrosoft.Extensions.Configuration
, but doesn't take advantage of its strongly-typed patterns.IConfigurationRoot
gets built; many apps (especially all ASP.NET Core) will have existing configServiceProvider
withMicrosoft.Extensions.DependencyInjection
could have access to everything you need to access current configuration, resolve anILoggerProvider
, etcAll things being equal, I would probably target version 2.1.x for these dependencies due to support on .NET Framework.