log and return empty mono when WebClient request fails sending zipkin trace data

[jarlehansen]

The implementation of WebClientSender can cause a lot of On error dropped stack traces if it is not able to send the trace data to the server. It is also difficult to handle these exceptions without a completely custom Sender. It would be better if the webclient handles the exception and logs the message, this avoid a lot of unnecessary error log that fills up the application logs.

This is atm only a suggestion, if it seems like a good idea we also need to rewrite a couple of tests.

[marcingrzejszczak]

Yeah, looks like a good idea, I like it.

[marcingrzejszczak]

Theoretically, if you add tests and we merge this soon, then it could be picked for today's release!

[jarlehansen]

I am not sure how to do this, to get the tests to work I have to implement the same behavior in RestTemplateSender. Which is perhaps what makes most sense?
Also, the tests that check the results should expect OK now instead of failed, because we handle the exceptions and log a message.

Let me know if you have some other suggestions on how to solve this.

[marcingrzejszczak]

Hmm come to think of it that's a breaking change. We want to know that there's sth wrong when trying to send the spans. Maybe we could change the logging level somehow but still point that the check result is not ok? WDYT?

[jarlehansen]

What if we made WebClientSender a bit more flexible, where it would be possible to add a custom Function that will be included in onErrorResume. This way you can override the Sender bean and send in your own error handling. The default error handling will remain the same, to support backwards compatibility.

See the latest commit if this might be an option?

[marcingrzejszczak]

What if instead of passing the error processing function we would give a function that would allow you to take in a Mono<ResponseEntity<Void>> and would return Mono<ResponseEntity<Void>> ? Sth like this?

private static void post(String url, MediaType mediaType, byte[] json, WebClient webClient, long checkTimeout, Function<Mono<ResponseEntity<Void>>, Mono<ResponseEntity<Void>>> function) {
		return function.apply(webClient.post().uri(URI.create(url)).accept(mediaType).contentType(mediaType).bodyValue(json).retrieve()
				.toBodilessEntity().timeout(Duration.ofMillis(checkTimeout)));
	}

That way we could inject some WebClientSenderExecutionCustomizer bean which would be injected to the WebClientSender and the user can do whatever they want with errors or other stuff. WDYT?

[jarlehansen]

That is a really good idea, please have a look at the latest commit.
Added a new test as well, for the custom error handling functionality that can be added now.

[marcingrzejszczak]

That looks good! Now we need to ensure that the autoconfiguraiton is properly setup to include that function

[jarlehansen]

It should still work, right? Since I kept the old constructor and added a new one that included the function.

@Bean(ZipkinAutoConfiguration.SENDER_BEAN_NAME)
		Sender webClientSender(ZipkinProperties zipkin, ZipkinWebClientBuilderProvider zipkinWebClientBuilderProvider) {
			WebClient.Builder webClientBuilder = zipkinWebClientBuilderProvider.zipkinWebClientBuilder();
			return new WebClientSender(webClientBuilder.build(), zipkin.getBaseUrl(), zipkin.getApiPath(),
					zipkin.getEncoder(), zipkin.getCheckTimeout());
		}

By overriding this with your own bean it should be easy to add for example error handling or retries.

[marcingrzejszczak]

Yeah, you're right. You'll just have to provide your own sender.

[marcingrzejszczak]

Great job @jarlehansen ! This will be in Sleuth 3.1.4 unfortunately (we've just done the 3.1.3 release).

[jarlehansen]

Thanks @marcingrzejszczak, too bad we did not make the 3.1.3 release but it was a good idea to get the best implementation of this and not to rush it.

Thanks for all the assistance on this feature.

[marcingrzejszczak]

Yeah, I love how the cooperation on this issue looked like - an ideal open source contribution :)