-
Notifications
You must be signed in to change notification settings - Fork 397
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
feat(feat-flags): new simple feature toggles rule engine #494
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #494 +/- ##
===========================================
- Coverage 99.90% 99.23% -0.67%
===========================================
Files 105 113 +8
Lines 4233 4454 +221
Branches 206 239 +33
===========================================
+ Hits 4229 4420 +191
- Misses 1 22 +21
- Partials 3 12 +9
Continue to review full report at Codecov.
|
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 really like this idea!!! Thanks for this effort. I'm adding some comments as someone that had a first look at the code and doesn't know all the context. Hope this helps.
Some considerations to make it more similar to the rest of the project:
- ConfigurationException should be in its own exceptions.py file, have a comment and avoid pass
- Functions shouldn't expect a logger as a parameter. The module should have a logger initialised like in sqs.py
) -> bool: | ||
for rule in rules: | ||
rule_name = rule.get(RULE_NAME_KEY, "") | ||
rule_default_value = rule.get(RULE_DEFAULT_VALUE) |
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.
Shouldn't this and similar get operations all handle missing/default values?
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.
yeah, so basically i added a schema validation function so i wouldnt need to do all these checks here. In my original solution i used Pydantic to parse the input. But Heitor told me Pydantic is not an option here since it's a very large dependancy. How do you suggest I validate the schema? maybe similar to the validator utility?
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've commented below but I like the idea of keeping a simple function at the moment with if name in object
conditions which can be extended in the future with the validator if needed without breaking any external interfaces
aws_lambda_powertools/utilities/feature_toggles/configuration_store.py
Outdated
Show resolved
Hide resolved
hey @pcolazurdo , i've pushed lots of changes & answered your questions. waiting for your input regarding the schema validation. |
Sorry for the late response, I got my second jab and was feeling a bit under the weather. Great job, and happy to see that some of the comments were useful. Added some comments & closed some conversations as given your feedback I don't think we need to focus much on those. |
@risenberg-cyberark would be nice to allow for os environment variables for feature toggles. But there would need to be some kind of clear preference. eg:
|
@michaelbrewer dynamodb in my opinion is not a good choice for feature toggles. it costs more and doesnt have the deployment strategies that appconfig offers. This solution is only for appconfig. what do you mean by OS environment variable (fail over to next)? OS env feature toggles are static feature toggles, not dynamic like in appconfig. The best practice here it to split the deployment of appconfig from the deployment of the lambda/container which uses the toggles. That way, when you change the value of the toggle, you dont redeploy the entire lambda. OS vars toggle will always force you to redeploy the lambda, hence static, and not that great.
|
@pcolazurdo Also added schema validation code. let me know how it looks. |
OS environment variables are no additional cost to the user and are as fast as doing it directly in code, but conveniently grouped together. And the db layer for feature toggles should be flexible, sure we can have recommendations on using appconfig, but it does not have to be the only solution. The parameters utility already covers alot of this functionality, just not formally as feature toggles. Otherwise feature toggles are often paired with a canary backed deployment, where a subsection of users maybe have the toggle enabled. |
Regarding the os env - i think it would really complicate the logic here. What value will the a toggle get if it exists both in the os env and appconfig? Regarding the db layer - while i get your point for offering flexibility, we should strive to guide users to use AWS defined best practices. Forcing the schema here (rules, restrcitions etc.) on a dynamodb table would be awkward and not very trivial. it's not a key:boolean value schema anymore. However, even without it, i dont think dynamodb is the correct tool the job here. Appconfig has canary support, configuration validation, it can automatically revert in case of errors in runtime (due to the flag change), t's cheaper and it's literally built for storing configuration in multiple envs/applications etc. or from: |
This is why i suggest clearly documenting the layering. At Gyft we have been doing feature toggles from the beginning (before dynamodb and appconfig). Example logic would be:
if configuration.bool_value("credit.card.enabled", True):
print("Credit card payment is enabled")
print(configuration.str_value("theme", "darkMode")) |
Nothing is cheaper than loading from a configuration file :) |
Oh, and when i refer to feature flags, it is also about things that could be stored on a users profile to. |
Also if this library only going to support AWS only services ? What about all of the great services purpose built for this kind of thing? Launch Darkly for example? |
that's static though, not dynamic. |
that's a great implementation but it's simpler than the one i created it. The solution i did has a simple rule engine that changes valeus according to session context. That what makes dynamodb/rds even harder to use. |
i actually have a blog draft where i talk about the 3rd party options, if you want i'd send you a link in private. LaunchDarkley has far more capabilities than this solution but it really depends on your requirements. I think that for most developers, it might be enough for starters. |
@risenberg-cyberark have a look at the LaunchDarkly SDK. My concern is when a flag is on, there is no way to know why. So having an option to get the evaluation reasons can help alot with the debugging. |
I have. I started this whole API adventure after going through their SDK and the requirements from our dev architects. The api that i wrote has the the same signature as the api of launchdarkly. I've added logs that tell you what matched, why and how. Would that be enough? |
@michaelbrewer I guess this solution is like a cheaper/simpler/aws only alternative to 3rd party tools. |
hopefully :). But it is nice to be able to pull it down. So the use cases i see are:
if features.enabled("welcome.message", False):
print("Show welcome message")
if features.enabled("welcome.message", context, False):
print("Show welcome message")
flags: List[str] = features.all_enabled(context)
details = features.enabled_details("welcome.message", context, False)
details.value
details.reason
# etc.. |
So any feature that doesnt have a set of rules can be considered as "global". You can pass a context or an empty context, it will still match. Interesting idea regarding the list all enabled, it should be very easy to implement too. |
If Enum is set, how do we pass the config? Is the idea to have a condition in get_configuration()? |
That's a good point. Perhaps we need a JSON getter abstract class and have an appconfig implementation by inheritance. It will have a get_json_schema function that each implementation will override. This class will be passed as a param to the conf store init. It will make it more generic but that will force you to do some extra work prior to the init. |
@pcolazurdo @michaelbrewer @heitorlessa what do you think? how do we continue? |
@heitorlessa @pcolazurdo @michaelbrewer
where schema_fetcher is of abstract type SchemaFetcher which includes one function: get_json_configuration |
In regards to AppConfig, instead of directly integrating with it, wouldn’t it be more prescriptive to recommend the Lambda Extension? |
The implementation we did here allows you to switch from AppConfig to other config storing solutions. |
The built in polling of the lambda extension seems useful: And some people might already have this setup and want to use Powertools to ease integration |
Yeah I'm familiar with it. You still need to actively call the extension (Via HTTP i think) so the user experience is pretty much the same (behind the scenes it's a bit different but it's not a huge benifit in this use case). In any case, since I added a new fetcher API class, one could always add a fetcher that works with the lambda extension via HHTP. |
simple wins :). I always assumed that AppConfig was a little more magical in how it would push updates to lambdas, but it turns out to be a lot simpler. |
I like the new model, and I think it's clear and extensible enough. I agree the AppConfig extension may be overkilling it for most use cases as it add a dependency on the extension and increase the deployment size. The name is not my favorite :) but I ran out of creativity. @heitorlessa do you recommend having a complete documentation and fully functional sample before merging or what would you suggest? |
Let's definitely merge and have another PR with docs. This also gives us an opportunity to fix anything minor we might have missed, as we document it. @risenberg-cyberark if you don't mind, could you share a few snippets of the different features we should be highlighting? We can look at the tests and derive from there if you can't. @pcolazurdo feel free to merge ;) |
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.
Note to the future before releasing it:
- Missing docstrings with examples on how to use it in public Classes and public methods
- Review whether we have sufficient
logger.debug
coverage for future diagnostic - Docs: Extract key features for getting started vs advanced
Main features:
I'd point out that we validate the schema upon parsing. |
@@ -0,0 +1,2 @@ | |||
class ConfigurationException(Exception): | |||
"""When a a configuration store raises an exception on config retrieval or parsing""" |
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.
"""When a a configuration store raises an exception on config retrieval or parsing""" | |
"""When a configuration store raises an exception on config retrieval or parsing""" |
): | ||
"""This class fetches JSON schemas from AWS AppConfig | ||
|
||
Args: |
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.
You might want to use the mypy doc strings
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.
some minor notes
* develop: chore(deps): bump boto3 from 1.18.0 to 1.18.1 (aws-powertools#528) fix(tracer): mypy generic to preserve decorated method signature (aws-powertools#529) fix(parser): Make ApiGateway version, authorizer fields optional (aws-powertools#532) fix(mypy): fixes to resolve no implicit optional errors (aws-powertools#521) chore(deps): bump boto3 from 1.17.110 to 1.18.0 (aws-powertools#527) feat(feat-toggle): New simple feature toggles rule engine (WIP) (aws-powertools#494) chore(deps-dev): bump mkdocs-material from 7.1.9 to 7.1.10 (aws-powertools#522) chore(deps): bump boto3 from 1.17.102 to 1.17.110 (aws-powertools#523) chore(deps-dev): bump isort from 5.9.1 to 5.9.2 (aws-powertools#514) feat(mypy): add mypy support to makefile (aws-powertools#508) feat(api-gateway): add debug mode (aws-powertools#507)
Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 2.12.1 to 3.0.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's changelog</a>.</em></p> <blockquote> <h2>3.0.0 (2021-10-04)</h2> <p><strong>Note that this release drops support for Python 2.7 and Python 3.5.</strong></p> <ul> <li>Added support for Python 3.10 and updated various test dependencies. Contributed by Hugo van Kemenade in <code>[#500](pytest-dev/pytest-cov#500) <https://github.com/pytest-dev/pytest-cov/pull/500></code>_.</li> <li>Switched from Travis CI to GitHub Actions. Contributed by Hugo van Kemenade in <code>[#494](pytest-dev/pytest-cov#494) <https://github.com/pytest-dev/pytest-cov/pull/494></code>_ and <code>[#495](pytest-dev/pytest-cov#495) <https://github.com/pytest-dev/pytest-cov/pull/495></code>_.</li> <li>Add a <code>--cov-reset</code> CLI option. Contributed by Danilo Šegan in <code>[#459](pytest-dev/pytest-cov#459) <https://github.com/pytest-dev/pytest-cov/pull/459></code>_.</li> <li>Improved validation of <code>--cov-fail-under</code> CLI option. Contributed by ... Ronny Pfannschmidt's desire for skark in <code>[#480](pytest-dev/pytest-cov#480) <https://github.com/pytest-dev/pytest-cov/pull/480></code>_.</li> <li>Dropped Python 2.7 support. Contributed by Thomas Grainger in <code>[#488](pytest-dev/pytest-cov#488) <https://github.com/pytest-dev/pytest-cov/pull/488></code>_.</li> <li>Updated trove classifiers. Contributed by Michał Bielawski in <code>[#481](pytest-dev/pytest-cov#481) <https://github.com/pytest-dev/pytest-cov/pull/481></code>_.</li> </ul> <h2>2.13.0 (2021-06-01)</h2> <ul> <li>Changed the <code>toml</code> requirement to be always be directly required (instead of being required through a coverage extra). This fixes issues with pip-compile (<code>pip-tools#1300 <https://github.com/jazzband/pip-tools/issues/1300></code><em>). Contributed by Sorin Sbarnea in <code>[#472](pytest-dev/pytest-cov#472) <https://github.com/pytest-dev/pytest-cov/pull/472></code></em>.</li> <li>Documented <code>show_contexts</code>. Contributed by Brian Rutledge in <code>[#473](pytest-dev/pytest-cov#473) <https://github.com/pytest-dev/pytest-cov/pull/473></code>_.</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/560b95575efe9e55874bc8bbc99de1dd2db80ba9"><code>560b955</code></a> Bump version: 2.12.1 → 3.0.0</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/e988a6c48b45924433c9b38886f759f4f3be8a94"><code>e988a6c</code></a> Update changelog.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/f01593218d2cc99defa202a8e7ff83e3a08a7a73"><code>f015932</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-cov/issues/500">#500</a> from hugovk/add-3.10</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/60a3cc1e796428f2373fb2024122f8dbc7f1c56b"><code>60a3cc1</code></a> No need to build universal wheels for Python 3-only</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/0bc997adfea8b6ab63029163044a2fc42fd7ecf1"><code>0bc997a</code></a> Add support for Python 3.10</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/679935b82e8177799c34981e8f384a5466840301"><code>679935b</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-cov/issues/494">#494</a> from hugovk/test-on-github-actions</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/96f9aad28a35d9d0824add0ef2309e600052c531"><code>96f9aad</code></a> Add 'all good' job to be added as a required build</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/6395ecec756433194c05d34af490315b35dddafa"><code>6395ece</code></a> Test conditional collection on PyPy and CPython</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/f4a88d6187630295ce960010456def6b19ef01b2"><code>f4a88d6</code></a> Test both PyPy3.6 and PyPy3.7</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/a948e899fa002fbc7f52c6c0f7e7dffdf7987ec8"><code>a948e89</code></a> Test both PyPy3.6 and PyPy3.7</li> <li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-cov/compare/v2.12.1...v3.0.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=2.12.1&new-version=3.0.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
https://github.com/awslabs/aws-lambda-powertools-python/issues/470
First poc branch. Has only pure python, no pydantic.
Missing: md doc , schema validation implementstion (currently has a todo commengt there), more tests for exception cases
usage example can be found at this gist: https://gist.github.com/risenberg-cyberark/903bc41079218bb719c98321f8e6152b