-
Notifications
You must be signed in to change notification settings - Fork 784
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
[otlp] OTLP Exporter Custom serializer Part 1 - WritingPrimitives #5910
[otlp] OTLP Exporter Custom serializer Part 1 - WritingPrimitives #5910
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5910 +/- ##
==========================================
+ Coverage 83.38% 86.36% +2.97%
==========================================
Files 297 258 -39
Lines 12531 11345 -1186
==========================================
- Hits 10449 9798 -651
+ Misses 2082 1547 -535
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/WireType.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/WritingPrimitives.cs
Outdated
Show resolved
Hide resolved
if (buffer.Length == writePosition) | ||
{ | ||
if (!IncreaseBufferSize(ref buffer)) | ||
{ | ||
// TODO: throw an exception to indicate that the buffer is too large. | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to drop a comment everywhere for this, but it might make sense to remove all of this expansion logic. Just assume the buffer has room. If not, catch indexoutofrange exception upstream, expand the buffer, and retry the entire serialization operation. This is what we do in prometheus. Why? Once we get to stable buffer size it will be a lot of faster without needing to do these checks each time we write some tiny piece of data into the blob. Also we can move from ref byte[]
everywhere to just byte[]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes Part of #5730
Design discussion issue #
Changes
Please provide a brief description of the changes here.
Based on prototype - #5731
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes