Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add methods to support more generalized interceptors #4868

Closed
shawkins opened this issue Feb 10, 2023 · 5 comments
Closed

Add methods to support more generalized interceptors #4868

shawkins opened this issue Feb 10, 2023 · 5 comments
Milestone

Comments

@shawkins
Copy link
Contributor

Is your enhancement related to a problem? Please describe

At least to support logging as an interceptor pattern, we should provide the ability for interceptors to get http responses as well.

Describe the solution you'd like

Add an afterSuccess method.

Specifically for logging we may want to think about how to provide details on payloads / responses without corrupting the streams.

Describe alternatives you've considered

No response

Additional context

No response

@manusa
Copy link
Member

manusa commented Feb 15, 2023

Relates to:

@shawkins shawkins added this to the 6.5.0 milestone Feb 20, 2023
@shawkins
Copy link
Contributor Author

It looks like we should add a method like "void afterSuccess(HttpResponse<?> response)" as you can get the request from the response. Allowing this to modify or return a different response does not seem like a good idea at this time - while there is the HttpResponseAdapter which makes it easy to change the response code / headers the only type that the response body can be is AsyncBody which would not be easy to manipulate and would like mean making another httpclient call.

However it doesn't look like this would be ideal for logging, or at least not as good as putting the logging directly in the StandardHttpClient. Several considerations are:

  • we'd want this on both the Config driven httpclient, and the one created for the kubernetes token refresh - currently the okhttp logging interceptor is only added to the former. It's a little extra work to account for the token refresh ones.
  • the way the interceptors are consulted is as a fixed ordered chain, no matter where we put the logging interceptor it will miss intermediate calls from the afterFailure of other interceptors - this would attempting the request again from the backwards compatibility interceptor or the token refresh ones.
  • it's not as clear to track the context - we don't provide any unique identifier on the httprequest, so correlating across before and the other methods isn't necessarily clear. If we add appropriate logging in the StandardHttpClient then that will be much simpler to address.

Ideally we'd want users to be able to insert their own customized logging, but of course they'll hit those same issues. It seems like we may want to decouple this from the logging issue to put in something that just works into the StandardHttpClient and make more refinements to this proposal based upon any additional feedback from users.

@aditya-32
Copy link

Hi @manusa @shawkins is anyone working on this, I want to contribute to this
can you please brief me more about the requirement?

@manusa
Copy link
Member

manusa commented Apr 28, 2023

Somewhat tackled by #5037

@manusa manusa modified the milestones: 6.6.0, 6.7.0 May 4, 2023
@manusa manusa modified the milestones: 6.7.0, 6.8.0 May 31, 2023
@shawkins
Copy link
Contributor Author

I'd say what was introduced is good enough. The only bit of messiness is the StandardHttpClientBuilder controlling the addition of the logging interceptor, but that allows us to cover both configured and unconfigured clients. Someone may eventually want to inject their own override or other simiilar interceptor so we may have to introduce another factory method to do that.

@manusa manusa modified the milestones: 6.8.0, 6.7.0 Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants