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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,22 @@
generator.writeString(string);
}

@Override
public void writeRepeatedString(ProtoFieldInfo field, byte[][] utf8Bytes) throws IOException {
generator.writeArrayFieldStart(field.getJsonName());

Check warning on line 127 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java#L127

Added line #L127 was not covered by tests
for (byte[] value : utf8Bytes) {
// Marshalers encoded String into UTF-8 bytes to optimize for binary serialization where
// we are able to avoid the encoding process happening twice, one for size computation and one
// for actual writing. JsonGenerator actually has a writeUTF8String that would be able to
// accept
// this, but it only works when writing to an OutputStream, but not to a String like we do for
// writing to logs. It's wasteful to take a String, convert it to bytes, and convert back to
// the same String but we can see if this can be improved in the future.
generator.writeString(new String(value, StandardCharsets.UTF_8));

Check warning on line 136 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java#L136

Added line #L136 was not covered by tests
}
generator.writeEndArray();
}

Check warning on line 139 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/JsonSerializer.java#L138-L139

Added lines #L138 - L139 were not covered by tests

@Override
public void writeBytes(ProtoFieldInfo field, byte[] value) throws IOException {
generator.writeBinaryField(field.getJsonName(), value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@
return size;
}

/** Returns the size of a repeated string field. */
@SuppressWarnings("AvoidObjectArrays")
public static int sizeRepeatedString(ProtoFieldInfo field, byte[][] utf8Bytes) {
int size = 0;

Check warning on line 116 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java#L116

Added line #L116 was not covered by tests
for (byte[] i : utf8Bytes) {
size += MarshalerUtil.sizeBytes(field, i);

Check warning on line 118 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java#L118

Added line #L118 was not covered by tests
}
return size;

Check warning on line 120 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerUtil.java#L120

Added line #L120 was not covered by tests
}

/**
* Returns the size of a repeated uint64 field.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@
StatelessMarshalerUtil.writeUtf8(output, string, utf8Length, context);
}

@Override
public void writeRepeatedString(ProtoFieldInfo field, byte[][] utf8Bytes) throws IOException {
for (byte[] value : utf8Bytes) {
writeString(field, value);

Check warning on line 169 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/ProtoSerializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/ProtoSerializer.java#L169

Added line #L169 was not covered by tests
}
}

Check warning on line 171 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/ProtoSerializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/ProtoSerializer.java#L171

Added line #L171 was not covered by tests

@Override
public void writeBytes(ProtoFieldInfo field, byte[] value) throws IOException {
output.writeUInt32NoTag(field.getTag());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,18 @@
writeString(field, utf8Bytes);
}

/**
* Serializes a protobuf {@code repeated string} field. {@code utf8Bytes} is the UTF8 encoded
* bytes of the strings to serialize.
*/
@SuppressWarnings("AvoidObjectArrays")
public void serializeRepeatedString(ProtoFieldInfo field, byte[][] utf8Bytes) throws IOException {
if (utf8Bytes.length == 0) {
return;

Check warning on line 230 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java#L230

Added line #L230 was not covered by tests
}
writeRepeatedString(field, utf8Bytes);
}

Check warning on line 233 in exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java

View check run for this annotation

Codecov / codecov/patch

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java#L232-L233

Added lines #L232 - L233 were not covered by tests

/**
* Serializes a protobuf {@code string} field. {@code string} is the value to be serialized and
* {@code utf8Length} is the length of the string after it is encoded in UTF8. This method reads
Expand All @@ -246,6 +258,11 @@
ProtoFieldInfo field, String string, int utf8Length, MarshalerContext context)
throws IOException;

/** Writes a protobuf {@code repeated string} field, even if it matches the default value. */
@SuppressWarnings("AvoidObjectArrays")
public abstract void writeRepeatedString(ProtoFieldInfo field, byte[][] utf8Bytes)
throws IOException;

/** Serializes a protobuf {@code bytes} field. */
public void serializeBytes(ProtoFieldInfo field, byte[] value) throws IOException {
if (value.length == 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,7 @@ protected void writeTo(Serializer output) throws IOException {
output.serializeRepeatedMessage(Profile.ATTRIBUTE_TABLE, attributeMarshalers);
output.serializeRepeatedMessage(Profile.ATTRIBUTE_UNITS, attributeUnitMarshalers);
output.serializeRepeatedMessage(Profile.LINK_TABLE, linkMarshalers);
for (byte[] i : stringTable) {
output.serializeString(Profile.STRING_TABLE, i);
}
output.serializeRepeatedString(Profile.STRING_TABLE, stringTable);
output.serializeInt64(Profile.DROP_FRAMES, dropFrames);
output.serializeInt64(Profile.KEEP_FRAMES, keepFrames);
output.serializeInt64(Profile.TIME_NANOS, timeNanos);
Expand Down Expand Up @@ -192,9 +190,7 @@ private static int calculateSize(
size += MarshalerUtil.sizeRepeatedMessage(Profile.ATTRIBUTE_TABLE, attributeMarshalers);
size += MarshalerUtil.sizeRepeatedMessage(Profile.ATTRIBUTE_UNITS, attributeUnitMarshalers);
size += MarshalerUtil.sizeRepeatedMessage(Profile.LINK_TABLE, linkMarshalers);
for (byte[] i : stringTable) {
size += MarshalerUtil.sizeBytes(Profile.STRING_TABLE, i);
}
size += MarshalerUtil.sizeRepeatedString(Profile.STRING_TABLE, stringTable);
size += MarshalerUtil.sizeInt64(Profile.DROP_FRAMES, dropFrames);
size += MarshalerUtil.sizeInt64(Profile.KEEP_FRAMES, keepFrames);
size += MarshalerUtil.sizeInt64(Profile.TIME_NANOS, timeNanos);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ private static ProfileData sampleProfileData() {
Attributes.empty(),
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList(),
listOf("foo", "bar"),
3,
4,
5,
Expand Down Expand Up @@ -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.

.setDefaultSampleType(10);
}

Expand Down
Loading