Skip to content

Conversation

@jsuereth
Copy link
Contributor

Fixes an issue where repeated strings would serialize as the same JSON field multiple times (the same encoding used in binary proto).

This is because in JSON string is a primitive, but not in binary Protocol buffers.

E.g. instead of stringTable: ["one", "two"],, we'd get stringTable: "one", stringTable: "two".

  • Adds serializeRepeatedString abstract method which can appropriately handled string arrays in JSON.
  • Adds new sizeRepatedString for solidarity with other methods.

For maintainers - This code relies on existing protobuf tests. If you'd like to see the serialization primitives start to have test cases, I'd have to build up a test suite for them but happy to do so.

@jsuereth jsuereth requested a review from a team as a code owner November 18, 2024 15:10
@codecov
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (9a0bfb2) to head (b9176bf).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
...etry/exporter/internal/marshal/JsonSerializer.java 0.00% 5 Missing ⚠️
...metry/exporter/internal/marshal/MarshalerUtil.java 0.00% 4 Missing ⚠️
...elemetry/exporter/internal/marshal/Serializer.java 0.00% 4 Missing ⚠️
...try/exporter/internal/marshal/ProtoSerializer.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6888      +/-   ##
============================================
- Coverage     90.26%   90.23%   -0.03%     
- Complexity     6586     6594       +8     
============================================
  Files           729      729              
  Lines         19767    19800      +33     
  Branches       1944     1947       +3     
============================================
+ Hits          17842    17867      +25     
- Misses         1334     1341       +7     
- Partials        591      592       +1     

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


🚨 Try these New Features:

@trask
Copy link
Member

trask commented Nov 18, 2024

cc @jhalliday

@jack-berg jack-berg force-pushed the wip-support-repeated-string branch from b6d8daf to b9176bf Compare November 22, 2024 16:33
.build())
.addAllComment(listOf(8L, 9L))
.addStringTable("foo")
.addStringTable("bar")
Copy link
Member

Choose a reason for hiding this comment

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

This adds test coverage for the binary encoding. Making this change on main causes tests to fail, but the tests pass on this branch, indicating your fix is successful.

@jack-berg jack-berg merged commit 8a3329b into open-telemetry:main Nov 25, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants