Skip to content

Fix OTLP marshaling for empty string attributes #8014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
trask wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix OTLP marshaling for empty string attributes #8014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5468dc5
test
trask Jan 26, 2026
904396b
fix
trask Jan 26, 2026
11b7188
add back marshalStringNoAllocation condition
trask Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,16 @@ public void serializeStringWithContext(
if (string == null || string.isEmpty()) {
return;
}
writeStringWithContext(field, string, context);
}

/**
* Writes a protobuf {@code string} field, even if it matches the default value. This method reads
* elements from context, use together with {@link StatelessMarshalerUtil#getUtf8Size(String,
* MarshalerContext)}.
*/
public void writeStringWithContext(ProtoFieldInfo field, String string, MarshalerContext context)
throws IOException {
if (context.marshalStringNoAllocation()) {
writeString(field, string, context.getSize(), context);
} else {
Expand Down
12 changes: 10 additions & 2 deletions ...mmon/src/main/java/io/opentelemetry/exporter/internal/marshal/StatelessMarshalerUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,22 @@ public static int sizeStringWithContext(
if (value == null || value.isEmpty()) {
return sizeBytes(field, 0);
}
return sizeBytes(field, getUtf8Size(value, context));
}

/**
* Returns the UTF-8 size of a string, adding data to the context for later serialization. Use
* together with {@link Serializer#writeString(ProtoFieldInfo, String, int, MarshalerContext)}.
*/
public static int getUtf8Size(String value, MarshalerContext context) {
if (context.marshalStringNoAllocation()) {
int utf8Size = context.getStringEncoder().getUtf8Size(value);
context.addSize(utf8Size);
return sizeBytes(field, utf8Size);
return utf8Size;
} else {
byte[] valueUtf8 = MarshalerUtil.toBytes(value);
context.addData(valueUtf8);
return sizeBytes(field, valueUtf8.length);
return valueUtf8.length;
}
}

Expand Down
6 changes: 0 additions & 6 deletions ...common/src/main/java/io/opentelemetry/exporter/internal/otlp/StringAnyValueMarshaler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,10 @@ static MarshalerWithSize create(String value) {

@Override
public void writeTo(Serializer output) throws IOException {
if (valueUtf8.length == 0) {
return;
}
output.writeString(AnyValue.STRING_VALUE, valueUtf8);
}

private static int calculateSize(byte[] valueUtf8) {
if (valueUtf8.length == 0) {
return 0;
}
return AnyValue.STRING_VALUE.getTagSize()
+ CodedOutputStream.computeByteArraySizeNoTag(valueUtf8);
}
Expand Down
8 changes: 6 additions & 2 deletions ...c/main/java/io/opentelemetry/exporter/internal/otlp/StringAnyValueStatelessMarshaler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.exporter.internal.otlp;

import io.opentelemetry.exporter.internal.marshal.CodedOutputStream;
import io.opentelemetry.exporter.internal.marshal.MarshalerContext;
import io.opentelemetry.exporter.internal.marshal.Serializer;
import io.opentelemetry.exporter.internal.marshal.StatelessMarshaler;
Expand All @@ -26,11 +27,14 @@ private StringAnyValueStatelessMarshaler() {}
@Override
public void writeTo(Serializer output, String value, MarshalerContext context)
throws IOException {
output.serializeStringWithContext(AnyValue.STRING_VALUE, value, context);
output.writeStringWithContext(AnyValue.STRING_VALUE, value, context);
}

@Override
public int getBinarySerializedSize(String value, MarshalerContext context) {
return StatelessMarshalerUtil.sizeStringWithContext(AnyValue.STRING_VALUE, value, context);
int utf8Size = StatelessMarshalerUtil.getUtf8Size(value, context);
return AnyValue.STRING_VALUE.getTagSize()
+ CodedOutputStream.computeUInt32SizeNoTag(utf8Size)
+ utf8Size;
Copy link
Member

@jack-berg jack-berg Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the new implementations are performing the function of sizeStringWithContext, but without the if (value == null || value.isEmpty()) short circuiting:

  public static int sizeStringWithContext(
      ProtoFieldInfo field, @Nullable String value, MarshalerContext context) {
    if (value == null || value.isEmpty()) {
      return sizeBytes(field, 0);
    }
    if (context.marshalStringNoAllocation()) {
      int utf8Size = context.getStringEncoder().getUtf8Size(value);
      context.addSize(utf8Size);
      return sizeBytes(field, utf8Size);
    } else {
      byte[] valueUtf8 = MarshalerUtil.toBytes(value);
      context.addData(valueUtf8);
      return sizeBytes(field, valueUtf8.length);
    }
  }

What about the if (context.marshalStringNoAllocation()) {} else {} block? Would it be better to include an overload of serializeStringWithContext / sizeStringWithContext which ignores the if (value == null || value.isEmpty()) short circuiting?

Copy link
Member Author

@trask trask Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, for some reason I thought the stateless marshalers was always no allocation

fixed in 11b7188

}
}
18 changes: 17 additions & 1 deletion ...otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/ValueMarshalerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,23 @@ private static Stream<Arguments> serializeAnyValueArgs() {
of("hello world".getBytes(StandardCharsets.UTF_8)),
AnyValue.newBuilder()
.setBytesValue(ByteString.copyFrom("hello world".getBytes(StandardCharsets.UTF_8)))
.build()));
.build()),
// empty values
arguments(of(""), AnyValue.newBuilder().setStringValue("").build()),
arguments(of(false), AnyValue.newBuilder().setBoolValue(false).build()),
arguments(of(0), AnyValue.newBuilder().setIntValue(0).build()),
arguments(of(0.0), AnyValue.newBuilder().setDoubleValue(0.0).build()),
arguments(
Value.of(Collections.emptyList()),
AnyValue.newBuilder().setArrayValue(ArrayValue.newBuilder().build()).build()),
arguments(
of(Collections.emptyMap()),
AnyValue.newBuilder().setKvlistValue(KeyValueList.newBuilder().build()).build()),
arguments(of(new byte[0]), AnyValue.newBuilder().setBytesValue(ByteString.EMPTY).build())
// TODO add test for true empty value
// after https://github.com/open-telemetry/opentelemetry-java/pull/7973
// arguments(Value.empty(), AnyValue.newBuilder().build())
);
}

