chore(refactor): use memdb for flag storage#1697
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
cf6b001 to
d366b01
Compare
| 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 | ||
| go test -cover ./test/integration $(ARGS) | ||
| flagd-integration-test-harness: |
There was a problem hiding this comment.
Just added to make local integration testing easier.
| log := logger.NewLogger(nil, false) | ||
| je := NewJSON(log, store.NewFlags()) | ||
| je.store.Flags = tt.flags.Flags | ||
| je.store.Update("", "", tt.flags.Flags, model.Metadata{}) |
There was a problem hiding this comment.
Many tests have been updated to not directly mutate the State.
| import "encoding/json" | ||
|
|
||
| type Flag struct { | ||
| Key string `json:"-"` |
There was a problem hiding this comment.
New field used for indexing (not serialized).
Any other fields we add here (copies of certain metadata, flag type, etc, etc) could be used for indexing.
| @@ -0,0 +1,207 @@ | |||
| package store | |||
There was a problem hiding this comment.
Unfortunately, the large amount of changes here, plus the file rename from flags_test.go to store_test.go makes this look like a re-write - bot nothing has fundamentally changed - we are just using the public methods now instead of messing with the State struct directly.
|
/gemini summary |
Summary of ChangesThis pull request refactors the core flag storage layer to utilize Highlights
Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
An excellent refactoring effort! Switching to go-memdb is a great move for the project's future, and I appreciate the detailed explanation in the pull request description. The changes are extensive, and I've focused my review on ensuring the new implementation is robust and thread-safe.
I've found a couple of critical issues related to error handling and concurrency in the new store.go that need to be addressed. I've also included a minor suggestion for improving a test helper.
Once these points are addressed, this will be a solid foundation for the upcoming features. Great work!
86b98c4 to
3600715
Compare
|
gemini /review |
| "id": { | ||
| Name: "id", | ||
| Unique: true, | ||
| Indexer: &memdb.StringFieldIndex{Field: "Key", Lowercase: false}, | ||
| }, |
There was a problem hiding this comment.
Currently we just have a simple index on Key, to mimic the previous map behavior, but go-memdb supports all kinds of index types very useful to us, including compound indexes, numeric indexes, conditional indexes, etc. We can use these to great effect.
Note that Lowercase converts the field to lower before building the index - I've disabled this to maintain the previous, case-sensitive behavior, and for consistency with JSON map index behavior.
204768d to
3600715
Compare
aepfli
left a comment
There was a problem hiding this comment.
Thank you, this looks really cool, and i cant wait to use it. I think it will reduce our internal efforts heavily, as the memdb will offer a lot of flexibility. Thank you
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
753a92e to
53fbe0a
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
f646491 to
633107a
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
084123c to
6a5760c
Compare
🤖 I have created a release *beep* *boop* --- <details><summary>flagd: 0.13.0</summary> ## [0.13.0](flagd/v0.12.9...flagd/v0.13.0) (2025-12-23) ### 🐛 Bug Fixes * fixing sync return format missing flag layer, adding full e2e suite ([#1827](#1827)) ([570693d](570693d)) * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) ### ✨ New Features * add support for http-based ofrep metrics ([#1803](#1803)) ([fcd19b3](fcd19b3)) * cleanup evaluator interface ([#1793](#1793)) ([aa504f7](aa504f7)) * enable parsing of array flag configurations for flagd ([#1797](#1797)) ([97c6ffa](97c6ffa)) * multi-project support via selectors and flagSetId namespacing ([#1702](#1702)) ([f9ce46f](f9ce46f)) * normalize selector in sync (use header as in OFREP and RPC) ([#1815](#1815)) ([c1f06cb](c1f06cb)) ### 🧹 Chore * **refactor:** use memdb for flag storage ([#1697](#1697)) ([5c5c1cf](5c5c1cf)) ### 🔄 Refactoring * store cleanup ([#1705](#1705)) ([bcff8d7](bcff8d7)) </details> <details><summary>flagd-proxy: 0.8.1</summary> ## [0.8.1](flagd-proxy/v0.8.0...flagd-proxy/v0.8.1) (2025-12-23) ### 🐛 Bug Fixes * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) </details> <details><summary>core: 0.13.0</summary> ## [0.13.0](core/v0.12.1...core/v0.13.0) (2025-12-23) ### ⚠ BREAKING CHANGES * enable parsing of array flag configurations for flagd ([#1797](#1797)) * cleanup evaluator interface ([#1793](#1793)) * removes the `fractionalEvaluation` operator since it has been replaced with `fractional`. ([#1704](#1704)) ### 🐛 Bug Fixes * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1825](#1825)) ([44edcc9](44edcc9)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) ### ✨ New Features * Add OAuth support for HTTP Sync ([#1791](#1791)) ([268fd75](268fd75)) * Add OTEL default variables ([#1812](#1812)) ([c2e3fc6](c2e3fc6)) * allow null flagSetId Selector, restrict Selector to single key-value-pairs ([#1708](#1708)) ([#1811](#1811)) ([c12a0ae](c12a0ae)) * change jsonschema parser ([#1794](#1794)) ([bf3f722](bf3f722)) * cleanup evaluator interface ([#1793](#1793)) ([aa504f7](aa504f7)) * enable parsing of array flag configurations for flagd ([#1797](#1797)) ([97c6ffa](97c6ffa)) * multi-project support via selectors and flagSetId namespacing ([#1702](#1702)) ([f9ce46f](f9ce46f)) ### 🧹 Chore * **refactor:** use memdb for flag storage ([#1697](#1697)) ([5c5c1cf](5c5c1cf)) * removes the `fractionalEvaluation` operator since it has been replaced with `fractional`. ([#1704](#1704)) ([3228ad8](3228ad8)) ### 🔄 Refactoring * remove deprecated bearerToken option ([#1816](#1816)) ([efda06a](efda06a)) * removed unused Selector from Flag and Store. ([#1747](#1747)) ([1083005](1083005)) * store cleanup ([#1705](#1705)) ([bcff8d7](bcff8d7)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
This PR contains no behavioral changes, and no breaking changes.
It lays the groundwork for some of the upcoming improvements we want in flagd, as specified in recent ADRs. It does this by re-implementing our storage layer to use go-memdb, an open source in-memory database developed by Hashicorp and used in Consul (as well as MANY other projects).
Why?
This will allow us to easily support:
I have already PoC'd that these are all practical.
Changes in implementation
storepackage'sStateis no longer just a struct; it's a object with methods wrapping the internal dbstorepackage'sStatewas renamed toStoreand the file was renamed fromflags.gotostore.go, since it's ceased to be a simple stateful object, and for consistencyKeyfield was added to the flag type (for indexing)Storewas added which takes a logger and returns an error, the old was deprecated to avoid breaking changes in consumers (the Go flagd provider, mostly)Note that the go-memdb dependency is MPL2, which is not allowed by the CNCF, however, go-memdb is already used in CNCF projects and has a special exception.
Perfromance
There was no significant change in performance, see benchmark diff vs main below:
Benchmark diff vs main