-
Notifications
You must be signed in to change notification settings - Fork 772
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] Expand array buffer / add tests to existing base buffer #6013
[otlp] Expand array buffer / add tests to existing base buffer #6013
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6013 +/- ##
==========================================
+ Coverage 86.29% 86.45% +0.15%
==========================================
Files 257 257
Lines 11685 11699 +14
==========================================
+ Hits 10084 10114 +30
+ Misses 1601 1585 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
{ | ||
[ThreadStatic] | ||
private static byte[]? threadBuffer; | ||
internal static byte[]? ThreadBuffer; | ||
private const int MaxBufferSize = 2 * 1024 * 1024; |
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.
@CodeBlanch Should we change this to readonly? With the current implementation, even if customers want to use a reflection hack to increase the size, const makes it impossible.
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.
🤷
You could define some experimental options to control the two buffer sizes. Users will be able to bind them to IConfiguration/envvars if needed. Better mechanism than reflection if you want to make it configurable.
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.
We could consider post RC release.
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.
ArgumentException
could be thrown when using these callsMerge requirement checklist
CHANGELOG.md
files updated for non-trivial changes