@SuppressWarnings("unchecked")
Expand Down
24 changes: 23 additions & 1 deletion ...c/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,14 @@ void toProtoSpan(MarshalerSource marshalerSource) {
.put("long_array", 12L, 23L)
.put("double_array", 12.3, 23.1)
.put("boolean_array", true, false)
.put("empty_string", "")
.put("false_value", false)
.put("zero_int", 0L)
.put("zero_double", 0.0)
// TODO: add empty array, empty map, empty bytes, and true empty value
// after https://github.com/open-telemetry/opentelemetry-java/pull/7973
.build())
.setTotalAttributeCount(9)
.setTotalAttributeCount(13)
.setEvents(
Collections.singletonList(
EventData.create(12347, "my_event", Attributes.empty())))
Expand Down Expand Up @@ -231,6 +237,22 @@ void toProtoSpan(MarshalerSource marshalerSource) {
.addValues(AnyValue.newBuilder().setBoolValue(false).build())
.build())
.build())
.build(),
KeyValue.newBuilder()
.setKey("empty_string")
.setValue(AnyValue.newBuilder().setStringValue("").build())
Copy link
Member

@jack-berg jack-berg Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC correctly, this is the only new difference resulting from this PR. The other test cases are demonstrating existing logic.

What's the current behavior of empty string? Presumably it would just be some sort of null / emtpy value, like AnyValue.newBuilder().build()?

What's the spec language that leads to the conclusion that empty strings should be included vs. omitted? I see this line that empty values are meaningful and must be passed to exporters, but should the OTLP exporter serialize those or omit?

Copy link
Member Author

@trask trask Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC correctly, this is the only new difference resulting from this PR. The other test cases are demonstrating existing logic.

👍

What's the current behavior of empty string? Presumably it would just be some sort of null / emtpy value, like AnyValue.newBuilder().build()?

👍

What's the spec language that leads to the conclusion that empty strings should be included vs. omitted? I see this line that empty values are meaningful and must be passed to exporters, but should the OTLP exporter serialize those or omit?

to me it follows from protobuf specification itself https://protobuf.dev/programming-guides/proto3/#oneof

If you set a oneof field to the default value (such as setting an int32 oneof field to 0), the “case” of that oneof field will be set, and the value will be serialized on the wire.

(sorry, should have linked to open-telemetry/opentelemetry-specification#4660 (comment) in case you hadn't seen that)

Copy link
Member

@jack-berg jack-berg Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry, should have linked to open-telemetry/opentelemetry-specification#4660 (comment) in case you hadn't seen that)

Ah got it! Makes sense.

.build(),
KeyValue.newBuilder()
.setKey("false_value")
.setValue(AnyValue.newBuilder().setBoolValue(false).build())
.build(),
KeyValue.newBuilder()
.setKey("zero_int")
.setValue(AnyValue.newBuilder().setIntValue(0).build())
.build(),
KeyValue.newBuilder()
.setKey("zero_double")
.setValue(AnyValue.newBuilder().setDoubleValue(0.0).build())
.build());
assertThat(protoSpan.getDroppedAttributesCount()).isEqualTo(1);
assertThat(protoSpan.getEventsList())
Expand Down
Loading