Update RestClientBeanPostProcessor/WebClientBeanPostProcessor to create new bean only when adding OTEL Interceptor#15546
Conversation
6850089 to
c528516
Compare
b418b63 to
c7b9736
Compare
9aa1769 to
92119a4
Compare
|
This PR has been labeled as stale due to lack of activity and needing author feedback. It will be automatically closed if there is no further activity over the next 7 days. |
|
This PR has been labeled as stale due to lack of activity and needing author feedback. It will be automatically closed if there is no further activity over the next 7 days. |
|
@lenin-jaganathan in case you might not have noticed, there are some merge conflicts that need to be resolved |
933565b to
69a6c45
Compare
Just now coming off a break and saw it. I have fixed it and pushed changes. |
| @Override | ||
| public Object postProcessAfterInitialization(Object bean, String beanName) { | ||
| if (bean instanceof WebClient) { | ||
| WebClient webClient = (WebClient) bean; |
There was a problem hiding this comment.
Do I understand correctly that the intention here is that the interceptor will be added here when the WebClient wasn't created by spring. For example when there is something like
@Bean
WebClient webClient() {
return WebClient.create();
}I'd guess the WebClientCustomizer won't run then. What if there is
@Bean
WebClient.Builder webClientBuilder() {
return WebClient.builder();
}I'd guess the WebClientCustomizer won't run then either. Should we handle that? Should this be aligned with RestClientBeanPostProcessor (cc @zeitlinger, do you remember why you wrote RestClientBeanPostProcessor the way it is).
Assuming that the intent is to add the interceptor in post processor when the customizer didn't run do we have any tests for this?
There was a problem hiding this comment.
My goal was to make sure that we add the interceptor in every possible way that you can do it - either by creating a builder or the client bean directly.
I did add unit and smoke tests - but maybe I missed a case: #11038
There was a problem hiding this comment.
Sorry for not getting back here.
I'd guess the WebClientCustomizer won't run then either. Should we handle that? Should this be aligned with RestClientBeanPostProcessor
The existing behaviour was RestClient.Builder was not processed by a BPP but by WebClient.Builder was. The current state was that neither builder was handled in that case. I think this is an expected pattern in Spring Boot when a Builder bean is created; the choice to apply customizer beans lies with the creator, consistent with how other customizers would be treated in the Spring Boot world.
Open to discuss otherwise, too.
…ceptor already present The RestClientBeanPostProcessor was always creating a new RestClient bean via mutate().build(), even when the OpenTelemetry interceptor was already present. This resulted in unnecessary bean creation. Fixes open-telemetry#15545 Signed-off-by: Lenin Jaganathan<[email protected]>
Signed-off-by: Lenin Jaganathan <[email protected]>
Signed-off-by: Lenin Jaganathan <[email protected]> Signed-off-by: Lenin Jaganathan <[email protected]>
Signed-off-by: Lenin Jaganathan <[email protected]>
Signed-off-by: Lenin Jaganathan <[email protected]>
Signed-off-by: Lenin Jaganathan <[email protected]>
79ec6d8 to
047841e
Compare
|
Thank you for your contribution @lenin-jaganathan! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
The RestClientBeanPostProcessor was always creating a new RestClient bean via mutate().build(), even when the OpenTelemetry interceptor was already present. This resulted in unnecessary bean creation.
Fixes #15545