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

Feature flags #1682

Merged
merged 4 commits into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@iilyak
Contributor

iilyak commented Oct 25, 2018

Overview

Once in a while we merge a highly experimental features. It is beneficial to be able to have the codepath for these features effectivelly disabled by default until the implementation is stabilized.
This PR introduces an ability to add feature flag checks for experimental features.
This feature is for developers only and wouldn't need documentation.

The feature configuration

[feature_flags]

my_feature||* = false
my_feature||mytestdatabaseprefix* = true

Notes:

  • The * is only allowed at the end of the path
  • In order to distinguish between shards and clustered db the following convention is used.
    • it is a shard if pattern starts with /
  • It is also possible to enable multiple flags for the path using alternative syntax
    • mytestdatabaseprefix* = [my_featureA, my_featureB]

Usage of the feature in the code

case couch_flags:is_enabled(my_feature, Subject) of
    true -> ...;
    false -> ...
end

Where Subject is one of #db{} | #httpd{) | #shard{} | #ordered_shard{} | ShardName :: binary() | ShardName :: list() | DbName :: binary() | DbName :: list()

Performance

There is a naive performance test in the test suite it does 1 million of calls to is_enabled/2. One million of calls takes "1.884 s" on the laptop.

Security

It is not safe to allow configuration of the feature flags over http since there might be a possibility of injecting arbitrary code via name of the configuration key within feature_flags section. Therefore the http access is disabled.

Testing recommendations

make eunit apps=couch suites=couch_flags_tests,couch_flags_config

Related Issues or Pull Requests

N/A

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@nickva

This comment has been minimized.

Contributor

nickva commented Oct 25, 2018

FYI: There is already a similar config-based "features" feature implementation:

apache/couchdb-config@f62d553

That's used by various components to indicated their presence and then those features are bubbled up to the top level API.

https://github.com/apache/couchdb/blob/master/src/chttpd/src/chttpd_misc.erl#L53

@iilyak

This comment has been minimized.

Contributor

iilyak commented Oct 25, 2018

@nickva FYI: There is already a similar config-based "features" feature implementation:

There are few important differences:

  1. config-based "features" are really supported capabilities. People use welcome message to check if new API is available on the cluster. While feature flags are totally internal things. The flags suppose to be deleted once feature is stable and officially released.
  2. config-based "features" are global. It is impossible to enable it for some databases and disable for others.
@nickva

This comment has been minimized.

Contributor

nickva commented Oct 25, 2018

Ah you're right. It was mostly a capabilities thing. I just wanted to call it out in the PR so it's visible. As the old thing has the (confusing) "features" name.

Maybe it would be good to replace the old thing with this? The "config" capabilities thing was a quick adhock thing, this seems better designed.

@garrensmith

Nice work.

@garrensmith garrensmith referenced this pull request Oct 26, 2018

Open

Partition feature flag #1684

1 of 1 task complete

@iilyak iilyak force-pushed the cloudant:feature_flags2 branch 5 times, most recently from f239923 to a864b31 Oct 26, 2018

@garrensmith

This comment has been minimized.

Member

garrensmith commented Nov 1, 2018

@nickva just checking, are you happy with this PR? Are you ok with us merging it?

@nickva

This comment has been minimized.

Contributor

nickva commented Nov 1, 2018

@garrensmith I didn't review the PR contents in detail so as far as that I am +0 so if you reviewed and looks good, go ahead and merge.

@garrensmith

This comment has been minimized.

Member

garrensmith commented Nov 1, 2018

@iilyak iilyak force-pushed the cloudant:feature_flags2 branch 6 times, most recently from 3b4681f to 3ce0855 Nov 5, 2018

@jaydoane
make eunit apps=couch suites=couch_flags_tests,couch_flags_config
  [done in 15.251 s]
=======================================================
  All 5059 tests passed.

(that's a lot of new tests!)

% - provides public API to use to get value for a given feature flag and subject
% - implements {feature_flags, couch_flags} service
% The module rely on couch_epi_data_gen which uses the data returned by

This comment has been minimized.

@jaydoane

jaydoane Nov 12, 2018

Contributor

s/rely/relies/

-define(SERVICE_ID, feature_flags).
enabled(Subject) ->

This comment has been minimized.

@jaydoane

jaydoane Nov 12, 2018

Contributor

I would love to see type specs defined for public functions

@iilyak iilyak force-pushed the cloudant:feature_flags2 branch from 3ce0855 to 18367a6 Nov 13, 2018

@iilyak iilyak force-pushed the cloudant:feature_flags2 branch from 18367a6 to e9abe50 Nov 13, 2018

@iilyak iilyak merged commit f12f4c5 into apache:master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iilyak iilyak deleted the cloudant:feature_flags2 branch Nov 13, 2018

@iilyak iilyak referenced this pull request Nov 22, 2018

Merged

Do not use [] in feature_flags configuration #1754

1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment