Skip to content

Conversation

@xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jun 3, 2025

Fixes #
Design discussion issue #

Changes

Please provide a brief description of the changes here.

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)

@xiang17 xiang17 requested a review from a team as a code owner June 3, 2025 02:14
@xiang17 xiang17 changed the title Allow custom string size limit to increase ETW buffer space efficiency [Exporter.Geneva] Allow custom string size limit to increase ETW buffer space efficiency Jun 3, 2025
@github-actions github-actions bot added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Jun 3, 2025
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.89%. Comparing base (71655ce) to head (2b0db5c).
Report is 872 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 72.72% 3 Missing ⚠️
...rter.Geneva/Internal/MsgPack/MsgPackLogExporter.cs 90.90% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2813       +/-   ##
===========================================
- Coverage   73.91%   52.89%   -21.02%     
===========================================
  Files         267       55      -212     
  Lines        9615     5091     -4524     
===========================================
- Hits         7107     2693     -4414     
+ Misses       2508     2398      -110     
Flag Coverage Δ
unittests-Exporter.Geneva 52.89% <86.20%> (?)

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

Files with missing lines Coverage Δ
...r.Geneva/Internal/MsgPack/MessagePackSerializer.cs 91.80% <100.00%> (ø)
...rter.Geneva/Internal/MsgPack/MsgPackLogExporter.cs 92.20% <90.90%> (ø)
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.85% <72.72%> (-0.09%) ⬇️

... and 250 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@rajkumar-rangaraj rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

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

LGTM

@rajkumar-rangaraj rajkumar-rangaraj requested a review from Copilot June 4, 2025 16:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new connection‐string parameter to let users override the maximum MessagePack string length, propagates that limit through the exporter and serializer, adds tests for the new behavior, and updates documentation.

  • Adds PrivatePreviewLogMessagePackStringSizeLimit setting to ConnectionStringBuilder with default and validation
  • Exposes the new limit to MsgPackLogExporter and enforces it against the buffer size
  • Updates MessagePackSerializer to honor a configurable size limit and adds related unit tests

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/OpenTelemetry.Exporter.Geneva/Internal/ConnectionStringBuilder.cs Introduced PrivatePreviewLogMessagePackStringSizeLimit property with parsing and validation
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackLogExporter.cs Wired new size limit into the exporter and added a range check
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MessagePackSerializer.cs Extended serialization APIs to accept a custom char‐count limit
test/OpenTelemetry.Exporter.Geneva.Tests/MsgPackLogExporterTests.cs Added tests for default, valid, negative, and too‐large limits
test/OpenTelemetry.Exporter.Geneva.Tests/MessagePackSerializerTests.cs Updated ASCII and Unicode serialization tests to exercise truncation
test/OpenTelemetry.Exporter.Geneva.Tests/ConnectionStringBuilderTests.cs Added tests to verify parsing and validation of the new parameter
src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md Documented the new experimental connection‐string option
Comments suppressed due to low confidence (2)

src/OpenTelemetry.Exporter.Geneva/Internal/ConnectionStringBuilder.cs:85

  • Add XML documentation above this property to describe the PrivatePreviewLogMessagePackStringSizeLimit parameter, its default value, valid range, and behavior when omitted or out of range.
public int PrivatePreviewLogMessagePackStringSizeLimit

test/OpenTelemetry.Exporter.Geneva.Tests/MessagePackSerializerTests.cs:297

  • Consider adding tests that pass a non-default sizeLimit argument to MessagePackSerializer_TestASCIIStringSerialization and TestUnicodeStringSerialization to verify custom limits behave as expected.
this.MessagePackSerializer_TestASCIIStringSerialization(new string('Z', 1 << 15));

@rajkumar-rangaraj rajkumar-rangaraj merged commit f6bc5ab into open-telemetry:main Jun 4, 2025
65 checks passed
@xiang17 xiang17 deleted the xiang17/GenevaExporter-RelaxMessagePackStringLimit branch June 19, 2025 23:23
thompson-tomo pushed a commit to thompson-tomo/opentelemetry-dotnet-contrib that referenced this pull request Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants