Skip to content

Conversation

paulomorgado
Copy link
Contributor

Addresses #6543

Changes

Refactored serialization logic to use Span<byte> for buffer manipulation, improving performance, memory efficiency, and code readability. Updated method signatures to eliminate explicit writePosition management, replaced incremental write logic with consistent += operations, and streamlined encoding methods. Enhanced test coverage to validate edge cases and updated all test cases to align with the new implementation. Applied changes consistently across all serializers, ensuring backward compatibility and laying the groundwork for future optimizations. Improved error handling, debugging, and documentation for better maintainability and robustness.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Sep 30, 2025
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.46%. Comparing base (3d0d972) to head (a67075d).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6544      +/-   ##
==========================================
- Coverage   86.70%   86.46%   -0.24%     
==========================================
  Files         258      258              
  Lines       11888    11648     -240     
==========================================
- Hits        10308    10072     -236     
+ Misses       1580     1576       -4     
Flag Coverage Δ
unittests-UnstableCoreLibraries-Experimental 85.87% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ementation/Serializer/ProtobufOtlpLogSerializer.cs 99.08% <ø> (+0.59%) ⬆️
...ntation/Serializer/ProtobufOtlpMetricSerializer.cs 98.23% <ø> (-0.55%) ⬇️
...ation/Serializer/ProtobufOtlpResourceSerializer.cs 90.00% <ø> (-1.67%) ⬇️
...Implementation/Serializer/ProtobufOtlpTagWriter.cs 93.33% <ø> (-2.62%) ⬇️
...entation/Serializer/ProtobufOtlpTraceSerializer.cs 95.16% <ø> (+0.14%) ⬆️
...ol/Implementation/Serializer/ProtobufSerializer.cs 82.35% <ø> (-9.16%) ⬇️

@paulomorgado paulomorgado marked this pull request as ready for review October 1, 2025 12:40
@paulomorgado paulomorgado requested a review from a team as a code owner October 1, 2025 12:40
@paulomorgado
Copy link
Contributor Author

Hi @martincostello, @Kielek, can you help me here with the test failures?

{
writePosition = WriteLogRecord(buffer, writePosition, sdkLimitOptions, experimentalOptions, logRecords[i]);
}

Copy link
Member

Choose a reason for hiding this comment

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

Put the blank line back, and the style analyser won't fail the build.

Copy link
Member

Choose a reason for hiding this comment

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

In general, it is good practice to run local build in Release mode. Debug allows to compile with Warnings, Release is extremely strict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! Sorry!

I have fixed the style errors, but I still have a test failing:

Failed OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests.OtlpAttributeTests.IntegralTypesSupported(value: [1, 2, 3]) [< 1 ms]
Error Message:
 Google.Protobuf.InvalidProtocolBufferException : Protocol message contained an invalid tag (zero).

I can't figure out how this is related to my changes.

Runing the tests in debug mode I get this:

 OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests.OtlpAttributeTests.IntegralTypesSupported(value: [1, 2, 3])
   Source: OtlpAttributeTests.cs line 56
   Duration: 7 ms

  Message: 
Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException : Method Debug.Fail failed with 'bytesWritten did not match numberOfUtf8CharsInString
', and was translated to Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException to avoid terminating the process hosting the test.

  Stack Trace: 
TestHostTraceListener.GetException(String message)
TestHostTraceListener.Fail(String message, String detailMessage)
TraceInternal.Fail(String message, String detailMessage)
Debug.Fail(String message, String detailMessage)
ProtobufOtlpTagWriter.TryWriteByteArrayTag(OtlpTagWriterState& state, String key, ReadOnlySpan`1 value) line 94
TagWriter`2.TryWriteTag(TTagState& state, String key, Object value, Nullable`1 tagValueMaxLength) line 70
TagWriter`2.TryWriteTag(TTagState& state, KeyValuePair`2 tag, Nullable`1 tagValueMaxLength) line 28
OtlpAttributeTests.TryTransformTag(KeyValuePair`2 tag, KeyValue& attribute) line 293
OtlpAttributeTests.IntegralTypesSupported(Object value) line 59

This happens because ProtobufOtlpTagWriter.TryWriteByteArrayTag is calling ProtobufSerializer.WriteStringWithTag with ProtobufOtlpCommonFieldNumberConstants.KeyValue_Key (1) for numberOfUtf8CharsInString with key as "key".

What am I missing here?

@paulomorgado paulomorgado force-pushed the memory branch 3 times, most recently from a67075d to 787412e Compare October 1, 2025 18:08
…ation, improving performance, memory efficiency, and code readability. Updated method signatures to eliminate explicit `writePosition` management, replaced incremental write logic with consistent `+=` operations, and streamlined encoding methods. Enhanced test coverage to validate edge cases and updated all test cases to align with the new implementation. Applied changes consistently across all serializers, ensuring backward compatibility and laying the groundwork for future optimizations. Improved error handling, debugging, and documentation for better maintainability and robustness.
Copy link
Contributor

github-actions bot commented Oct 9, 2025

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants