openzipkin / zipkin-reporter-java

Shared library for reporting zipkin spans on transports such as http or kafka
Apache License 2.0
126 stars 70 forks source link

Deprecates Sender for much simpler BytesMessageSender #244

Closed codefromthecrypt closed 10 months ago

codefromthecrypt commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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) https://github.com/openzipkin/zipkin-reporter-java/pull/245

reta commented 10 months ago

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

codefromthecrypt commented 10 months ago

Thanks for the thought @reta and @anuraaga!

codefromthecrypt commented 10 months ago

https://github.com/openzipkin/brave-example/pull/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 commented 10 months ago

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;
  }
}