-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[featureflag]: use float to check for flag probability #1237
[featureflag]: use float to check for flag probability #1237
Conversation
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
Works for me! Small tweaks that could be a follow up PR: Make the input a number field and add some help text for how to use it. It might also be nice to add a failure scenario flag that causes an error with 100% of requests so that this FF ratio is easy to verify — current scenarios are mostly sporadic. |
@puckpuck to make migrations easier, maybe we want to just drop the ffs_postgres database at startup? |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
I've tried to do this, and maybe I'm missing something, but if using a number type input with Phoenix, the value needs to be an integer. |
# Conflicts: # CHANGELOG.md
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
@puckpuck I added a PR to your repo that does a decimal number input with a number(2,1) column type |
@puckpuck will you add @joshleecreates' PR into this one, or should we get this one merged? |
Where are we at with this PR? |
…lags Add help text for float feature flags; Fix postgres upgrades
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
This PR is ready now. I merged in the changes from @joshleecreates and made some edits to the help text. I checked what we have for existing docs, and anything about this feature would be net new, so I'm not sure we need to add anything there. |
I think you will want to change the SQL column to |
@puckpuck I'm not able to run the FeatureFlagService, I'm getting this error:
|
@julianocosta89 this may be because I originally did the migration incorrectly and @joshleecreates cleaned it up. Could you remove the featureflag service and db, build from main then try again from this branch. |
@joshleecreates How?
|
I think that makes a lot of sense and will definitely simplify any future schema changes. |
@julianocosta89 I will test again this evening |
src/featureflagservice/lib/featureflagservice/feature_flags/feature_flag.ex
Show resolved
Hide resolved
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 have the same problem as @julianocosta89 even after first cleaning up my docker env with
docker system prune -a
I think the problem is that the file src/featureflagservice/priv/repo/migrations/20220524172636_create_featureflags.ex
still contains booleans. I got this working with the following changes in that file:
def change do
create table(:featureflags) do
add :name, :string
add :description, :string
add :enabled, :float, default: 0.0, null: 0.0
timestamps()
end
create unique_index(:featureflags, [:name])
execute(&execute_up/0, &execute_down/0)
end
defp execute_up do
repo().insert(%Featureflagservice.FeatureFlags.FeatureFlag{
name: "productCatalogFailure",
description: "Fail product catalog service on a specific product",
enabled: 0.0
})
repo().insert(%Featureflagservice.FeatureFlags.FeatureFlag{
name: "recommendationCache",
description: "Cache recommendations",
enabled: 0.0
})
repo().insert(%Featureflagservice.FeatureFlags.FeatureFlag{
name: "adServiceFailure",
description: "Fail ad service requests sporadically",
enabled: 0.0
})
repo().insert(%Featureflagservice.FeatureFlags.FeatureFlag{
name: "cartServiceFailure",
description: "Fail cart service requests sporadically",
enabled: 0.0
})
end
@mviitane I submitted a fix here: https://github.com/puckpuck/opentelemetry-demo/pull/2 |
return to single migration with decimal type
Please try this version that no longer tries to migrate a database but instead does a clean setup of the database. This may require you to do a |
We should get #1296 merged and included in this branch. Currently, this PR branch doesn't compile after |
Seems to still be some errors in the ff service --
|
Looks like the problem is coming from the recommendation server |
That looks like it's coming from the Erlang API, I can take a look tonight. |
Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
I just moved everything from decimal which is a particular struct type to a float which is a scalar type in erlang. Math works properly, and since we only care about 1/100th precision, anyhow this is fine. |
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.
tested and working locally
Also, propagate the change from bool to float introduced in open-telemetry#1237 more consistently via proto definitions by differentiating between the GetFlag operation (which evaluates the probabilty and therefore returns a bool) and all other operations, which need to operate with a float value/probability directly.
Also: * propagate the change from bool to float introduced in open-telemetry#1237 more consistently via proto definitions by differentiating between the GetFlag operation (which evaluates the probabilty and therefore returns a bool) and all other operations, which need to operate with a float value/probability directly. To that end, the Flag grpc message has been split into two new types, FlagEvaluationResult and FlagDefinition. * Rename the UpdateFlag operation to UpdateFlagProbability, as it actually only updates the enabled/probability value, but not the description or the name.
Also: * propagate the change from bool to float introduced in open-telemetry#1237 more consistently via proto definitions by differentiating between the GetFlag operation (which evaluates the probabilty and therefore returns a bool) and all other operations, which need to operate with a float value/probability directly. To that end, the Flag grpc message has been split into two new types, FlagEvaluationResult and FlagDefinition. * Rename the UpdateFlag operation to UpdateFlagProbability, as it actually only updates the enabled/probability value, but not the description or the name.
Also: * propagate the change from bool to float introduced in open-telemetry#1237 more consistently via proto definitions by differentiating between the GetFlag operation (which evaluates the probabilty and therefore returns a bool) and all other operations, which need to operate with a float value/probability directly. To that end, the Flag grpc message has been split into two new types, FlagEvaluationResult and FlagDefinition. * Rename the UpdateFlag operation to UpdateFlagProbability, as it actually only updates the enabled/probability value, but not the description or the name.
…y#1237) * use float to check for flag probability Signed-off-by: Pierre Tessier <pierre@pierretessier.com> * use float to check for flag probability Signed-off-by: Pierre Tessier <pierre@pierretessier.com> * Update CHANGELOG.md * Add help text for float feature flags; Make upgrades possible for postgres * add description for feature flag value Signed-off-by: Pierre Tessier <pierre@pierretessier.com> * clean up help text Signed-off-by: Pierre Tessier <pierre@pierretessier.com> * return to single migration with decimal type * use float instead of decimal Signed-off-by: Pierre Tessier <pierre@pierretessier.com> --------- Signed-off-by: Pierre Tessier <pierre@pierretessier.com> Co-authored-by: Austin Parker <austin@ap2.io> Co-authored-by: Josh Lee <josh@joshuamlee.com> Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
Changes
This adds the ability to do A/B testing with feature flags. Instead of storing the feature flag enabled as a boolean, it is now stored as a float. 0 = false. 1 = true. Everything between 0 and 1 will be used as a probability. If the flag has a value of 0.3, it will return true 30% of the time.
A new attribute called
app.featureflag.raw_value
is added to the feature flag spans containing the actual raw value for the flag as a float.Merge Requirements
For new features contributions please make sure you have completed the following
essential items:
CHANGELOG.md
updated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.