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

Exponentially decaying reservoir update #1135

Merged

Conversation

@IstvanM
Copy link
Contributor

@IstvanM IstvanM commented May 31, 2017

Exponentially Decaying Reservoir was giving incorrect values in the snapshot if the inactive period was too long. This could result in wrong metrics aggregation.

white spaces
@arteam
Copy link
Member

@arteam arteam commented May 31, 2017

Thanks for the pull request! I will review today.

@arteam arteam self-requested a review May 31, 2017
@IstvanM
Copy link
Contributor Author

@IstvanM IstvanM commented Jun 1, 2017

Thank you! Let me know if you need anything to be changed .

@@ -38,6 +38,12 @@ public WeightedSample(long value, double weight) {
* @param values an unordered set of values in the reservoir
*/
public WeightedSnapshot(Collection<WeightedSample> values) {
if (values.size()==1 && Double.compare(values.iterator().next().weight,0D)==0) {

This comment has been minimized.

@arteam

arteam Jun 1, 2017
Member

I think it's not very obvious why we perform this check in the constructor of the snapshot. I understand that the idea to avoid the situation when we have only one element in the sample with a 0 weight (because it provides bogus data), but I believe it should be performed in ExponentiallyDecayingReservoir, where the logic of rescaling resides. What do you think, if we add a guard condition:

if (scalingFactor == 0) {
    values.clear();
}

in the ExponentiallyDecayingReservoir#rescale method? I think it could be a more clear solution.

This comment has been minimized.

@IstvanM

IstvanM Jun 6, 2017
Author Contributor

Hi thanks, yes I will look into that today. I agree here it isn't the best to solve this problem.

@IstvanM
Copy link
Contributor Author

@IstvanM IstvanM commented Jun 13, 2017

Should be done, can you check please?

@arteam arteam merged commit 6caa4ad into dropwizard:3.2-development Jun 13, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam
Copy link
Member

@arteam arteam commented Jun 13, 2017

Thank you very much for the contribution!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.