Allow empty strings to be written to protobuf #7656
Conversation
Yup that's exactly what changed between 1.48-alpha and 1.49-alpha. See open-telemetry/opentelemetry-java-contrib#2138 |
| // time. If the lengths are equal and the resulting protos are equal, the marshaling is | ||
| // guaranteed to be valid. | ||
| assertThat(result.getSerializedSize()).isEqualTo(serialized.length); | ||
| // assertThat(result.getSerializedSize()).isEqualTo(serialized.length); |
There was a problem hiding this comment.
It really weirds me out that this would change...but something about the defaults causes this to fail now? Anybody know why?
| KeyValue.newBuilder() | ||
| .setKey("key") | ||
| .setValue(AnyValue.newBuilder().setStringValue("value").build()) | ||
| .build()) |
There was a problem hiding this comment.
Now that there's more than one attribute, we can't rely on the order and this test fails, so it had to be reworked to sort.
|
Ok, I'm still very much still open to feedback on this PR, but it's very broken for some tricky reasons. After talking with @jsuereth, I have opened this spec issue: open-telemetry/opentelemetry-specification#4660 Going to close for now and pursue another approach in contrib. |
2 participants
Related to open-telemetry/opentelemetry-java-contrib#2257
It looks like there's a small issue around serializing string Attributes that contain an empty string value. Per the spec, these empty string values are important:
The
Serializerclass special cases null and empty and treats them the same -- by returning early without actually serializing. In the case of protobuf, this is especially problematic because the proto field tag (which contains the type information!) doesn't get written. When that happens, the proto can no longer indicate that the field is a string. In addition to the crash/exception happening in the disk buffering code, this can also be seen by sending a span with an empty attribute value to the collector with verbose logging on:The collector shows that the field is
Empty()instead ofStr().I'm not entirely sure why this showed up recently in disk buffering, but maybe we switched the default implementation to low-allocation or something that caused this?