Complex FF Definition #12

asoni-stripe · 2018-03-06T21:20:12Z

This is a WIP branch for complex feature flag definition. Formalizes a flag into three properties -- name, an active toggle (on/off), and an optional sequence of rules.

Rules can be of many types -- we have a match_list and rate rule type initially.

Rules must be able to be executed and produce a boolean value, and must specify behavior if evaluated to true or false (return true, return false, or continue to the next rule). Clients must now provide a map of properties which will be used by rules in their evaluation.

vasi-stripe · 2018-03-06T21:58:16Z

flags.go

+		}
+		h.Write([]byte(buffer.String()))
+		bs := h.Sum(nil)
+		encodedStr := hex.EncodeToString(bs)


You can just use binary.LittleEndian.Uint16(buf) to get a uint16 from raw binary bytes :)

Also, we should probably use 64 bits!

I figured there was a way instead of going thru this hex workaround! Thanks 👍 I also thought it was odd the Ruby impl limited it to 16 bits...I figured the more the better in terms of randomness/bits of entropy but tried to mirror the Ruby impl as much as possible.

vasi-stripe · 2018-03-06T22:00:02Z

examples_test.go

@@ -14,15 +14,18 @@ func Example() {
 	backend := BackendFromFile("flags.csv")
 	Init(30*time.Second, backend)

-	if Enabled(ctx, "go.sun.mercury") {
+	enabled, err := Enabled(ctx, "go.sun.moon", map[string]string{"host_name": "apibox_123"})


This doesn't agree with the log line below it 😨

vasi-stripe · 2018-03-06T22:00:21Z

flags.go

-	Flags       []Flag  `json:"flags"`
-	UpdatedTime float64 `json:"updated"`
+	Flags       []FlagJSONFormat `json:"flags"`
+	UpdatedTime float64          `json:"updated"`
 }


We should try to retain the ability to parse "version 1" of this.

vasi-stripe · 2018-03-06T22:01:34Z

flags.go

+type MatchListRule struct {
+	Property string
+	Values   []string
+	OnMatch  string `json:"on_match"`


It seems a bit weird for both rule types to have OnMatch and OnMiss. Maybe instead we can factor that out?

type RuleInfo struct { OnMatch string OnMiss string Rule Rule }

vasi-stripe · 2018-03-06T22:02:52Z

flags.go

+func getProperty(ctx context.Context, props map[string]string, prop string) string {
+	if v, ok := props[prop]; ok {
+		return v
+	} else if v, ok := ctx.Value(overrideContextKey).(string); ok {


overrideContextKey lets us say "flag foo is forced enabled". It's not useful for getting tags.

Ah sorry this was me being careless with copy+paste :|

vasi-stripe · 2018-03-06T22:03:16Z

flags.go

+	} else if v, ok := ctx.Value(overrideContextKey).(string); ok {
+		return v
+	} else {
+		return ""


It should probably be an error of some sort if the tag doesn't exist in props.

Yup agreed, adjusting

vasi-stripe · 2018-03-06T22:05:03Z

flags.go

@@ -188,7 +313,48 @@ func parseFlagsJSON(r io.Reader) (map[string]Flag, time.Time, error) {
 		log.Print("[goforit] error parsing JSON file:\n", err)
 		return nil, time.Time{}, err
 	}
-	return flagsToMap(v.Flags), time.Unix(int64(v.UpdatedTime), 0), nil
+	// we first unmarshall into the intermediate FlagJSONFormat


You can do some cool things with the json.Unmarshaler interface here. https://golang.org/pkg/encoding/json/#Unmarshaler . Then each type becomes responsible for unmarshaling itself, it makes the code easier to read usually.

vasi-stripe · 2018-03-06T22:05:51Z

flags.go

-		flags[name] = Flag{Name: name, Rate: rate}
+		var active = false
+		var rules = []Rule{}
+		if rate > 0 {


You could just unconditionally add the RateRule, right? If Rate: 0, then it will be equivalent to off

vasi-stripe · 2018-03-06T22:06:40Z

flags.go

+		case "off":
+			enabled = false
+			return
+		case "match_list":


Is "match_list" really a matchBehavior???

Sigh another careless mistake...changed it to "continue". Thanks!

vasi-stripe · 2018-03-06T22:07:10Z

flags.go

+			return
+		}
+	}
+	enabled = true


If we get to the bottom, it should probably be disabled!

Fair enough, changed!

vasi-stripe · 2018-03-06T22:07:31Z

flags.go

+			matchBehavior = r.onMiss()
+		}
+		switch matchBehavior {
+		case "on":


Can we use constants for "on", "off", "continue"? And their own type even!

vasi-stripe · 2018-03-06T22:08:24Z

Overall this looks quite good!

vasi-stripe · 2018-03-08T17:40:39Z

backend.go

@@ -23,11 +24,77 @@ type jsonFileBackend struct {
 	filename string
 }

+type flagJson struct {


This is cute, it's like a merge of both old and new flags!

It might be better to use Version to select old vs new? But I'm uncertain, happy to keep this. We should add a comment to explain what this is for, since it's not obvious.

i thought about versioning but if its ok with you, i think this just might be simpler for now! i added a comment explaining what was happening

Yup, that's fine

vasi-stripe · 2018-03-08T17:41:34Z

backend.go

+
+	if len(raw.Rules) == 0 {
+		if raw.Rate > 0 {
+			raw.Active = true


And if the Rate is 0, then we're still ok, because that's equivalent to inactive. Cooool. Again, comment this :)

yup! i added a comment here as well.

vasi-stripe · 2018-03-08T17:44:19Z

flags.go

+			matchBehavior = r.OnMiss
+		}
+		switch matchBehavior {
+		case "on":


Use constants here please.

good catch, fixed!

vasi-stripe · 2018-03-08T17:45:10Z

flags.go

 	enabled = false
 	return
 }

+//


woops...removed

vasi-stripe · 2018-03-08T17:45:44Z

flags.go

+		case "continue":
+			continue
+		default:
+			log.Printf("[goforit] unknown match behavior: " + string(matchBehavior))


Things like this should probably be some kind of metric too? But let's punt on that for now, maybe it's a good case for Sentry integration.

Also, we should use the logger inside g *goforit instead of just a basic log.

changed to g.logger, but yeah would love to tackle Sentry before we ship!

vasi-stripe · 2018-03-08T17:46:59Z

flags.go

+		var buffer bytes.Buffer
+		buffer.WriteString(flag)
+		for _, val := range r.Properties {
+			prop, err := getProperty(props, val)


We should add a separator between props, so that ("foo", "barblah") is not equivalent to ("foobar","blah"). Maybe use a zero byte, since that's unlikely to be in an actual property value.

vasi-stripe · 2018-03-08T17:47:28Z

flags.go

+		bs := h.Sum(nil)
+		// get the most significant 32 digits
+		x := binary.BigEndian.Uint32(bs)
+		// check to see if the 16 most significant bits of the hex


This comment is a lie, we're not using 16 bits

good catch, fixed :)

vasi-stripe · 2018-03-08T17:48:40Z

flags.go

+	for _, r := range flag.Rules {
+		res, err := r.Rule.Handle(flag.Name, mergedProperties)
+		if err != nil {
+			log.Printf("[goforit] error evaluating rule:\n", err)


Using Printf without any percent-interpolations! That's making some tests fail

ah, thats why the builds were failing. great, fixed them up!

vasi-stripe · 2018-03-08T17:49:29Z

flags.go

@@ -201,6 +334,12 @@ func (g *goforit) SetStalenessThreshold(threshold time.Duration) {
 	g.stalenessThreshold = threshold
 }

+func (g *goforit) AddDefaultTags(tags map[string]string) {
+	for k, v := range tags {


Protect with a mutex?

yes, was meaning to add this, just did! thanks!

vasi-stripe · 2018-03-08T17:50:48Z

flags_test.go

@@ -182,6 +292,56 @@ func assertFlagsEqual(t *testing.T, expected, actual map[string]Flag) {
 	}
 }

+// func TestDefaultTags(t *testing.T) {


removed, thanks

vasi-stripe · 2018-03-08T17:52:28Z

flags_test.go

+	// test combination of global tag and local props
+	g.AddDefaultTags(map[string]string{"cluster": "northwest-01"})
+	assert.True(t, g.Enabled(context.Background(), "test", map[string]string{"host_name": "apibox_001", "db": "mongo-prod"}))
+


We should also be able to check the log buffer for errors.

vasi-stripe · 2018-03-08T17:53:13Z

Getting super close!

vasi-stripe · 2018-03-08T20:20:21Z

flags.go

 	for k, v := range g.defaultTags {
 		mergedProperties[k] = v
 	}
+	g.tagsMtx.RUnlock() // should i defer this? maybe inside its own scope?


We could have a little getDefaultTags method to copy-and-return them :)

vasi-stripe · 2018-03-08T20:23:51Z

flags.go

+	}
+
+	// if there are no rules, but flag is active, always return true
+	if len(flag.Rules) == 0 {


This is confusing. Shouldn't "continue off the end of the rules" always have the same behavior?

vasi-stripe · 2018-03-08T20:24:47Z

Awesome tests!

vasi-stripe · 2018-03-09T00:30:18Z

LGTM

asoni-stripe added 3 commits March 5, 2018 14:31

init

2204dcb

update tests

62731ce

running goimports

9ce1e15

vasi-stripe reviewed Mar 6, 2018

View reviewed changes

asoni-stripe added 6 commits March 6, 2018 23:07

addressing vasi review

19cbfe2

rebase with master

85ce252

addressing more review comments

f2a1ec7

goimports and remove context from Handle()

40be54b

accidental removal of helper method in tests

43eff5d

adding more unit tests

6a1efae

vasi-stripe reviewed Mar 8, 2018

View reviewed changes

add more unit tests and address vasi latest review

3e41cc0

vasi-stripe reviewed Mar 8, 2018

View reviewed changes

final cleanup

ee30480

vasi-stripe changed the title ~~[WIP] Complex FF Definition~~ Complex FF Definition Mar 9, 2018

asoni-stripe merged commit ce6c759 into master Mar 9, 2018

Complex FF Definition #12

Complex FF Definition #12

Conversation

asoni-stripe commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasi-stripe commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasi-stripe commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasi-stripe commented Mar 8, 2018

vasi-stripe commented Mar 9, 2018