Skip to content
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.

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

Fix repeated string serialization for JSON. #6888

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

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
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 19 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

@@ -304,6 +304,8 @@ private static Profile.Builder sampleProfileBuilder() {
.AGGREGATION_TEMPORALITY_CUMULATIVE)
.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.

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