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

Support percentages with up to 3 decimals #274

Merged
merged 10 commits into from Jul 1, 2017

Conversation

Projects
None yet
3 participants
@jnunemaker
Owner

jnunemaker commented Jun 27, 2017

Would love some eyes on this to make sure I'm doing it right or if there are any downsides with the approach.

closes #273

jnunemaker added some commits Apr 14, 2017

Add Typecast.to_percentage
For handling backwards compatible int but supporting float going
forward.
@jqr

I think this approach is looking sound, minus a few questions I had.

I do wonder if we should store the full float value or round it to 3 decimal places since that's what it will be used against. Or perhaps this is just a display artifact and we should store the set value.


it 'enables feature for accurate number of actors' do
margin_of_error = 0.02 * number_of_actors
expected_open_count = 100_000 * decimal

This comment has been minimized.

@jqr

jqr Jun 27, 2017

Collaborator

Should this be number_of_actors * decimal? If not where is 100_000 coming from?

This comment has been minimized.

@jnunemaker

jnunemaker Jun 27, 2017

Owner

Ha, yeah I think so. Good catch!

This comment has been minimized.

@jnunemaker

jnunemaker Jun 27, 2017

Owner

Fixed in dd85fe9 for both specs

def context(integer, feature = feature_name, thing = nil)
Flipper::FeatureCheckContext.new(
feature_name: feature,
values: Flipper::GateValues.new(percentage_of_actors: integer),

This comment has been minimized.

@jqr

jqr Jun 27, 2017

Collaborator

Should this be percentage_of_time?

This comment has been minimized.

@jnunemaker

This comment has been minimized.

@jnunemaker
Fix up gate % specs
* better variable names for context method
* correct usage of expected open count
* rename % of time variable to invocations from actors
* clarify that % of time is based on invocations not actors by passing
same actor every time
@@ -30,7 +30,7 @@ def open?(context)
if Types::Actor.wrappable?(context.thing)
actor = Types::Actor.wrap(context.thing)
id = "#{context.feature_name}#{actor.value}"
Zlib.crc32(id) % 100 < percentage
Zlib.crc32(id) % 100_000 < percentage * 1_000

This comment has been minimized.

@jqr

jqr Jun 27, 2017

Collaborator

I think a comment or breaking out the scaling factor (1000) might be useful here. (This comment was previously on the wrong item)

This comment has been minimized.

@jnunemaker

jnunemaker Jun 27, 2017

Owner

Tweaked in bba3969. Do you think that helps? Enough? Any other suggestions?

This comment has been minimized.

@jqr

jqr Jun 27, 2017

Collaborator

Perfect!

Zlib.crc32(id) % 100 < percentage
# this is to support up to 3 decimal places in percentages
scaling_factor = 1_000
Zlib.crc32(id) % (100 * scaling_factor) < percentage * scaling_factor

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 29, 2017

Collaborator

👍

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 30, 2017

Collaborator

Out of pure curiosity did ever look at any other hash functions? (other than crc32)

jnunemaker added some commits Jun 29, 2017

@@ -19,7 +19,7 @@ def initialize(id)
alias_method :flipper_id, :id
end

total = 10_000
total = 100_000

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 30, 2017

Collaborator

awesome, will probably want to update percentage_of_time example too

This comment has been minimized.

@jnunemaker

jnunemaker Jul 1, 2017

Owner

fixed in ece70a3. Great idea.

@@ -411,10 +434,27 @@
end
end

context 'for not enabled percentage of time' do
context 'for enabled float percentage of time' do
before do
# ensure percentage of time returns percentage that makes five percent

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 30, 2017

Collaborator

super minor, but should this now say that makes 4.1 percent instead of five since its being enabled for 4.1% now

This comment has been minimized.

@jnunemaker

jnunemaker Jul 1, 2017

Owner

fixed in 2b97394. Thanks!

subject { described_class.new }

it 'enables feature for accurate number of actors' do
margin_of_error = 0.02 * number_of_actors

This comment has been minimized.

@AlexWheeler

AlexWheeler Jun 30, 2017

Collaborator

These tests are great. Also curious, any specific reason you chose 2% margin of error? For fun running it on my machine 100 times with 0.01 and getting 0 errors

This comment has been minimized.

@jnunemaker

jnunemaker Jun 30, 2017

Owner

Mostly just a "this felt like an ok error margin for the number of iterations". Since I've increased the number of iterations, I would probably be ok with increasing the error margin as it is likely lower.

jnunemaker added some commits Jul 1, 2017

@jnunemaker jnunemaker merged commit e47e1e8 into master Jul 1, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@jnunemaker jnunemaker deleted the float-ing-down-a-river branch Jul 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment