Add CheckRedirect callback #269
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 77.29% 77.71% +0.42%
==========================================
Files 25 25
Lines 2281 2324 +43
==========================================
+ Hits 1763 1806 +43
Misses 410 410
Partials 108 108 ☔ View full report in Codecov by Sentry. |
echlebek
force-pushed
the
better-opamp-redirects
branch
from
c19b44d to
2b91867
Compare
|
@tigrannajaryan Apologies, I had actually forgotten to implement the HTTP client logic and tests, so it took a bit longer. However, I think now the feature is complete for both WS and HTTP clients. Please take another look when you get a chance. |
tigrannajaryan
added a commit
to tigrannajaryan/opamp-go
that referenced
this pull request
Dec 18, 2024
The interface has the following downsides: - Impossible to define non-trivial default behavior. Here is an example where it was needed: open-telemetry#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 braking change, existing users won't be affected.
tigrannajaryan
added a commit
to tigrannajaryan/opamp-go
that referenced
this pull request
Dec 18, 2024
The interface has the following downsides: - Impossible to define non-trivial default behavior. Here is an example where it was needed: open-telemetry#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 braking change, existing users won't be affected.
tigrannajaryan
added a commit
that referenced
this pull request
Dec 19, 2024
The interface has the following downsides: - Impossible to define non-trivial default behavior. Here is an example where it was needed: #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 braking change, existing users won't be affected.
tigrannajaryan
added a commit
to tigrannajaryan/opamp-go
that referenced
this pull request
Dec 19, 2024
This continues work start in open-telemetry#324 for Client. The interface has the following downsides: - Impossible to define non-trivial default behavior. Here is an example where it was needed: open-telemetry#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 braking change, existing users won't be affected.
Verifies that redirect chains work
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.
echlebek
force-pushed
the
better-opamp-redirects
branch
from
a461f1f to
b711c39
Compare
This commit adds support for a CheckRedirect callback to the HTTP opamp client. It also unifies the API for CheckRedirect between WS and HTTP, so that the same callback can be used in either circumstance. Signed-off-by: Eric Chlebek <[email protected]>
echlebek
force-pushed
the
better-opamp-redirects
branch
from
b711c39 to
22851bb
Compare
|
Please forgive a bit of churn as I fix up some issues found after rebasing. |
|
I think the PR is ready for review again. |
tigrannajaryan
approved these changes
Dec 19, 2024
srikanthccv
reviewed
Dec 27, 2024
| { | ||
| Name: "check redirect returns error", | ||
| Redirector: redirectServer("http://"+redirectee.Endpoint, 302), | ||
| MockRedirect: mockRedirect(t, 1, errors.New("hello")), |
There was a problem hiding this comment.
should this be mockRedirectHTTP?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.