Skip to content
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(kit/feature): add feature flag package #17851

Merged
merged 3 commits into from
Apr 30, 2020
Merged

feat(kit/feature): add feature flag package #17851

merged 3 commits into from
Apr 30, 2020

Conversation

gavincabbage
Copy link
Contributor

@gavincabbage gavincabbage commented Apr 23, 2020

This PR adds basic feature flag capabilities to InfluxDB. It allows users to create, consume and manipulate feature flags to exercise experimental feature before they become default behavior. The majority of code in this PR has been deployed internally for the past month and is merely open sourced by this change.

The best description of the functionality provided by this PR is in the included doc.go file:

Package feature provides feature flagging capabilities for InfluxDB servers. This document describes this package and how it is used to control experimental features in influxd.

Flags are configured in flags.yml at the top of this repository. Running make flags generates Go code based on this configuration to programmatically test flag values in a given request context. Boolean flags are the most common case, but integers, floats and strings are supported for more complicated experiments.

The Flagger interface is the crux of this package.
It computes a map of feature flag values for a given request context. The default implementation always returns the flag default configured in flags.yml. The override implementation allows an operator to override feature flag defaults at startup. Changing these overrides requires a restart.

In influxd, a Flagger instance is provided to a Handler middleware
configured to intercept all API requests and annotate their request context with a map of feature flags.

A flag can opt in to be exposed externally in flags.yml. If exposed, this flag will be included in the response from the new /api/v2/flags endpoint. This allows the UI and other API clients to control their behavior according to the flag in addition to the server itself.

A concrete example to illustrate the above:

I have a feature called "My Feature" that will involve turning on new code in both the UI and the server.

First, I add an entry to flags.yml.

- name: My Feature
  description: My feature is awesome
  key: myFeature
  default: false
  expose: true
  contact: My Name

My flag type is inferred to be boolean by my defaulf of false when I run make flags and the feature package now includes func MyFeature() BoolFlag.

I use this to control my backend code with

if feature.MyFeature.Enabled(ctx) {
  // new code...
} else {
  // new code...
}

and the /api/v2/flags response provides the same information to the frontend.

{
  "myFeature": false
}

While false by default, I can turn on my experimental feature by starting my server with a flag override.

env INFLUXD_FEATURE_FLAGS="{\"flag1\":\value1\",\"key2\":\"value2\"}" influxd
influxd --feature-flags flag1=value1,flag2=value2

@gavincabbage gavincabbage force-pushed the feat/flags branch 2 times, most recently from 63f384b to 239dfd0 Compare April 24, 2020 13:14
@gavincabbage gavincabbage requested a review from a team as a code owner April 24, 2020 13:14
@gavincabbage gavincabbage requested review from hoorayimhelping and removed request for a team April 24, 2020 13:14
@gavincabbage gavincabbage force-pushed the feat/flags branch 4 times, most recently from 0bd63ca to 8a1c3df Compare April 24, 2020 13:59
@drdelambre drdelambre requested a review from desa April 24, 2020 15:23
cmd/influxd/launcher/launcher.go Outdated Show resolved Hide resolved
flags.yml Outdated Show resolved Hide resolved
@gavincabbage gavincabbage force-pushed the feat/flags branch 5 times, most recently from 6fdd666 to aae18b4 Compare April 27, 2020 18:19
@gavincabbage gavincabbage requested review from a team and stuartcarnie and removed request for a team April 27, 2020 18:45
@gavincabbage gavincabbage force-pushed the feat/flags branch 2 times, most recently from 4832296 to f1586a2 Compare April 28, 2020 13:15
@gavincabbage gavincabbage requested review from brettbuddin and removed request for stuartcarnie April 28, 2020 14:57
@glinton
Copy link
Contributor

glinton commented Apr 28, 2020

Hey there @gavincabbage, i see you're into force pushing. Since you've requested reviews, please refrain from force-pushing until at least all the reviews are complete as it makes it difficult to track your recent work and wastes more time.

Copy link
Contributor

@desa desa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a couple nits/comments. Overall really clean.

d = o.Default.(map[string]string)
}
if hasShort {
flagset.StringToStringVarP(destP, o.Flag, string(o.Short), d, o.Desc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will d being nil cause a panic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with this flag both set and not and didn't run into any panics. It's just used as a default, and in launcher.go we don't use this map if its nil or empty anyway.

kit/feature/_codegen/main.go Outdated Show resolved Hide resolved
@gavincabbage
Copy link
Contributor Author

Thanks for taking a look @desa!

@desa desa self-requested a review April 28, 2020 18:24
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

kit/feature/doc.go Outdated Show resolved Hide resolved
kit/feature/doc.go Show resolved Hide resolved
kit/feature/feature.go Outdated Show resolved Hide resolved
@drdelambre drdelambre removed their request for review April 28, 2020 21:16
Copy link
Contributor

@brettbuddin brettbuddin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful work here, @gavincabbage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants