PR Description

  • Introduce abstract HttpSender class
  • Introduce ZipkinWebClientSender
  • Add ZipkinWebClientSender bean to configuration

related to #30661

Comment From: pivotal-cla

@StefanBratanov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-cla

@StefanBratanov Thank you for signing the Contributor License Agreement!

Comment From: StefanBratanov

Hi, just wondering if there an update for this PR? Thanks.

Comment From: wilkinsona

Nothing at the moment, @StefanBratanov. Moritz is out for a few weeks on paternity leave so it may take us a little while to get to it. 3.0 doesn't GA until November so there's no rush at this stage.

Comment From: StefanBratanov

Nothing at the moment, @StefanBratanov. Moritz is out for a few weeks on paternity leave so it may take us a little while to get to it. 3.0 doesn't GA until November so there's no rush at this stage.

Sure. No worries. Thank you for quick update. Congratulations to Moritz. 🙂

Comment From: mhalbritter

Hey, thanks for the contribution. I'm back and have some comments on the code.

Comment From: StefanBratanov

Hey, thanks for the contribution. I'm back and have some comments on the code.

Welcome back! Thank you. I will have a look.

Comment From: StefanBratanov

Hi Moritz, sorry for the delay. I did make a refactor based on the comments, which i agree with. I wasn't sure about handling the WebClient async call, but now i think it makes sense and is cleaner. I also changed the ZipkinRestTemplateSender async implementation to use a CompletableFuture since it was blocking before. I also added tests for both sync and async in both implementations.

I am not sure about initializing the WebClient in ZipkinConfigurations. I was using the ReactorClientHttpConnector which is based onNetty, so that i can honor the read and connect timeouts set in ZipkinProperties. What do you think is a better approach for initializing the WebClient? Also i couldn't find a mock server similar to MockRestServiceServer for WebClient, so kept my old implementation. If there is one, I am happy to change it.

Comment From: mhalbritter

Thanks @StefanBratanov for the changes, the PR looks quite nice now!

Hi Moritz, sorry for the delay. I did make a refactor based on the comments, which i agree with. I wasn't sure about handling the WebClient async call, but now i think it makes sense and is cleaner. I also changed the ZipkinRestTemplateSender async implementation to use a CompletableFuture since it was blocking before. I also added tests for both sync and async in both implementations.

No need to be sorry :)

I don't think we need that CompletableFuture handling. The UrlConnectionSender from zipkin implements the enqueue call without additional threading, so we should be fine here without introducing multi-threading.

I am not sure about initializing the WebClient in ZipkinConfigurations. I was using the ReactorClientHttpConnector which is based onNetty, so that i can honor the read and connect timeouts set in ZipkinProperties. What do you think is a better approach for initializing the WebClient?

I have to double check if there's really no way to set timeouts without tying us directly on the implementation.

Also i couldn't find a mock server similar to MockRestServiceServer for WebClient, so kept my old implementation. If there is one, I am happy to change it.

Yes, I was wrong, there's no such support in the Spring Framework. The recommendation is to use the OkHttp MockServer, like you did.

Comment From: StefanBratanov

Hi @mhalbritter thanks for review, I replaced theCompletableFuture implementation with a normal one. I will wait for you to check about the WebClient initialization. My logic was that @ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.REACTIVE) in ZipkinConfigurations should mean that Netty should be on the classpath when true. I could be wrong though.

Comment From: mhalbritter

Unfortunately, this is not the case. You can use other servers for reactive applications.

Spring Boot includes support for the following embedded reactive web servers: Reactor Netty, Tomcat, Jetty, and Undertow

The best option would be to have timeout setter on the WebClient.Builder, but unfortunately this is not available and won't be.

I would drop the timeout code in the configuration for now, and I will bring this up with the team if we can find a solution which works for all the connector implementations.

Comment From: StefanBratanov

Unfortunately, this is not the case. You can use other servers for reactive applications.

Spring Boot includes support for the following embedded reactive web servers: Reactor Netty, Tomcat, Jetty, and Undertow

The best option would be to have timeout setter on the WebClient.Builder, but unfortunately this is not available and won't be.

I would drop the timeout code in the configuration for now, and I will bring this up with the team if we can find a solution which works for all the connector implementations.

Got it. Makes sense. Good to know! The default builder will take care of initialising the appropriate connector. I removed setting the Netty connector explicitly.

Comment From: mhalbritter

I've merged your PR. Thanks for your work and congratulations on your first contribution to Spring Boot!

Comment From: StefanBratanov

I've merged your PR. Thanks for your work and congratulations on your first contribution to Spring Boot!

Thank you! It was fun. :) Looking forward to doing more in the future.