Add CheckRedirect callback

[echlebek]

This commit adds a CheckRedirect callback that opamp-go will call before
following a redirect from the server it's trying to connect to. Like in
net/http, CheckRedirect can be used to observe the request chain that
the client is taking while attempting to make a connection.

The user can optionally terminate redirect following by returning an
error from CheckRedirect.

Unlike in net/http, the via parameter for CheckRedirect is a slice of
responses. Since the user would have no other way to access these in the
context of opamp-go, CheckRedirect makes them available so that users
can know exactly what status codes and headers are set in the response.

Another small improvement is that the error callback is no longer called
when redirecting. This should help to prevent undue error logging by
opamp-go consumers. Since the CheckRedirect callback is now available,
it also doesn't represent any loss in functionality to opamp-go
consumers.

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 75.37%. Comparing base (c7fc585) to head (c19b44d).

Files	Patch %	Lines
client/wsclient.go	92.85%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
+ Coverage   75.07%   75.37%   +0.29%     
==========================================
  Files          25       25              
  Lines        1657     1677      +20     
==========================================
+ Hits         1244     1264      +20     
  Misses        300      300              
  Partials      113      113              

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

[tigrannajaryan]

Is it valid to return ErrUseLastResponse? I assume it is not valid at least for WS transport since it typically means there is no WS connection established.

[tigrannajaryan]

For clarity we should probably also link to https://pkg.go.dev/net/http#Client.CheckRedirect somewhere in the comment above.

[tigrannajaryan]

This add CheckRedirect to Callbacks but implements it only for wsClient. I think we also need an implementation for httpClient.

[echlebek]

@tigrannajaryan Thank you for the review!

Regarding ErrUseLastResponse, I would agree it is not applicable here. I think it should result in the same behaviour as returning any other error, however. I'll mention it specifically in the docs.

I agree that we should have an implementation for httpClient. Perhaps the name of this callback should be WSCheckRedirect or similar, denoting that it is only for websocket clients. I don't think the function signature, as implemented, is valid for HTTP clients.

I looked at httpsender and it uses the default HTTP client (with an override method for TLS config). Perhaps httpClient could accept a net/http.Client supplied by the library consumer, instead of having a specific API for things like CheckRedirect. Then the API surface would not grow, and library consumers would be able to use any custom configuration of the HTTP client they desire. Any thoughts on this?

[tigrannajaryan]

I agree that we should have an implementation for httpClient. Perhaps the name of this callback should be WSCheckRedirect or similar, denoting that it is only for websocket clients. I don't think the function signature, as implemented, is valid for HTTP clients.

Why is it not valid for HTTP clients?

Perhaps httpClient could accept a net/http.Client supplied by the library consumer, instead of having a specific API for things like CheckRedirect. Then the API surface would not grow, and library consumers would be able to use any custom configuration of the HTTP client they desire. Any thoughts on this?

Yes, this is a good option and we do something similar in the Server implementation, where you can use your own http.Server:

opamp-go/server/server.go

Line 69 in c7fc585

Attach(settings Settings) (HTTPHandlerFunc, ConnContext, error)

[echlebek]

HTTP clients should rightly expect to have the signature func(*http.Request, []*http.Request) error, as that would be 1:1 with net/http.

[tigrannajaryan]

HTTP clients should rightly expect to have the signature func(*http.Request, []*http.Request) error, as that would be 1:1 with net/http.

Do we have the need to use a different signature for WS? I see it is mentioned in the comment but it is not clear to me how exactly you would use the response for WS and why it is useful for WS and not for http.

Ideally we should use the same signature as the standard lib and make this setting applicable to both WS and plain HTTP implementations.

[echlebek]

I think there is a need for situations where the opamp-go users wants to know what status the server wrote. 301 vs 302 for instance, where the client may want to cache permanent redirects but not cache temporary redirects. Since the opamp-go user isn't privy to the HTTP response, there is no other opportunity to know that information.

I'm definitely open to another design, as I agree it would be better to maintain parity with net/http in general.

[tigrannajaryan]

@echlebek Sorry for long pause, I finally got back to this PR.

What do you think if we change CheckRedirect to look like this:

CheckRedirect(req *http.Request, viaReq []*http.Request, viaResp []*http.Response) error

The wsClient will pass both viaReq and viaResp parameters, while httpClient will only pass viaReq and will leave viaResp nil.