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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (fd0ffde) to head (904396b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8014      +/-   ##
============================================
+ Coverage     90.14%   90.19%   +0.04%     
- Complexity     7476     7479       +3     
============================================
  Files           834      834              
  Lines         22540    22539       -1     
  Branches       2236     2236              
============================================
+ Hits          20319    20329      +10     
+ Misses         1516     1509       -7     
+ 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?