zipkin.ThriftSpanConverter does not truncate tags

[mabn]

I believe that the intention was to truncate tags to MAX_TAG_LENGTH chars, but the truncated value is ignored. tagValue should not be modified.

    if (stringTagValue.length() > Constants.MAX_TAG_LENGTH) {
      tagValue = stringTagValue.substring(0, Constants.MAX_TAG_LENGTH);
    }
    banno.setValue(stringTagValue.getBytes(UTF_8)).setAnnotation_type(AnnotationType.STRING);

https://github.com/uber/jaeger-client-java/blob/a40d353e14902e458ff8cc742d75a57ab8fe7321/jaeger-zipkin/src/main/java/com/uber/jaeger/senders/zipkin/ThriftSpanConverter.java#L140

[yurishkuro]

It's an open question if the client library should be truncating the tags to max length or not. We did this in the earlier clients, but not in the later ones, because some teams did want to store large payloads in the tags (although maybe logs are a better option for that).

It might be better NOT to truncate. If a service is storing large payloads in tags/logs, it's that service who's penalized the most in performance. And truncating can always be added on the backend, where it can be better controlled than in the client libs.

[mabn]

The thing is that right now it is inconsistent - zipkin.ThriftSpanConverter does not truncate tags (due to the mentioned issue) but JaegerThriftSpanConverter does.

JaegerThriftSpanConverter truncates logs - messages, fields and paylods.
zipkin.ThriftSpanConverter does not truncate log messages (and does not report fields/payloads).

I'm a devil's advocate here because I actually want to report longer payloads.

If the truncation stays on the client-side it might make sense to make the Constants.MAX_TAG_LENGTH configurable, maybe also introduce separate value for logs. If it doesn't then there would need to be a mechanism to work around MTU limitation of UDP - like you mentioned on the mailing list.

[yurishkuro]

Personally I think we should remove the limitation in the client libs, or like you said, make it configurable if people really want it.

[pavolloffay]

+1 it should be configurable. If nobody requested this feature let's go without truncation and add it later if necessary.