Fix OTLP marshaling for empty string attributes

[trask]

Based on 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.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.18%. Comparing base (fd0ffde) to head (11b7188).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...orter/internal/marshal/StatelessMarshalerUtil.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8014      +/-   ##
============================================
+ Coverage     90.14%   90.18%   +0.03%     
- Complexity     7476     7486      +10     
============================================
  Files           834      836       +2     
  Lines         22540    22563      +23     
  Branches       2236     2235       -1     
============================================
+ Hits          20319    20348      +29     
+ Misses         1516     1514       -2     
+ Partials        705      701       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jack-berg]

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?

[trask]

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)

[jack-berg]

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

Ah got it! Makes sense.

[jack-berg]

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?

[trask]

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

fixed in 11b7188