Add context and info to logging interface

[jaronoff97]

Allow the logging interface to propagate context where possible. This will let logging sinks contain richer information about potential opamp IO errors.

[codecov]

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (fcfa095) 72.50% compared to head (5211c1e) 72.60%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
server/serverimpl.go	8.33%	11 Missing ⚠️
client/internal/packagessyncer.go	54.54%	9 Missing and 1 partial ⚠️
client/internal/httpsender.go	40.00%	6 Missing ⚠️
client/internal/receivedprocessor.go	61.53%	5 Missing ⚠️
client/wsclient.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   72.50%   72.60%   +0.09%     
==========================================
  Files          25       25              
  Lines        2022     2022              
==========================================
+ Hits         1466     1468       +2     
+ Misses        446      445       -1     
+ Partials      110      109       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jaronoff97]

Before i continue with this... @andykellr @evan-bradley @tigrannajaryan what do you all think of this approach? Propagating the context would make it easier for consumers to understand the potential problems that the impls run in to, but i'm not sure how i feel about the interface. Let me know your thoughts. Thank you.

[tigrannajaryan]

I have a hard time reviewing the code due to all the junk codecov added. I am going to disable it for now.

[tigrannajaryan]

The idea to pass the context to logging functions sounds good to me. I am not sure we need a separate set of functions for that. Perhaps just add context parameter to existing functions?

[jaronoff97]

@tigrannajaryan sounds good! I'll update to do that.

[mwear]

One benefit to the current approach is that the interface matches the new Go structured logger, slog. Rather than adding context to the existing functions, it might make more sense to keep the context versions, and remove (or deprecate) the non-context versions. That way you can use any logger that is slog compatible.

[jaronoff97]

yeah that was the intended purpose... I see benefits for both approaches... i'll follow up in slack.

[tigrannajaryan]

The Logger interface here is not a general-purpose logging API. It is on purpose minimalistic and designed to support only what is needed by opamp implementation and nothing else. If we want to pass the context I am happy to make it a required parameter, I don't see the need to give a choice (side note: I argued that slog also makes it a required parameter, but wasn't able to convince the author).

I prefer to keep Logger interface as small as possible and bridge to any other logging API as needed. Coding a bridge from Logger to slog is so easy that I don't see the need to make the interface identical.

We also do not have stability promises in this repo yet, so I am not too worried about making a breaking change that is very easy to adopt. I don't think we need a deprecations process for this.

[jaronoff97]

done! I will also work on the follow up described here which will make these even more valuable.

[tigrannajaryan]

Mostly LGTM, except I think we don't need the Infof() for now. Can be added later when needed.

[jaronoff97]

the codecov is failing for probably obvious reasons – testing error logs isn't really done right now

[tigrannajaryan]

This is a breaking change in the API, allowed because the API is not declared stable yet.