Breaking: Replace Callbacks interface by Callbacks struct

[tigrannajaryan]

The interface has the following downsides:

• Impossible to define non-trivial default behavior. Here is an example where it was needed: Add CheckRedirect callback #269 (comment)
• Adding new callbacks requires expanding the interface, which is a breaking change for existing client users.

Getting rid of the interface and keeping just a struct for callbacks solves both problems:

• Arbitrarily complex default behavior can be now defined on the struct if the user does not provide the particular callback func.
• Adding new callback funcs is not a breaking change, existing users won't be affected.

[codecov]

Codecov Report

Attention: Patch coverage is 88.65979% with 11 lines in your changes missing coverage. Please review.

Project coverage is 77.29%. Comparing base (f7d56df) to head (2204615).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
client/internal/receivedprocessor.go	90.90%	4 Missing and 3 partials ⚠️
client/types/callbacks.go	76.47%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   77.24%   77.29%   +0.04%     
==========================================
  Files          25       25              
  Lines        2303     2281      -22     
==========================================
- Hits         1779     1763      -16     
+ Misses        420      410      -10     
- Partials      104      108       +4     

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

[tigrannajaryan]

@open-telemetry/opamp-go-approvers please take a look, I would like to know what your opinion is.

[tigrannajaryan]

If we like this direction I will also make similar changes to Server callbacks.

[echlebek]

Thanks @tigrannajaryan, I think this is a nice simplification of the callbacks API!

I might suggest preserving the CallbacksStruct type name (although the name is a bit awkward now) for backwards compatibility.

Perhaps add a type alias, to avoid breaking the API for people using CallbacksStruct. It could also be given a "Deprecated" comment, so that users can be made aware of its deprecated status by static analysis tools. (https://go.dev/wiki/Deprecated)

// Callbacks is blah blah
type Callbacks struct { ... }

// CallbacksStruct is an alias for Callbacks
// Deprecated: CallbacksStruct will be removed in the future
type CallbacksStruct = Callbacks

[andykellr]

Thanks, LGTM

[tigrannajaryan]

I might suggest preserving the CallbacksStruct type name (although the name is a bit awkward now) for backwards compatibility.

I am not sure about this. It is not just the struct name, we also have to preserve the field names, which all have the "Func" suffix now, otherwise it is a breaking change anyway.

[echlebek]

That's a good point, I didn't notice the field names had changed as well.