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 accessor for logPrefix in ClientResponse to allow tying a ClientRequest to a ClientResponse #24146

Closed
epiwd opened this issue Dec 6, 2019 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@epiwd
Copy link

epiwd commented Dec 6, 2019

Affects: spring-framework:spring-webflux:5.1.9.RELEASE


Hello,

I have a use case while making webclient requests where we use parts of the clientrequest and clientresponse together to form a log. In the clientRequest, logPrefix is an accessible unique identifier that is passed from the request to the response, yet when we try to do the same for the clientResponse, the field is present but not accessible.

We've considered a workaround where we create a custom ExchangeFunction that forces the logPrefix id into the headers of the clientRequest so that it can be read in the response, but even then, DefaultClientResponse is package-protected, which makes creating the ExchangeFunction difficult.

There is also a possibility we've overlooked a solution for this, so if there's a way to easily tie together a webclient request and response, please let me know.

If not, our proposed fix is simply to expose the logPrefix like all the other clientResponse fields.

Thank you!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 6, 2019
@poutsma poutsma self-assigned this Dec 6, 2019
@poutsma
Copy link
Contributor

poutsma commented Dec 6, 2019

I don't it's a good idea to expose the logPrefix field in the ClientResponse interface, as it's an implementation detail of DefaultClientResponse that has does not deserve an accessor.

We could, however, expose the HttpRequest that is available in DefaultClientResponse as well. Would that help?

@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue labels Dec 6, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 6, 2019

The logPrefix() in ClientRequest, as well as in ServerWebExchange on the server side, is meant to provide applications and the framework with a common handle for logging, that applications can also control via a well-known request attribute. As such it is part of the public API. From that perspective perhaps it does make sense to expose via ClientResponse. It could be documented as the logPrefix used for the request, if any.

@epiwd
Copy link
Author

epiwd commented Dec 6, 2019

If the HttpRequest were available in the ClientResponse, that would work as well.

(To be even more explicit, we're trying to use ExchangeFilterFunctions to create a log line using both the request and response, but there's seemingly nothing built-in that can tie a request to a response.)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 6, 2019
@rstoyanchev
Copy link
Contributor

but there's seemingly nothing built-in that can tie a request to a response

In the public API you mean. Indeed it does seem like a missing link there for further logging downstream, e.g. in an onStatus handler or when handling a ClientResponseStatusException.

@poutsma poutsma added this to the 5.2.3 milestone Dec 13, 2019
@poutsma poutsma added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 13, 2019
@poutsma
Copy link
Contributor

poutsma commented Dec 13, 2019

I did not know that ClientResponse already exposed the logPrefix, so for consistency's sake I added it to ClientResponse as well.

If you (or anyone else) would like to see ClientResponse expose the HttpRequest as well, then please file a separate issue for that.

@epiwd
Copy link
Author

epiwd commented Dec 13, 2019

Thanks for the fast turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants