support write byte[] to bytes_value instead of array_value in TagWriter #6534
Conversation
|
|
df532c1 to
13c13ff
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6534 +/- ##
==========================================
+ Coverage 86.62% 86.64% +0.01%
==========================================
Files 258 258
Lines 11878 11888 +10
==========================================
+ Hits 10289 10300 +11
+ Misses 1589 1588 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@JunweiSUN, EasyCLA is mandatory, before we can consider your contribution. Could you please sign it? |
13c13ff to
09b7fe5
Compare
|
Thanks @Kielek , I'm new to EasyCLA and I just got approved of CLA by my employer. Could you tell me how to rerun the EasyCLA check? Also I will finish the UT and open a discussion issue later. Thanks a lot! |
|
/easycla |
|
@JunweiSUN, when you are ready, just post message |
|
/easycla |
1 similar comment
|
/easycla |
...nTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufOtlpTagWriter.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Tests/Internal/JsonStringArrayTagWriterTests.cs
Outdated
Show resolved
Hide resolved
|
/easycla |
|
The PR is ready to review. I will mark the PR as ready for review now. |
5832b62 to
6766101
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the serialization performance of byte arrays in the OpenTelemetry Protocol (OTLP) exporter by writing byte[] objects directly to the bytes_value field instead of treating them as generic arrays that get serialized to array_value.
- Introduces a new
TryWriteByteArrayTagmethod to theTagWriterbase class for specialized byte array handling - Implements optimized byte array serialization in
ProtobufOtlpTagWriterusing the protobufbytes_valuefield - Updates existing tag writers to provide default implementations that fall back to array handling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/TagWriter/TagWriter.cs | Adds byte array detection and new abstract method for specialized handling |
| src/Shared/TagWriter/JsonStringArrayTagWriter.cs | Provides default implementation that falls back to array handling |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufOtlpTagWriter.cs | Implements optimized byte array serialization using protobuf bytes_value |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufSerializer.cs | Adds helper method for writing byte arrays with protobuf tags |
| test/OpenTelemetry.Tests/Internal/JsonStringArrayTagWriterTests.cs | Adds required method implementation to test mock |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpAttributeTests.cs | Updates test to verify byte arrays use bytes_value field |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md | Documents the change |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...nTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufOtlpTagWriter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I would replace all APIs byte[] with ReadOnlySpan<byte>/Span<byte>. At most, have shallow byte[] overloads for backward compatibility marked with ObsoletAttribute.
I have this branch where I played around with that.
|
Hi @paulomorgado , I think all new APIs in this PR are already using |
This should use a [MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int WriteByteArrayWithTag(byte[] buffer, int writePosition, int fieldNumber, ReadOnlySpan<byte> value)
{
writePosition = WriteTag(buffer, writePosition, fieldNumber, ProtobufWireType.LEN);
writePosition = WriteLength(buffer, writePosition, value.Length);
value.CopyTo(buffer.AsSpan(writePosition));
writePosition += value.Length;
return writePosition;
} |
|
Hi @paulomorgado , |
Isn't this method being added now? Why add to the "problem" instead of adding to the "solution"? Have a look at my branch. |
|
Hi @paulomorgado , I don't think it's a "problematic" way to keep the behavior of the new API aligned with other existing APIs. If we want to use Span in |
|
@JunweiSUN I think the current approach of aligning with existing APIs is fine for now, so following up with a dedicated PR later makes sense. At the same time, I agree with @paulomorgado that we should eventually move toward a more consistent usage of Span to avoid carrying forward limitations. |
|
@paulomorgado I reviewed your branch, and it looks good. If you’re okay with it, would you like to create a follow-up PR with it? |
b816045
into
open-telemetry:main
Design discussion issue #6535
Changes
When writing big byte[] object to the attribute of LogRecord using ProtobufOtlpTagWriter, current implementation of TagWriter will treat byte[] as array_value in the proto schema instead of bytes_value, which brings two major concerns:
This PR allows ProtobufOtlpTagWriter to write byte[] as bytes_value directly, which will bring significant performance improvement when writing large byte[] objects.
A minimum benchmark could be found in #6535 .
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes