Skip to content
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

Fixed default sampling function. #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomec-martin
Copy link

For sampling rate 0.5 function gets arguments (0,2) and should return only 0 or 1 (not 2).
Still, this is not correct for sampling rates like 0.4, (because of floor in appendToBuffer) but fix could break BC...

For sampling rate 0.5 function gets arguments (0,2) and should return only 0 or 1 (not 2).
@liuggio
Copy link
Owner

liuggio commented Nov 18, 2016

Hi thanks a lot but this will affect everybody this is a big BC what do you think?
Which is your suggestion?

@tomec-martin
Copy link
Author

I think that this patch is the best compromise - because it does not afect API for sampling function.
Less breaking workaround - you can mention in documentation that the default sampling function is wrong and users should replace it.
But the API for sampling function should be specified and corrected. Floor in appendToBuffer is really wrong and should be removed (maybe in 2.0?):
https://github.com/liuggio/statsd-php-client/blob/master/src/Liuggio/StatsdClient/Service/StatsdService.php#L175

@liuggio liuggio added the 2.0 label Nov 21, 2016
@liuggio
Copy link
Owner

liuggio commented Nov 21, 2016

Thanks labelled as 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants