Remove Serilog Dependency + Log to File

[dahlbyk]

• Part 2 of Deprecate Serilog Configuration #221
  • This is a breaking change for anyone using Serilog

Opening as a draft so we can evaluate the end state after Serilog is removed. A few observations:

• It strikes me as maybe not ideal that we end up with an extra level of indirection that ultimately provides no value:

  • ippConfig.Logger.CustomLogger could be ippConfig.Logger
  • ippConfig.AdvancedLogger.Logger could be ippConfig.AdvancedLogger

  Not sure there's an alternative in the short-term, so maybe leave like that until Adopt Microsoft.Extensions.* #286?

• Inuit.Ipp.Core ends up defaulting to TraceLogger in all cases, which seems reasonable, but OAuth2PlatformClient defaults to NullOAuthLogger. Maybe a TraceOAuthLogger would be a better default?

• IOAuthLogger and NullOAuthLogger end up being the only types in Intuit.Ipp.OAuth2PlatformClient.Diagnostics. Does that need to be a separate project? Maybe I should try to find a way to avoid adding IOAuthLogger so the project can just go away?

• I also noticed that IAdvancedLogger and IOAuthLogger do a bit of basic logging, but mostly exist to log request and response. I designed IOAuthLogger for this, but IAdvancedLogger is a simple Log(). Should they be more similar? Should LogRequest()/LogResponse() be extension methods on Intuit.Ipp.Diagnostics.ILogger (or Microsoft.Extensions.Logging.ILogger, since OAuth2PlatformClient doesn't depend on Intuit.Ipp.Diagnostics)?

[nimisha84]

@dahlbyk Thanks for creating this PR and raising the questions. Let me review them over the weekend and get back to you as I do have any local setup for the SDK anymore. If something breaks for logging apart from serilog deprecation then we will be in a messy situation. @MatthewSteeples would you be able to review this PR and provide some guidance or if you already have a solution for this in your fork?

[nimisha84]

cc: @sujitharamadass

[MatthewSteeples]

I've rebased this branch off the latest master and included a few other changes. We're validating it in our test environment for the next couple of weeks, and then planning on our production environment after that.

[dahlbyk]

@MatthewSteeples which of the existing logging features are you using?

I'm wondering if there's value in this intermediate step, vs just one big breaking change that completely revamps logging with the modern abstraction.

[MatthewSteeples]

We've not been using any of the built in logging features really. We did a while ago but for the most part if we're having issues we end up resorting to Fiddler and the Debugger (or enabling tracing in the framework to log the requests and responses).

Happy to talk through making it one big change. Does make more sense than having a few breaking changes one after the other. I did try some of that on a separate branch but it was a pretty big rabbit hole to go down. I was replacing centralised logging with logging per class, and trying to wire in dependency injection at the same time though, so possibly bit off too much

[dahlbyk]

…but it was a pretty big rabbit hole to go down. I was replacing centralised logging with logging per class, and trying to wire in dependency injection at the same time though, so possibly bit off too much

With just this you've convinced me we need intermediate steps. 😁 Consumers deserve deprecations notices anyway. Roughly:

1. Document deprecation and alternatives: Deprecate Serilog Configuration #221
2. Serilog dependency is removed in this PR, and there is much rejoicing
3. Start on better Config/DI/Logging, e.g. Adopt Microsoft.Extensions.* #286 (comment)
4. Deprecate the remaining QBO logging abstractions as better logging patterns are implemented?

[topheryun]

@MatthewSteeples thanks for working on removing the Serilog dependency. Could you help me understand how far along you are with these changes?

[sujitharamadass]

@MatthewSteeples @dahlbyk Can you please share the status of this PR?

[MatthewSteeples]

Apologies, Intuit's deprecation of Desktop in the UK has taken more of my time than expected (as my team is responsible for migrating the customers to Quickbooks Online). I started organising some time with dahlbyk to go over this PR and the general plan but never followed up on it. I hope to be able to start looking into it again towards the end of July

[sujitharamadass]

Thanks @MatthewSteeples . Let us know if there is any update on this.

[sujitharamadass]

@dahlbyk @MatthewSteeples Any update on this PR?

[dahlbyk]

Did he not clarify he meant end of July 2024? 😏

I'm still interested in landing this PR, and #221 before it (if you care at all about a deprecation cycle). If @MatthewSteeples still has limited availability I could be contracted to take the lead on modernizing this SDK.

[MatthewSteeples]

Apologies all, had a lot going on. Still very much interested in getting this library modernised. I've dropped you an email @dahlbyk

[clement911]

For a more modern library for the QBO API, consider trying QuickBooksSharp. It is a modern version that uses async, modern .NET and has minimal dependencies.

Disclaimer: I created this library because I had so many issues working with the official .NET library. While some functionality might still be missing compare to the official API, I'm confident that the dev experience is much better.
Contributions from the community are welcome.

[sujitharamadass]

Thanks @MatthewSteeples @dahlbyk . Will wait for your update.

[Shane32]

Did he not clarify he meant end of July 2024? 😏

I'm still interested in landing this PR, and #221 before it (if you care at all about a deprecation cycle). If @MatthewSteeples still has limited availability I could be contracted to take the lead on modernizing this SDK.

Is there any assistance I could provide towards landing this PR and #221 ?

[MatthewSteeples]

OK, better late than never...! We've been running a derivative of this branch without a hitch in our production environment for quite some time now, so I'll try and find out who from Intuit (@nimisha84 would this still be you?) we need to get it merged in.

I personally don't have time to put any effort in to #221 so I would propose releasing this as v16 and some release notes instructing people on what changes they need to make. Realistically, the current version of the library is broken for the majority of users anyway so I can't see it causing much disruption!

[dahlbyk]

I'm fine with the more breaking-change route if Intuit is. In that context, thoughts on any of my "Observations" from this PR? I really meant this as the start of a conversation on the right way to log this stuff, not a release candidate. 😁

Or I could spend a bit more time and we could jump straight to Microsoft.Extensions.Logging without the custom interfaces. 🤷‍♂️

[Shane32]

I looked through all files changed in this PR. Looks good to me.

[sujitharamadass]

Will check and update on this soon

…

On Sun, 1 Dec 2024, 10:08 Shane Krueger, ***@***.***> wrote: ***@***.**** approved this pull request. I looked through all files changed in this PR. Looks good to me. — Reply to this email directly, view it on GitHub <#289 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2RPCF3SVDJ4Q2KBB65SZPL2DKHFRAVCNFSM6AAAAAAUONNRAKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZRGMYDQMBXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>