incubator-retired-htrace

incubator-retired-htrace copied to clipboard

The thrift Annotation Type doesn't conform to the actual type it encodes. Notice the TraceScope.addKVAnnotation function signature:

public void addKVAnnotation(String key, String value)

Using StandardCharsets doesn't throw the UnsupportedEncodingException

@adriancole I wanted to but I think the difference only happens after passing through the whole thrift encoding/decoding inside zipkin collector on the zipkin side. process and I couldn't find a place this is done.

The key thing here is testing what you're asserting (ex that you fixed the bug as before we weren't encoding strings properly).

Here's a test that should cover it.

  @Test
  public void testKVSerializesAsUTF8String() throws IOException, TException {
    Span span = new MilliSpan.Builder().
        description(ROOT_SPAN_DESC).
        spanId(new SpanId(100, 100)).
        tracerId("test").
        begin(1).
        build();
    span.addKVAnnotation("http.path", "/foo/⻩");

    com.twitter.zipkin.gen.Span zs =
        new HTraceToZipkinConverter(127<<24|1, (short) 80).convert(span);

    assertEquals(1, zs.getBinary_annotations().size());
    BinaryAnnotation path = zs.getBinary_annotations().get(0);
    assertEquals(AnnotationType.STRING, path.getAnnotation_type());
    assertEquals("/foo/⻩", new String(path.getValue(), UTF_8));
  }

@adriancole The test does cover it and I wrote a test a different test before sending the PR but it wouldn't fail.

I'm pretty sure the problem lies in https://github.com/openzipkin/zipkin/blob/master/zipkin/src/main/java/zipkin/internal/ThriftCodec.java#L244

Where the type should be inferred as String but was inferred as Bytes

 @Test
    public void testBinaryAnnotationEncoding() throws IOException, InterruptedException {
        FakeZipkinTransport transport = new FakeZipkinTransport();
        Tracer tracer = newTracer(transport);
        Span rootSpan = new MilliSpan.Builder().
                description("root").
                spanId(new SpanId(100, 100)).
                tracerId("test").
                begin(System.currentTimeMillis()).
                build();
        TraceScope rootScope = tracer.newScope("root");
        rootScope.addKVAnnotation("foo", "bar");
        rootScope.close();
        com.twitter.zipkin.gen.BinaryAnnotation binaryAnnotations = transport.nextMessageAsSpan().getBinary_annotations().get(0);
        Assert.assertTrue(binaryAnnotations.getKey().equals("foo"));
        Assert.assertTrue(new String(binaryAnnotations.getValue()).equals("bar"));
        tracer.close();
    }

Or do we place the test there to enforce this change?

I'd recommend using the test I wrote as it would fail before this change, particularly as the wrong type was used. merging code with no tests isn't a good plan, especially when you are handed one!

@adriancole I totally agree, I just wanted to make sure I am testing for the right things.

@raam86 yeah I would consider this backfilling a unit test vs adding an end-to-end test (which this isn't). It is still valuable.

If you want an end-to-end test, you could add a dependency on zipkin-collector-scribe and use that directly. something like this https://github.com/openzipkin/zipkin-reporter-java/blob/master/libthrift/src/test/java/zipkin/reporter/libthrift/LibthriftSenderTest.java

Thank you for the suggestion, @adriancole. I've added the test but I still can't make it fail when changing encoding type.

In the zipkin frontend this change definitely fixes the reported bug but I can't find a way to reproduce it in a test.

I Hope it's just the beginning 👯