Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add an API to send multiple samples at once #235
feat: Add an API to send multiple samples at once #235
Changes from 2 commits
bd9d85f
55ebdc5
d14c446
82eea75
d4f3cf8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This counts characters, while the capacity passed into
writeTo
is in bytes, which can cause payload drops if the direct client is used with non-ascii data. Would it be possible to account for encoded length here?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.
Good catch! I have fixed that in 37f858d
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 have again pushed the commit to use the target encoding:
https://github.com/DataDog/java-dogstatsd-client/pull/235/files#diff-363801336ae6e5f0955834149b9e550375970e5c0dcde6b72ce36c69abed7bc6R67
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.
This code is allocating, if it is a blocker, I can intern this code from guava: https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Utf8.java#L49
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 could be missing something, but this looks like it will skip values for chunks after second one: let's say we start with offset=3 and write 4 values at positions 3, 4, 5 and 6 before hitting overflow at i=7. This sets offset to 12 (3+7), loosing samples 7-11.
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 are right, I have updated the test and the implementation to fix that in d4f3cf8 as
i
is tracking the absolute index in the array not the relative one compare to the offset.