-
Notifications
You must be signed in to change notification settings - Fork 147
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
Configurable sampler #260
Configurable sampler #260
Conversation
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.
Looks awesome @labbati! Just one little nitpick and then a question about what we should do about how the ID's are generated. :)
@@ -101,7 +101,7 @@ public function reset() | |||
{ | |||
$this->scopeManager = new ScopeManager(); | |||
$this->globalConfig = Configuration::get(); | |||
$this->sampler = new AlwaysKeepSampler(); | |||
$this->sampler = new ConfigurableSampler(); |
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.
❤️
Co-Authored-By: labbati <luca.abbati@datadoghq.com>
@SammyK up again for second round of CR |
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.
Awesome work @labbati! Just a question left in the comments, but other than that this looks good to go! :D
$first31bits = mt_rand(0, mt_getrandmax()) << 32; | ||
$second31bits = mt_rand(0, mt_getrandmax()) << 1; | ||
$random1or0 = mt_rand(0, 1); | ||
return (string) ($first31bits | $second31bits | $random1or0); |
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.
Nice workaround @labbati! For the future PR, we'll look into possibly supporting 32-bit architectures as well. :)
*/ | ||
public static function getMaxId() | ||
{ | ||
return (mt_getrandmax() << 32) | (mt_getrandmax() << 1) | 1; |
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 curious why this exists. Is it just to show that the return value is the same as PHP_INT_MAX
in the tests? I don't see it being used anywhere else in the tracer and that test might break on 32-bit boxes. Just double checking. :)
// algorithm. To compensate this we use here a `mt_rand` generate value to decide how to set the priority | ||
// sampling. When we will introduce client sampling we will have to implement the real and final knuth hashing | ||
// function. | ||
$shouldKeep = mt_rand(1, mt_getrandmax()) <= $rate * mt_getrandmax(); |
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.
❤️
Description
This PR will allow user to configure the sampling rate through an environmental variable
DD_SAMPLING_RATE
which can be set as a floating point number between0.0
(always reject) and1.0
(always keep).Readiness checklist
Special kudos to @SammyK in the PR header to have spotted the 32/64 bits issue 😄