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

Allow using negative values in RangeSampler#increment and RangeSampler#decrement #532

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

Conversation

jcazevedo
Copy link
Contributor

This pull request modifies the increment and decrement methods of RangeSampler so that they correctly track the minimum and maximum values if they're supplied with negative values.

@ivantopo
Copy link
Contributor

Hey @jcazevedo! Thanks for the PR :)... Just wanted to ask, what was the use case where allowing negative numbers was necessary?

@jcazevedo
Copy link
Contributor Author

It's mostly an API convenience. I have a situation where I'm keeping track of deltas with a min max counter and I'm currently doing:

if (delta >= 0) metric.increment(delta)
else metric.decrement(-1 * delta)

If the increment and decrement methods would support negative values, I could simply do metric.increment(delta).

@ivantopo
Copy link
Contributor

Seems like a nice thing to have, thanks for sharing! Although, I think it would be better to add a new method to the RangeSampler API, lets say def delta(value: Long) (or another name that makes sense) that does the right thing for this case.

If we go that way we keep the increment/decrement with the same cost as before (only updating two places) and only incur in the additional expense when it is actually needed. What do you think about that? Also, what does @dpsoft think about this change?

@jcazevedo
Copy link
Contributor Author

I'd be fine with the delta method, but I think that allowing negative values in the increment and decrement methods can still be a source of confusion. At least it was for me, as I was expecting the min value to decrease if I'd supply increment with a negative value. Improving the documentation on those methods with a mention to this issue would also be nice.

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

Successfully merging this pull request may close these issues.

2 participants