From 91f301b13ae92838c5805640077f32d2047b4d9c Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Mon, 4 Dec 2023 15:01:45 +0000 Subject: [PATCH] FFM-9927 Update MM3 Hash / Fallback to identifier for BB (#60) --- CHANGELOG.md | 10 +- rebar.config | 8 +- src/cfclient.app.src | 2 +- src/cfclient_evaluator.erl | 18 +- src/cfclient_metrics_attributes.hrl | 2 +- src/murmur_example.erl | 9 + test/cfclient_evaluator_test_data.erl | 12 +- test/cfclient_evaluator_tests.erl | 272 ++++++++++++++++---------- 8 files changed, 210 insertions(+), 123 deletions(-) create mode 100644 src/murmur_example.erl diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bf3887..dd97ba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [3.0.0] - 2023-12-04 +## ** Breaking for Erlang applications (not affecting Elixir applications) ** + +- The percentage rollout hash algorithm was slightly different compared to other Feature Flags SDKs, which resulted +in a different bucket allocation for the same target. The overall distribution was the same, but this change ensures +that the same target will get the same allocation per SDK +- if a custom BucketBy field is set on the web app, but it can't be found in a target, then the SDK will fall back +to bucketing by target identifier for that target a warning will be logged + ## [2.0.1] - 2023-07-02 ### Fixes - Some SDK dependencies were not getting included in releases created by `mix ` @@ -150,4 +159,3 @@ Evaluation Rules not yet complete: - Pre-requisites - Percentage Rollout - \ No newline at end of file diff --git a/rebar.config b/rebar.config index 8589f6c..8c9f740 100644 --- a/rebar.config +++ b/rebar.config @@ -53,10 +53,4 @@ ] }. -{ - shell, - [ - % {config, "config/sys.config"}, - {apps, [cfclient]} - ] -}. +{shell, [{config, "config/sys.config"}, {apps, [cfclient]}]}. diff --git a/src/cfclient.app.src b/src/cfclient.app.src index 86d4e29..9e506e0 100644 --- a/src/cfclient.app.src +++ b/src/cfclient.app.src @@ -4,7 +4,7 @@ [ {description, "Harness Feature Flags Server SDK"}, {pkg_name, "harness_ff_erlang_server_sdk"}, - {vsn, "2.0.1"}, + {vsn, "3.0.0"}, {registered, []}, {mod, {cfclient_app, []}}, {applications, [kernel, stdlib, cfapi, base64url, murmur, jsx, mochiweb]}, diff --git a/src/cfclient_evaluator.erl b/src/cfclient_evaluator.erl index 5e62605..f597d38 100644 --- a/src/cfclient_evaluator.erl +++ b/src/cfclient_evaluator.erl @@ -387,9 +387,19 @@ search_rules_for_inclusion([Rule | Tail], Target, Config) -> Distribution when Distribution /= null -> #{bucketBy := BucketBy, variations := Variations} = Distribution, #{identifier := Id, name := Name} = Target, - Attributes = maps:get(attributes, Target, #{}), - TargetAttributeValue = get_attribute_value(Attributes, BucketBy, Id, Name), - apply_percentage_rollout(Variations, BucketBy, TargetAttributeValue, 0) + TargetAttributes = maps:get(attributes, Target, #{}), + TargetAttributeValue = get_attribute_value(TargetAttributes, BucketBy, Id, Name), + {FinalTargetAttributeValue, FinalBucketBy} = + case TargetAttributeValue of + <<>> -> + % If the BucketBy field isn't found in the target, then fall back to the ID and log a warning + ?LOG_WARNING("BucketBy attribute '~p' not found in target attributes, falling back to 'identifier' ~p", [BucketBy, Id]), + {Id, <<"identifier">>}; + + _ -> + {TargetAttributeValue, BucketBy} + end, + apply_percentage_rollout(Variations, FinalBucketBy, FinalTargetAttributeValue, 0) end; _ -> search_rules_for_inclusion(Tail, Target, Config) @@ -557,7 +567,7 @@ apply_percentage_rollout([], _, _, _) -> excluded. -spec should_rollout(binary(), binary(), integer()) -> boolean(). should_rollout(BucketBy, TargetValue, Percentage) -> - Concatenated = <>, + Concatenated = <>, % Using a pure Elixir library for murmur3 Hash = 'Elixir.Murmur':hash_x86_32(Concatenated), BucketID = (Hash rem 100) + 1, diff --git a/src/cfclient_metrics_attributes.hrl b/src/cfclient_metrics_attributes.hrl index 38f3f8a..80f34a6 100644 --- a/src/cfclient_metrics_attributes.hrl +++ b/src/cfclient_metrics_attributes.hrl @@ -18,6 +18,6 @@ %% Constants for Metrics Data Attribute Values -define(SDK_TYPE_ATTRIBUTE_VALUE, <<"server">>). --define(SDK_VERSION_ATTRIBUTE_VALUE, <<"1.1.0">>). +-define(SDK_VERSION_ATTRIBUTE_VALUE, <<"3.0.0">>). -define(SDK_LANGUAGE_ATTRIBUTE_VALUE, <<"erlang">>). -define(TARGET_GLOBAL_IDENTIFIER, <<"__global__cf_target">>). diff --git a/src/murmur_example.erl b/src/murmur_example.erl new file mode 100644 index 0000000..143fdc8 --- /dev/null +++ b/src/murmur_example.erl @@ -0,0 +1,9 @@ +-module(murmur_example). + +-export([run_hash/0]). + +run_hash() -> + Concatenated = "identifier:test", + % Using a pure Elixir library for murmur3 + Hash = 'Elixir.Murmur':hash_x86_32(Concatenated), + BucketID = (Hash rem 100) + 1. diff --git a/test/cfclient_evaluator_test_data.erl b/test/cfclient_evaluator_test_data.erl index b05a361..105d9f6 100644 --- a/test/cfclient_evaluator_test_data.erl +++ b/test/cfclient_evaluator_test_data.erl @@ -29,8 +29,8 @@ prerequisite_matches_flag_2/0, prerequisite_matches_flag_3/0, generate_targets/2, - percentage_rollout_boolean/2, - percentage_rollout_multi_variate/3 + percentage_rollout_boolean/3, + percentage_rollout_multi_variate/4 ] ). @@ -905,7 +905,7 @@ target_group_for_percentage_rollout() -> }. -percentage_rollout_boolean(Weight1, Weight2) -> +percentage_rollout_boolean(Weight1, Weight2, BucketBy) -> #{ defaultServe => #{variation => <<"true">>}, environment => <<"dev">>, @@ -937,7 +937,7 @@ percentage_rollout_boolean(Weight1, Weight2) -> distribution => #{ - bucketBy => <<"identifier">>, + bucketBy => BucketBy, variations => [ @@ -959,7 +959,7 @@ percentage_rollout_boolean(Weight1, Weight2) -> version => 4 }. -percentage_rollout_multi_variate(Weight1, Weight2, Weight3) -> +percentage_rollout_multi_variate(Weight1, Weight2, Weight3, BucketBy) -> #{ defaultServe => #{variation => <<"variation3">>}, environment => <<"dev">>, @@ -991,7 +991,7 @@ percentage_rollout_multi_variate(Weight1, Weight2, Weight3) -> distribution => #{ - bucketBy => <<"identifier">>, + bucketBy => BucketBy, variations => [ diff --git a/test/cfclient_evaluator_tests.erl b/test/cfclient_evaluator_tests.erl index 52bb6dd..a0f9b2c 100644 --- a/test/cfclient_evaluator_tests.erl +++ b/test/cfclient_evaluator_tests.erl @@ -1755,13 +1755,12 @@ percentage_rollout_boolean_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_boolean_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_boolean(50, 50) + cfclient_evaluator_test_data:percentage_rollout_boolean(50, 50, <<"identifier">>) end ) end, fun - (_) -> - [{timeout, 100, ?_assertEqual({99992, 100008}, do_bool_variation_200k_times({0, 0}, 0))}] + (_) -> [{timeout, 100, ?_assertEqual({25016, 24984}, do_bool_variation_50k_times(<<"My_boolean_flag">>, none))}] end }, { @@ -1777,17 +1776,54 @@ percentage_rollout_boolean_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_boolean_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_boolean(100, 0) + cfclient_evaluator_test_data:percentage_rollout_boolean(100, 0, <<"identifier">>) end ) end, + fun (_) -> [{timeout, 100, ?_assertEqual({50000, 0}, do_bool_variation_50k_times(<<"My_boolean_flag">>, none))}] end + }, + { + "0/100", + setup, fun - (_) -> - [{timeout, 100, ?_assertEqual({200000, 0}, do_bool_variation_200k_times({0, 0}, 0))}] + () -> + meck:expect( + cfclient_ets, + get, + fun + (_, <<"segments/target_group_1">>) -> + cfclient_evaluator_test_data:target_group_for_percentage_rollout(); + + (_, <<"flags/My_boolean_flag">>) -> + cfclient_evaluator_test_data:percentage_rollout_boolean(0, 100, <<"identifier">>) + end + ) + end, + fun (_) -> [{timeout, 100, ?_assertEqual({0, 50000}, do_bool_variation_50k_times(<<"My_boolean_flag">>, none))}] end + }, + { + "70/30", + setup, + fun + () -> + meck:expect( + cfclient_ets, + get, + fun + (_, <<"segments/target_group_1">>) -> + cfclient_evaluator_test_data:target_group_for_percentage_rollout(); + + (_, <<"flags/My_boolean_flag">>) -> + cfclient_evaluator_test_data:percentage_rollout_boolean(70, 30, <<"identifier">>) + end + ) + end, + fun + (_) -> [{timeout, 100, ?_assertEqual({35074, 14926}, do_bool_variation_50k_times(<<"My_boolean_flag">>, none))}] end }, { - "0/100", + "70/30 with custom bucket by field", setup, fun () -> @@ -1799,17 +1835,16 @@ percentage_rollout_boolean_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_boolean_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_boolean(0, 100) + cfclient_evaluator_test_data:percentage_rollout_boolean(70, 30, <<"custom_bucket_by_field">>) end ) end, fun - (_) -> - [{timeout, 100, ?_assertEqual({0, 200000}, do_bool_variation_200k_times({0, 0}, 0))}] + (_) -> [{timeout, 100, ?_assertEqual({34971, 15029}, do_bool_variation_50k_times(<<"My_boolean_flag">>, custom_bucket_by_field))}] end }, { - "70/30", + "70/30 with custom bucket by field falls back to identifier", setup, fun () -> @@ -1821,13 +1856,13 @@ percentage_rollout_boolean_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_boolean_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_boolean(70, 30) + cfclient_evaluator_test_data:percentage_rollout_boolean(70, 30, <<"custom_bucket_by_field">>) end ) end, fun - (_) -> - [{timeout, 100, ?_assertEqual({140098, 59902}, do_bool_variation_200k_times({0, 0}, 0))}] + % Same assertion as regular 70/30 due to using identifier + (_) -> [{timeout, 100, ?_assertEqual({35074, 14926}, do_bool_variation_50k_times(<<"My_boolean_flag">>, a_field_that_cant_be_found))}] end } ] @@ -1850,19 +1885,13 @@ percentage_rollout_multivariate_string_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_string_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_multi_variate(34, 33, 33) + cfclient_evaluator_test_data:percentage_rollout_multi_variate(34, 33, 33, <<"identifier">>) end ) end, fun (_) -> - [ - { - timeout, - 100, - ?_assertEqual({68024, 66092, 65884}, do_string_variation_200k_times({0, 0, 0}, 0)) - } - ] + [{timeout, 100, ?_assertEqual({17014, 16622, 16364}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] end }, { @@ -1878,19 +1907,12 @@ percentage_rollout_multivariate_string_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_string_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_multi_variate(100, 0, 0) + cfclient_evaluator_test_data:percentage_rollout_multi_variate(100, 0, 0, <<"identifier">>) end ) end, fun - (_) -> - [ - { - timeout, - 100, - ?_assertEqual({200000, 0, 0}, do_string_variation_200k_times({0, 0, 0}, 0)) - } - ] + (_) -> [{timeout, 100, ?_assertEqual({50000, 0, 0}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] end }, { @@ -1906,19 +1928,12 @@ percentage_rollout_multivariate_string_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_string_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_multi_variate(0, 0, 100) + cfclient_evaluator_test_data:percentage_rollout_multi_variate(0, 0, 100, <<"identifier">>) end ) end, fun - (_) -> - [ - { - timeout, - 100, - ?_assertEqual({0, 0, 200000}, do_string_variation_200k_times({0, 0, 0}, 0)) - } - ] + (_) -> [{timeout, 100, ?_assertEqual({0, 0, 50000}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] end }, { @@ -1934,19 +1949,34 @@ percentage_rollout_multivariate_string_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_string_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_multi_variate(0, 50, 50) + cfclient_evaluator_test_data:percentage_rollout_multi_variate(0, 50, 50, <<"identifier">>) + end + ) + end, + fun + (_) -> [{timeout, 100, ?_assertEqual({0, 25016, 24984}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] + end + }, + { + "80/10/10", + setup, + fun + () -> + meck:expect( + cfclient_ets, + get, + fun + (_, <<"segments/target_group_1">>) -> + cfclient_evaluator_test_data:target_group_for_percentage_rollout(); + + (_, <<"flags/My_string_flag">>) -> + cfclient_evaluator_test_data:percentage_rollout_multi_variate(80, 10, 10, <<"identifier">>) end ) end, fun (_) -> - [ - { - timeout, - 100, - ?_assertEqual({0, 99992, 100008}, do_string_variation_200k_times({0, 0, 0}, 0)) - } - ] + [{timeout, 100, ?_assertEqual({40016, 5006, 4978}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] end }, { @@ -1962,75 +1992,111 @@ percentage_rollout_multivariate_string_flag() -> cfclient_evaluator_test_data:target_group_for_percentage_rollout(); (_, <<"flags/My_string_flag">>) -> - cfclient_evaluator_test_data:percentage_rollout_multi_variate(80, 10, 10) + cfclient_evaluator_test_data:percentage_rollout_multi_variate(80, 10, 10, <<"identifier">>) end ) end, fun (_) -> - [ - { - timeout, - 100, - ?_assertEqual({160095, 19903, 20002}, do_string_variation_200k_times({0, 0, 0}, 0)) - } - ] + [{timeout, 100, ?_assertEqual({40016, 5006, 4978}, do_string_variation_50k_times(<<"My_string_flag">>, none))}] + end + }, + { + "80/10/10 with custom bucket by field", + setup, + fun + () -> + meck:expect( + cfclient_ets, + get, + fun + (_, <<"segments/target_group_1">>) -> + cfclient_evaluator_test_data:target_group_for_percentage_rollout(); + + (_, <<"flags/My_string_flag">>) -> + cfclient_evaluator_test_data:percentage_rollout_multi_variate(80, 10, 10, <<"custom_bucket_by_field">>) + end + ) + end, + fun + (_) -> [{timeout, 100, ?_assertEqual({40026, 4959, 5015}, do_string_variation_50k_times(<<"My_string_flag">>, custom_bucket_by_field))}] + end + }, + { + "80/10/10 with custom bucket by field falls back to identifier", + setup, + fun + () -> + meck:expect( + cfclient_ets, + get, + fun + (_, <<"segments/target_group_1">>) -> + cfclient_evaluator_test_data:target_group_for_percentage_rollout(); + + (_, <<"flags/My_string_flag">>) -> + cfclient_evaluator_test_data:percentage_rollout_multi_variate(80, 10, 10, <<"custom_bucket_by_field">>) + end + ) + end, + fun + % Same bucketIDs as regular 70/10/10 test due to targetID fallback + (_) -> [{timeout, 100, ?_assertEqual({40016, 5006, 4978}, do_string_variation_50k_times(<<"My_string_flag">>, custom_field_that_is_not_found))}] end } ] }. -do_bool_variation_200k_times({TrueCounter, FalseCounter}, 200000) -> {TrueCounter, FalseCounter}; - -do_bool_variation_200k_times({TrueCounter, FalseCounter}, AccuIn) -> - Counter = AccuIn + 1, - TargetIdentifierNumber = integer_to_binary(Counter), - DynamicTarget = - #{ - identifier => <<"target", TargetIdentifierNumber/binary>>, - name => <<"targetname", TargetIdentifierNumber/binary>>, - anonymous => <<"">> - }, - case cfclient_evaluator:bool_variation(<<"My_boolean_flag">>, DynamicTarget, config()) of - {ok, _VariationIdentifier, true} -> - do_bool_variation_200k_times({TrueCounter + 1, FalseCounter}, Counter); +do_bool_variation_50k_times(FlagName, TargetAttribute) -> + InitialCounters = {0, 0}, + lists:foldl( + fun + (Counter, {TrueCounter, FalseCounter}) -> + TargetIdentifierNumber = integer_to_binary(Counter), + DynamicTarget = + #{ + identifier => <<"target", TargetIdentifierNumber/binary>>, + name => <<"targetname", TargetIdentifierNumber/binary>>, + anonymous => <<"">>, + attributes => #{TargetAttribute => integer_to_binary(Counter)} + }, + case cfclient_evaluator:bool_variation(FlagName, DynamicTarget, config()) of + {ok, _VariationIdentifier, true} -> {TrueCounter + 1, FalseCounter}; + {ok, _VariationIdentifier, false} -> {TrueCounter, FalseCounter + 1} + end + end, + InitialCounters, + lists:seq(1, 50000) + ). - {ok, _VariationIdentifier, false} -> - do_bool_variation_200k_times({TrueCounter, FalseCounter + 1}, Counter) - end. +do_string_variation_50k_times(FlagName, TargetAttribute) -> + InitialCounters = {0, 0, 0}, + lists:foldl( + fun + (Counter, {Variation1Counter, Variation2Counter, Variation3Counter}) -> + TargetIdentifierNumber = integer_to_binary(Counter), + DynamicTarget = + #{ + identifier => <<"target", TargetIdentifierNumber/binary>>, + name => <<"targetname", TargetIdentifierNumber/binary>>, + anonymous => <<"">>, + attributes => #{TargetAttribute => integer_to_binary(Counter)} + }, + case cfclient_evaluator:string_variation(FlagName, DynamicTarget, config()) of + {ok, _VariationIdentifier, <<"variation1">>} -> + {Variation1Counter + 1, Variation2Counter, Variation3Counter}; -do_string_variation_200k_times({Variation1Counter, Variation2Counter, Variation3Counter}, 200000) -> - {Variation1Counter, Variation2Counter, Variation3Counter}; + {ok, _VariationIdentifier, <<"variation2">>} -> + {Variation1Counter, Variation2Counter + 1, Variation3Counter}; -do_string_variation_200k_times({Variation1Counter, Variation2Counter, Variation3Counter}, AccuIn) -> - Counter = AccuIn + 1, - TargetIdentifierNumber = integer_to_binary(Counter), - DynamicTarget = - #{ - identifier => <<"target", TargetIdentifierNumber/binary>>, - name => <<"targetname", TargetIdentifierNumber/binary>>, - anonymous => <<"">> - }, - case cfclient_evaluator:string_variation(<<"My_string_flag">>, DynamicTarget, config()) of - {ok, _VariationIdentifier, <<"variation1">>} -> - do_string_variation_200k_times( - {Variation1Counter + 1, Variation2Counter, Variation3Counter}, - Counter - ); - - {ok, _VariationIdentifier, <<"variation2">>} -> - do_string_variation_200k_times( - {Variation1Counter, Variation2Counter + 1, Variation3Counter}, - Counter - ); - - {ok, _VariationIdentifier, <<"variation3">>} -> - do_string_variation_200k_times( - {Variation1Counter, Variation2Counter, Variation3Counter + 1}, - Counter - ) - end. + {ok, _VariationIdentifier, <<"variation3">>} -> + {Variation1Counter, Variation2Counter, Variation3Counter + 1} + end + end, + InitialCounters, + lists:seq(1, 50000) + ). prerequisite_matches_flag1() -> cfclient_evaluator_test_data:prerequisite_matches_flag_1().