feat: multi-project support via selectors and flagSetId namespacing #1702
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
dosubot
bot
added
the
size:XXL
toddbaert
requested review from
alexandraoberaigner,
beeme1mr,
aepfli,
tangenti and
guidobrei
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @toddbaert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented substantial changes to enhance flagd's capabilities, primarily focusing on multi-project support and flexible flag querying. This involved integrating go-memdb as the new storage backend, which allowed me to introduce flagSetId for namespacing duplicate flag keys and enable a robust selector-based query syntax. A key outcome of this migration is the removal of the previous 'mux' layer, as go-memdb inherently provides the necessary watcher functionality for change notifications. These notifications are now intelligently scoped to specific selectors, ensuring that clients only receive relevant updates. This PR significantly improves flag management flexibility and system efficiency.
Highlights
- Multi-Project Support and Duplicate Flag Keys: This PR introduces support for duplicate flag keys, which are now namespaced using a
flagSetId. This allows different flag sources to define flags with the same key without conflict, provided they belong to different flag sets. - Enhanced Query Syntax with Selectors: A robust query syntax has been implemented for 'selector' fields and headers. This enables clients to request flags based on specific criteria like
sourceorflagSetId, providing more granular control over flag retrieval and evaluation. - Migration to go-memdb and Removal of Mux Layer: The core storage mechanism has been migrated to
go-memdb. This change is significant asgo-memdbprovides built-in watcher functionality, which simplifies the architecture by eliminating the need for the custom 'mux' layer previously responsible for fanning out changes. - Scoped Change Notifications: Change notifications are now scoped to particular selectors. This means clients interested in flag changes will only receive updates relevant to their specific selection (e.g., flags from a certain
flagSetIdorsource), reducing unnecessary traffic and improving efficiency. - Simplified Key-Overwrite Logic: The logic for key-overwrite by source priority has been simplified and integrated with the new
go-memdbdatabase, ensuring a more streamlined and efficient process for handling flag conflicts. - New Demo Target: A new
make run-flagd-selector-demotarget has been added to easily demonstrate and test the new selector functionality across various APIs and endpoints.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
| @@ -47,8 +47,6 @@ test-flagd: | |||
| go test -race -covermode=atomic -cover -short ./flagd/pkg/... -coverprofile=flagd-coverage.out | |||
| test-flagd-proxy: | |||
| go test -race -covermode=atomic -cover -short ./flagd-proxy/pkg/... -coverprofile=flagd-proxy-coverage.out | |||
| flagd-integration-test: # dependent on ./bin/flagd start -f file:test-harness/flags/testing-flags.json -f file:test-harness/flags/custom-ops.json -f file:test-harness/flags/evaluator-refs.json -f file:test-harness/flags/zero-flags.json | |||
There was a problem hiding this comment.
This was a duped recipe.
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to support multi-project configurations through selectors and namespacing by flagSetId. It replaces the custom "mux" layer with go-memdb's watcher functionality for handling state changes and notifications, which is a great architectural improvement. The changes are extensive, touching the core store, evaluator, and service layers. My review focuses on the correctness of the new store implementation and related changes. I've found a potential bug in the flag retrieval logic and a leftover file with commented-out code. Overall, this is a solid step forward for flagd's capabilities.
core/pkg/evaluator/json.go
Outdated
| selector := store.NewSelector("") | ||
| s := ctx.Value(store.SelectorContextKey{}) | ||
| if s != nil { | ||
| selector = s.(store.Selector) | ||
| } | ||
| allFlags, flagSetMetadata, err := je.store.GetAll(ctx, &selector, nil) |
There was a problem hiding this comment.
The IStore interface (and implementation) is also used by the in-process provider in the go-sdk-contribs. The in-process provider will not need to support selectors/quering as flagd does, so the interface was not modified. The selector is instead propagated in a special field in the context.Context, and if it's set, used in the storage layer.
Please note that we can't make breaking changes to interfaces used by the go-provider (without updating and releasing it) as it's purposely used for integration testing in this repo (ie: breaking any contracts the go in-process provider uses will cause the integration test runs here to fail).
| @@ -1,213 +0,0 @@ | |||
| package sync | |||
There was a problem hiding this comment.
The entire mux layer was deleted! This is now handled by the mem-db as mentioned.
| @@ -476,62 +476,6 @@ func TestResolveAllValues(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestMetadataResolveType(t *testing.T) { | |||
There was a problem hiding this comment.
I mentioned the reason for deleting this in the description. This is covered here now.
| @@ -0,0 +1,3 @@ | |||
| package service | |||
|
|
|||
| const FLAGD_SELECTOR_HEADER = "Flagd-Selector" | |||
There was a problem hiding this comment.
This is used for the selector in OFREP and connect (gRPC over HTTP/1.1). Apparently X- prefixes are not to be used for custom headers anymore.
Let me know if you don't like this header name.
core/pkg/store/store.go
Outdated
| if watcher != nil { | ||
|
|
||
| // a "one-time" watcher that will be notified of changes to the query | ||
| changes := it.WatchCh() | ||
|
|
||
| go func() { | ||
| select { | ||
| case <-changes: | ||
| s.logger.Debug("flags store has changed, notifying watchers") | ||
|
|
||
| // recursively get all flags again in bytes new goroutine to keep the watcher responsive | ||
| // as long as we do this in bytes new goroutine, we don't risk stack overflow | ||
| bytes, _, err := s.GetAll(ctx, selector, watcher) | ||
| if err != nil { | ||
| s.logger.Error(fmt.Sprintf("error getting flags in watcher: %v", err)) | ||
| break | ||
| } | ||
| watcher <- Payload{ | ||
| Flags: bytes, | ||
| } | ||
|
|
||
| case <-ctx.Done(): | ||
| close(watcher) | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| return flags, queryMeta, nil |
There was a problem hiding this comment.
This enables the new "Watcher" functionality. Note that the WatchCh() returns a "one-time", channel, immediately closed after one message, which is fired if anything impacting the associated query was changed, and the channel doesn't return any data - it's merely used as a signal to call the query again to see the changes, which we do with the recursive call in the new goroutine.
| // a unique index must exist for each set of constraints - for example, to look up by key and source, we need a compound index on key+source, etc | ||
| // we maybe want to generate these dynamically in the future to support more robust querying, but for now we will hardcode the ones we need |
There was a problem hiding this comment.
As the comment here mentions, we need a distinct index for every possible combination of constraint arguments. If we move beyond selector and source, we will probably need to generate these dynamically, but these are all that's needed for now.
| @@ -0,0 +1,46 @@ | |||
| package utils | |||
There was a problem hiding this comment.
This was extracted from the store, since we also need the same logic here now, to diff flag changes on the RPC event stream.
| &memdb.StringFieldIndex{Field: model.FlagSetId, Lowercase: false}, | ||
| &memdb.StringFieldIndex{Field: model.Key, Lowercase: false}, |
There was a problem hiding this comment.
📣 This is a significant change, worth noting. The new primary index for our store is flagdSetId+key. This index is unique and is fundamentally what decouples flag sources from logical flag sets. We don't reject changes that overwrite this key, so users should understand that no matter what source a flag comes from, it's fundamental "id" is a combination of the flagSetId and key. Note that, the nil flagSetId is assigned to every flag without a flagSetId, though this flagSetId cannot be specifically selected for (this can be changed, but I'm not sure it's needed).
| // NewSelector creates a new Selector from a selector expression string. | ||
| // For example, to select flags from source "./mySource" and flagSetId "1234", use the expression: | ||
| // "source=./mySource,flagSetId=1234" | ||
| func NewSelector(selectorExpression string) Selector { | ||
| return Selector{ | ||
| indexMap: expressionToMap(selectorExpression), | ||
| } | ||
| } |
There was a problem hiding this comment.
📣 One thing I think we should do, probably before merging this, is enhance this to interpret selectors without any expressions as sources, ie: "source=../my/source" and "../my/source" would mean the same thing. This is to preserve the existing behavior of the selector in the sync.proto, which currently assumes the selector is a reference to a source. If we don't do so, this will be a breaking behavioral change we will have to call out loudly.
core/pkg/store/store.go
Outdated
| // a "one-time" watcher that will be notified of changes to the query | ||
| changes := it.WatchCh() | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
Can we have a separate function for watching? It can be modified to a for(ever) loop with the WatchSet provided by memdb.
for {
txn := db.Txn(false)
ws := memdb.NewWatchSet()
iter, _ := txn.Get(...)
ws.Add(iter.WatchCh())
watcher <- ...
if err := ws.WatchCtx(ctx); err != nil {
close(watcher)
return err // ctx cancelled or deadline
}
// loop restarts with a brand-new snapshot & watch set
}
There was a problem hiding this comment.
This sounds much nicer than my optional recursion. Let me see...
There was a problem hiding this comment.
This is much nicer. Excellent recommendation. Please see this commit for my changes (mostly related to this comment, but also some of your others).
Please note that to add the Watch func you recommended here, I extracted some private functions to reduce repitition (otherwise GetAll and Watch would have contained some trivial duplication). Despite that, I still think the ergonomics of your proposal are better, since in many cases we did a GetAll without watchng, and vice versa.
I also extracted the notifications as you suggested and added tests for that.
cc @chrfwow
core/pkg/store/query.go
Outdated
| } | ||
|
|
||
| // Split the selector by commas | ||
| pairs := strings.Split(selector, ",") |
There was a problem hiding this comment.
It seems both , and = are allowed in the flag set IDs, so the parsing here may not work as intended for corner cases.
There was a problem hiding this comment.
I considered this, but wasn't sure how to proceed! Thanks for the eye for detail. ..
Do you have a suggestion? In general there's not much validation here. I can't decide on the best course. I'm not sure I want to implement escape characters or something.
There was a problem hiding this comment.
Any thoughts on this?
There was a problem hiding this comment.
Since we're getting this data as an HTTP Header, I would vote to follow the format specified in RFC9651 or a subset of it since I don't think we will support list 🤔
Hence, we should replace , with ; and disallow = in the schema definition of metadata.flagSetId.
There was a problem hiding this comment.
I would prefer to keep the simplicity for the moment until we have better understandings of more use cases of the selector.
There was a problem hiding this comment.
I agree with keeping it simple for now, but @thisthat 's proposal brings up the question of whether , or ; might be a better separator token.
There was a problem hiding this comment.
If I have to choose, I'd go with ; since it splits parameters. However, I am happy with both options as long as it is removed from being a valid char in the schema. We can discuss this in a follow-up since this PR is already quite big :)
There was a problem hiding this comment.
In general I think it's not a good practice to mark some chars as special.
I've considered ANTLR but it seems a bit overkill at the moment, but if we don't have a better option, I'd suggesting using ANTLR
I will run or benchmark suite again, like I did when we did the memdb migration. I expect things will be slower, especially for "selector-less" requests, but IMO as long as it's not an order of magnetude shower or more, it's acceptable, considering how fast flagd already is. |
| oldFlag, ok := raw.(model.Flag) | ||
| // if we already have a flag with the same key and source, we need to check if it has the same flagSetId | ||
| if ok { | ||
| if oldFlag.FlagSetId != newFlag.FlagSetId { |
There was a problem hiding this comment.
why do we delete the flag if the flagSets are different?
There was a problem hiding this comment.
Good question.
It's because logically the primary key is flagSetId+key. We are looping through all the flags for a source here; if the flag set has changed, the previous flag must be deleted (any clients selecting on the old flag set should get a message now missing this flag). This is an important case some of my new tests cover.
Does this make sense?
I can add a comment about this.
There was a problem hiding this comment.
I've added a comment to this effect.
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Todd Baert <[email protected]>
|
EDIT: Fixed. |
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
|
@tangenti @beeme1mr after running the benchmark suite, there's a small reduction in speed (some tests are identical to before the change, or even slightly faster, and the most negatively impacted are ~33% slower) but this is not significant in my opinion; we're still talking about 1/100th of a millisecond for our slowest operations. There's also a small increase in allocations (something like 10%, eyeballing it). See below: Bench diff-BenchmarkFractionalEvaluation/[email protected] 423930 13316 ns/op 7229 B/op 135 allocs/op
-BenchmarkFractionalEvaluation/[email protected] 469594 13677 ns/op 7229 B/op 135 allocs/op
-BenchmarkFractionalEvaluation/[email protected] 569103 13286 ns/op 7229 B/op 135 allocs/op
-BenchmarkFractionalEvaluation/[email protected] 412386 13023 ns/op 7229 B/op 135 allocs/op
-BenchmarkResolveBooleanValue/test_staticBoolFlag-16 3106903 1792 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveBooleanValue/test_targetingBoolFlag-16 448164 11250 ns/op 6065 B/op 87 allocs/op
-BenchmarkResolveBooleanValue/test_staticObjectFlag-16 3958750 1476 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveBooleanValue/test_missingFlag-16 5331808 1353 ns/op 784 B/op 12 allocs/op
-BenchmarkResolveBooleanValue/test_disabledFlag-16 4530751 1301 ns/op 1072 B/op 13 allocs/op
-BenchmarkResolveStringValue/test_staticStringFlag-16 4583056 1525 ns/op 1040 B/op 13 allocs/op
-BenchmarkResolveStringValue/test_targetingStringFlag-16 839954 10388 ns/op 6097 B/op 89 allocs/op
-BenchmarkResolveStringValue/test_staticObjectFlag-16 4252830 1677 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveStringValue/test_missingFlag-16 3743324 1495 ns/op 784 B/op 12 allocs/op
-BenchmarkResolveStringValue/test_disabledFlag-16 3495699 1709 ns/op 1072 B/op 13 allocs/op
-BenchmarkResolveFloatValue/test:_staticFloatFlag-16 4382868 1511 ns/op 1024 B/op 13 allocs/op
-BenchmarkResolveFloatValue/test:_targetingFloatFlag-16 867987 10344 ns/op 6081 B/op 89 allocs/op
-BenchmarkResolveFloatValue/test:_staticObjectFlag-16 3913120 1695 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveFloatValue/test:_missingFlag-16 3910468 1349 ns/op 784 B/op 12 allocs/op
-BenchmarkResolveFloatValue/test:_disabledFlag-16 3642919 1666 ns/op 1072 B/op 13 allocs/op
-BenchmarkResolveIntValue/test_staticIntFlag-16 4077288 1349 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveIntValue/test_targetingNumberFlag-16 922383 7601 ns/op 6065 B/op 87 allocs/op
-BenchmarkResolveIntValue/test_staticObjectFlag-16 4995128 1229 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveIntValue/test_missingFlag-16 5574153 1274 ns/op 768 B/op 12 allocs/op
-BenchmarkResolveIntValue/test_disabledFlag-16 3633708 1734 ns/op 1072 B/op 13 allocs/op
-BenchmarkResolveObjectValue/test_staticObjectFlag-16 1624102 4559 ns/op 2243 B/op 37 allocs/op
-BenchmarkResolveObjectValue/test_targetingObjectFlag-16 443880 11995 ns/op 7283 B/op 109 allocs/op
-BenchmarkResolveObjectValue/test_staticBoolFlag-16 3462445 1665 ns/op 1008 B/op 11 allocs/op
-BenchmarkResolveObjectValue/test_missingFlag-16 4207567 1458 ns/op 784 B/op 12 allocs/op
-BenchmarkResolveObjectValue/test_disabledFlag-16 3407262 1848 ns/op 1072 B/op 13 allocs/op
-PASS
-ok github.com/open-feature/flagd/core/pkg/evaluator 239.506s
+BenchmarkFractionalEvaluation/[email protected] 466902 17410 ns/op 7683 B/op 150 allocs/op
+BenchmarkFractionalEvaluation/[email protected] 369529 15836 ns/op 7683 B/op 150 allocs/op
+BenchmarkFractionalEvaluation/[email protected] 384133 16960 ns/op 7683 B/op 150 allocs/op
+BenchmarkFractionalEvaluation/[email protected] 322873 17541 ns/op 7683 B/op 150 allocs/op
+BenchmarkResolveBooleanValue/test_staticBoolFlag-16 3500460 2044 ns/op 1176 B/op 25 allocs/op
+BenchmarkResolveBooleanValue/test_targetingBoolFlag-16 610275 10674 ns/op 6249 B/op 101 allocs/op
+BenchmarkResolveBooleanValue/test_staticObjectFlag-16 2736013 2451 ns/op 1192 B/op 25 allocs/op
+BenchmarkResolveBooleanValue/test_missingFlag-16 2921277 2202 ns/op 1016 B/op 28 allocs/op
+BenchmarkResolveBooleanValue/test_disabledFlag-16 2441956 2608 ns/op 1240 B/op 27 allocs/op
+BenchmarkResolveStringValue/test_staticStringFlag-16 2510768 2509 ns/op 1224 B/op 27 allocs/op
+BenchmarkResolveStringValue/test_targetingStringFlag-16 419666 12443 ns/op 6282 B/op 103 allocs/op
+BenchmarkResolveStringValue/test_staticObjectFlag-16 2544326 2534 ns/op 1192 B/op 25 allocs/op
+BenchmarkResolveStringValue/test_missingFlag-16 2883064 2302 ns/op 1016 B/op 28 allocs/op
+BenchmarkResolveStringValue/test_disabledFlag-16 2482075 2679 ns/op 1241 B/op 27 allocs/op
+BenchmarkResolveFloatValue/test:_staticFloatFlag-16 2715316 2462 ns/op 1192 B/op 27 allocs/op
+BenchmarkResolveFloatValue/test:_targetingFloatFlag-16 491826 13936 ns/op 6265 B/op 103 allocs/op
+BenchmarkResolveFloatValue/test:_staticObjectFlag-16 2295111 2364 ns/op 1192 B/op 25 allocs/op
+BenchmarkResolveFloatValue/test:_missingFlag-16 2779935 2477 ns/op 1016 B/op 28 allocs/op
+BenchmarkResolveFloatValue/test:_disabledFlag-16 2432382 2799 ns/op 1241 B/op 27 allocs/op
+BenchmarkResolveIntValue/test_staticIntFlag-16 2850421 2564 ns/op 1176 B/op 25 allocs/op
+BenchmarkResolveIntValue/test_targetingNumberFlag-16 628972 12417 ns/op 6250 B/op 101 allocs/op
+BenchmarkResolveIntValue/test_staticObjectFlag-16 2350480 2459 ns/op 1192 B/op 25 allocs/op
+BenchmarkResolveIntValue/test_missingFlag-16 3103119 2174 ns/op 1000 B/op 28 allocs/op
+BenchmarkResolveIntValue/test_disabledFlag-16 2485292 2499 ns/op 1241 B/op 27 allocs/op
+BenchmarkResolveObjectValue/test_staticObjectFlag-16 1000000 5445 ns/op 2427 B/op 51 allocs/op
+BenchmarkResolveObjectValue/test_targetingObjectFlag-16 326774 16668 ns/op 7467 B/op 123 allocs/op
+BenchmarkResolveObjectValue/test_staticBoolFlag-16 2559854 2671 ns/op 1176 B/op 25 allocs/op
+BenchmarkResolveObjectValue/test_missingFlag-16 2775954 2186 ns/op 1016 B/op 28 allocs/op
+BenchmarkResolveObjectValue/test_disabledFlag-16 2447349 2761 ns/op 1241 B/op 27 allocs/op |
core/pkg/store/query.go
Outdated
| } | ||
|
|
||
| // Split the selector by commas | ||
| pairs := strings.Split(selector, ",") |
There was a problem hiding this comment.
I would prefer to keep the simplicity for the moment until we have better understandings of more use cases of the selector.
toddbaert
force-pushed
the
feat/selector
branch
from
783916d to
421053c
Compare
toddbaert
force-pushed
the
feat/selector
branch
from
421053c to
e25a49d
Compare
Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
feat/selector
branch
from
e25a49d to
56b743e
Compare
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
feat/selector
branch
from
12dfe41 to
0c169de
Compare
|
|
||
| // flagSetId defaults to a UUID generated at startup to make our queries consistent | ||
| // any flag without a "flagSetId" is assigned this one; it's never exposed externally | ||
| var nilFlagSetId = uuid.New().String() |
There was a problem hiding this comment.
What's the motivation behind this decision?
IIUC, this will make it impossible for providers to select those flags without a flag set ID.
There was a problem hiding this comment.
The motivation is mostly due to limitations in the query language and index support of the DB. To support the compound flagSetId+key primary index we want, we have to put something in the flagSetId, even when there isn't one defined by the config.
These flags can be selected by simply omitting a flagSetId. Logically, we are treating the flagSetId: null as it's own set, but the way the Get query with no constraints works here, everything is returned. In combination, we get the behavior that specifying a flagSetId will only return flags belonging to that set, but omitting one will return everything (with the previous "source priority" rules honored for duplicated flags). I have tests covering this in the store_test.go. See gif for a demo.
There was a problem hiding this comment.
Note that currently, there's no way to explicitly select flags without a flagSetId - but I think that's acceptable, and perhaps even optimal - these flags don't belong to a particular set, so I would argue they shouldn't be queried that way, but instead only be returned in source-queries, or set-less queries as above. This can be changed though.
There was a problem hiding this comment.
Thanks. I'll take a look tomorrow to see if such query can be supported without a UUID hack.
I think we discussed the problem when introducing the flag lists. Imagine there are two flags {key: feature, set: A} and {key: feature, set: null}, when an in-process provider constructs the selector, there's no way for it to get a valid flag map with {key: feature, set: null} in the result. It will be a problem for adopting flag sets - basically without the ability to select on the null set, users have to create a "default" set for all existing flags if they need to add a flag with the same key under another set.
There was a problem hiding this comment.
Thanks. I'll take a look tomorrow to see if such query can be supported without a UUID hack.
I think it can be done with a memdb.ConditionalIndex; but IMO, it adds some complexity to do it this way, without much additional value (UUIC, you'd need to do a query something like txn.Get(flagsTable, flagSetPresentIndex, setISPresent) where flagSetPresentIndex is a memdb.ConditionalIndex index returning true if flagSetId is set, and setISPresent is a bool parameter to match the memdb.ConditionalIndex predicate's return value against. Maybe you could branch of mine, and give it a shot. If it works I'm happy to merge it in.
You also may need to use a memdb.CompoundMultiIndex, because as the in-line doc says:
if AllowMissing is set, not only is it valid to have empty index fields, but it will still create index values up to the first empty index. This means that if you have a value with an empty field, rather than using a prefix for lookup, you can simply pass in less arguments
Otherwise, we can't even insert a a flag with a zero-value for flagSetId. There may be other ways as well, including some iteration.
I think we discussed the problem when introducing the flag lists. Imagine there are two flags {key: feature, set: A} and {key: feature, set: null}, when an in-process provider constructs the selector, there's no way for it to get a valid flag map with {key: feature, set: null} in the result. It will be a problem for adopting flag sets - basically without the ability to select on the null set, users have to create a "default" set for all existing flags if they need to add a flag with the same key under another set.
This is true; I suppose I was being a but myopic here, assuming that if you're using flagSets you're using them comprehensively.
Even with the current implementation with the "UUID hack", it's possible to implement selection on null - I think the biggest question is how we'd encode that in the selector... would it be "flagsSetId=" ? Another option could be to automatically put them in a "well-known" default flagSet... (ie: flagSetId=default) but I'm less enthusiastic about that. Take care that we also need to maintain the old behavior where all flags are returned and the source-based priority override is honored if no flagSetId is specified (even if we make it possible to select on the old set)
There was a problem hiding this comment.
@tangenti would you be OK with merging this as is, and creating a new issue to discuss and implement:
- selection on null flagSetId
- possible refactors to avoid the nilFlagSetId/uuid hack?
There was a problem hiding this comment.
Yes, please. Sorry for not being able to check this earlier.
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Sorry for how big this PR seems (a lot of the change lines are not significant (DB schema or tests), so I've tried to highlight the important parts. This is a substantial PR that builds on our recent changes to use go-memdb for storage. It primarily supports 2 new crucial features:
flagSetId) as described in this ADRBoth of these were accomplished using the new go-memdb module. The built-in "watcher" functionality also allows us to completely delete our "mux" layer, which was responsible for fanning out changes from sync-sources to listeners; this is something the go-memdb module providers for free (the ability to watch a query for changes). Now, we completely rely on this feature for all change notifications. Additionally, unlike before, change notifications are now scoped to particular selectors (ie: if a client is only interested in changes for flags from
flagSetId: xorsource: y, they will only get change notifications pertaining to that selection. Currently, the only supported query fields for theselectorare"source"and"flagSetId", but this functionality can easily be extended. By default, if noselectoris specified, the previous key-overwrite by source priority apples (this logic has also been simplified using the new database). Most of the new functionality is tested here.Selector in action for
SyncAPI:Selector in action for
EvaluationAPI:To test, run the new make target
make run-flagd-selector-demo, then use the OFREP or gRPC endpoints to experiment. This new functionality is available on all APIs and endpoints. The command I ran in the gifs above are:streaming:
single-evaluations:
So if you used a selector like
"flagSetId=1234,source=../my/source", the top-level metadata object in the response would be:This is useful for the provider's ability to report errors, etc in telemetry.
Fixes: #1675
Fixes: #1695
Fixes: #1611
Fixes: #1700
Fixes: #1610