Deprecates Sender for much simpler BytesMessageSender

[codefromthecrypt]

Yesterday @shakuzen @anuraaga @kojilin and I chatted about how the async reporters never actually use the async code of the senders. The async side was only used by zipkin-server, as the async reporter design intentionally uses a blocking loop to flush the span backlog. So, this makes things simpler by only requiring a blocking implementation: BytesMessageSender.send (chosen to not create symbol collisions and similar to BytesMessageEncoder.

The side effect is that new senders can use this interface and completely avoid the complicated Call then execute chain, deferring to whatever the library-specific blocking path is. I have implemented this in armeria as a test, and it is absolutely easier.

As older spring libraries like sleuth may not upgrade, this maintains the old types until reporter 4. However, new senders can simply implement the new type and be done with it.

[codefromthecrypt]

PS this will also make things a lot easier for porting. For example, the spring boot senders are pretty much all synchronous anyway and so the code can be shredded once this is released. e.g. https://github.com/spring-projects/spring-boot/blob/7851c2362eb53c989cbdda706576c729a016bc49/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/zipkin/HttpSender.java#L43

cc @mhalbritter

[codefromthecrypt]

here's an example sender, for armeria, granted I didn't add gzip to it yet.. the lack of creating the Call objects makes it a lot easier as you'll see vs okhttp for example (not its fault, as it would also be simple if we could remove Call without deprecation here) #245

[codefromthecrypt]

almost everything implemented this with a post to empty list, and in any case check was almost never called!

[codefromthecrypt]

here's the type which defines only methods actually used

[codefromthecrypt]

retrofitted Sender as BytesMessageSender so that non-updated code can drop in and work. This is also why BytesMessageSender is an interface, as we can't extend two classes

[codefromthecrypt]

this was the only place where the sender was used for its real purpose in this repo. So, the net change here is just more efficient and straightforward of the same

[reta]

(This is just an opinion) The semantics of sending an empty list as to way to very the connection could be surprising - logically I think noop is expected. May be having a dedicated check / ping method is not as bad after all

[codefromthecrypt]

I really regret having put this check in the first place as it was a relic from finagle failfast. It isn't used for anything, but it is totally the case that sending an empty list is permitted today. I would rather document an empty list is permitted since almost all impls of check had to send an empty list. This is better than forcing people to implement check which would only do the same, and then people think it is smarter than that.

[codefromthecrypt]

please have a look at the new RATIONALE.md for notes on this!

[reta]

Thanks a lot @codefromthecrypt for very comprehensive clarification! (knowing context always pays off)

[reta]

Looks pretty clean, but letting other folks (who were part of the discussion) to take a look

[codefromthecrypt]

Thanks for the thought @reta and @anuraaga!

[codefromthecrypt]

openzipkin/brave-example#104 shows this works, so I'll release it so people can play with it, also to help with the maintenance work ahead for the spring hosted senders.

[codefromthecrypt]

Here's a working armeria sender in less than 50 lines including imports:

package brave.example;

import com.linecorp.armeria.client.WebClient;
import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpData;
import com.linecorp.armeria.common.HttpMethod;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.MediaType;
import java.io.IOException;
import java.util.List;
import zipkin2.reporter.BytesMessageEncoder;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.ClosedSenderException;
import zipkin2.reporter.Encoding;

final class WebClientSender extends BytesMessageSender.Base {
  final WebClient zipkinApiV2SpansClient;
  volatile boolean closeCalled; // volatile as not called from the reporting thread.

  WebClientSender(WebClient zipkinApiV2SpansClient) {
    super(Encoding.JSON);
    this.zipkinApiV2SpansClient = zipkinApiV2SpansClient;
  }

  @Override public int messageMaxBytes() {
    return 500_000; // Use the most common HTTP default
  }

  @Override public void send(List<byte[]> encodedSpans) throws IOException {
    if (closeCalled) throw new ClosedSenderException();
    byte[] body = BytesMessageEncoder.JSON.encode(encodedSpans);
    HttpRequest request =
        HttpRequest.of(HttpMethod.POST, "", MediaType.JSON, HttpData.wrap(body));
    AggregatedHttpResponse response = zipkinApiV2SpansClient.blocking().execute(request);
    try (HttpData content = response.content()) {
      if (!response.status().isSuccess()) {
        if (content.isEmpty()) {
          throw new IOException("response failed: " + response);
        }
        throw new IOException("response failed: " + content.toStringAscii());
      }
    }
  }

  @Override public void close() {
    closeCalled = true;
  }
}