feat: introduce flag merge behaviour

[james-milligan]

This PR

• introduces a priority merge order for sync providers
• triggers a resync of the state store on delete events, ensuring the store doesn't fall out of sync with flag sources

Related Issues

Notes

Follow-up Tasks

How to test

[codecov]

Codecov Report

Merging #414 (c60db46) into main (ab3e674) will increase coverage by 1.24%.
The diff coverage is 69.33%.

❗ Current head c60db46 differs from pull request most recent head ad2ec77. Consider uploading reports for the commit ad2ec77 to get more accurate results

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
+ Coverage   65.06%   66.30%   +1.24%     
==========================================
  Files          11       11              
  Lines        1374     1395      +21     
==========================================
+ Hits          894      925      +31     
+ Misses        425      414      -11     
- Partials       55       56       +1     

Impacted Files	Coverage Δ
pkg/eval/ievaluator.go	100.00% <ø> (ø)
pkg/sync/grpc/grpc_sync.go	61.97% <41.93%> (+1.65%)	⬆️
pkg/store/flags.go	86.57% <82.60%> (+8.51%)	⬆️
pkg/eval/json_evaluator.go	87.89% <90.00%> (+0.05%)	⬆️
pkg/sync/file/filepath_sync.go	54.54% <100.00%> (+1.27%)	⬆️
pkg/sync/http/http_sync.go	46.53% <100.00%> (+3.98%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[toddbaert]

I'm looking at this now. Sorry about the wait @james-milligan

[Kavindu-Dodan]

I had a look into the PR and I think we need some agreements.

Current implementation focus on resyncs after a deletion. This means the changes we introduce only concern the store delete scenarios. In my view, we need to be careful about core logic (ex:- store - runtime - isync ) changes and breaking changes must be justifiable.

With that, can't we simply rely on the following and ignore resyncs altogether,

• Sync provider priority is defined by the order of sync args
• If low-priority providers override an existing flag simply ignore
• If high priority provider overrides an existing flag, accept that
• Ignore any other assumptions - we rely on ISync impls to refresh any changes

wouldn't such an approach work or am I not seeing the big picture? 🤔

[toddbaert]

• Sync provider priority is defined by the order of sync args
• If low-priority providers override an existing flag simply ignore
• If high priority provider overrides an existing flag, accept that
• Ignore any other assumptions - we rely on ISync impls to refresh any changes

wouldn't such an approach work or am I not seeing the big picture? thinking

That seems to be (to me) more or less what this PR does. See the tests here: https://github.com/open-feature/flagd/pull/414/files/5415b75a4c48bf3ff8014bd92428666711a0a1f8#diff-575f3e52e026f848e85c7a6cd9561cda5def99f4a58a9786801d4d4097f0ae03R12-R73

[Kavindu-Dodan]

• Sync provider priority is defined by the order of sync args
• If low-priority providers override an existing flag simply ignore
• If high priority provider overrides an existing flag, accept that
• Ignore any other assumptions - we rely on ISync impls to refresh any changes

wouldn't such an approach work or am I not seeing the big picture? thinking

That seems to be (to me) more or less what this PR does. See the tests here: https://github.com/open-feature/flagd/pull/414/files/5415b75a4c48bf3ff8014bd92428666711a0a1f8#diff-575f3e52e026f848e85c7a6cd9561cda5def99f4a58a9786801d4d4097f0ae03R12-R73

Yes, I also think this is what it does. But I am yet to see the need for a resync 🤔 If a low-priority sync provider can't override an existing flag & if sync providers publish updates for changes, then is there any reason for a force refill?

[james-milligan]

• Sync provider priority is defined by the order of sync args
• If low-priority providers override an existing flag simply ignore
• If high priority provider overrides an existing flag, accept that
• Ignore any other assumptions - we rely on ISync impls to refresh any changes

wouldn't such an approach work or am I not seeing the big picture? thinking

That seems to be (to me) more or less what this PR does. See the tests here: https://github.com/open-feature/flagd/pull/414/files/5415b75a4c48bf3ff8014bd92428666711a0a1f8#diff-575f3e52e026f848e85c7a6cd9561cda5def99f4a58a9786801d4d4097f0ae03R12-R73

Yes, I also think this is what it does. But I am yet to see the need for a resync 🤔 If a low-priority sync provider can't override an existing flag & if sync providers publish updates for changes, then is there any reason for a force refill?

If flag-a exists in 2 sync providers, (provider-A and provider-B) with provider-A taking priority, then the starup and initial datasync will result in the value of flag-a from provider-B being discarded. In the event that the value of flag-a is then deleted in provider-A then we will not recieve the previously discarded flag-a value from provider-B until there is another update event from it, and the sdk will always return the default value.
We could habdle this with with the existing Sync method, however is many cases we will be starting a new watcher just to close it after the first event is written to the channel IMO this is innapropriate.
Another option is to store all 'discarded' states incase they are needed (or become the highest priority via a deletion) but again this is overly complex for an event that will not happen too often.
The infrequency of these update events also suggests that in the example described above flagd would not have a flag value for flag-a for a long time

[Kavindu-Dodan]

• Sync provider priority is defined by the order of sync args
• If low-priority providers override an existing flag simply ignore
• If high priority provider overrides an existing flag, accept that
• Ignore any other assumptions - we rely on ISync impls to refresh any changes

wouldn't such an approach work or am I not seeing the big picture? thinking

That seems to be (to me) more or less what this PR does. See the tests here: https://github.com/open-feature/flagd/pull/414/files/5415b75a4c48bf3ff8014bd92428666711a0a1f8#diff-575f3e52e026f848e85c7a6cd9561cda5def99f4a58a9786801d4d4097f0ae03R12-R73

Yes, I also think this is what it does. But I am yet to see the need for a resync 🤔 If a low-priority sync provider can't override an existing flag & if sync providers publish updates for changes, then is there any reason for a force refill?

If flag-a exists in 2 sync providers, (provider-A and provider-B) with provider-A taking priority, then the starup and initial datasync will result in the value of flag-a from provider-B being discarded. In the event that the value of flag-a is then deleted in provider-A then we will not recieve the previously discarded flag-a value from provider-B until there is another update event from it, and the sdk will always return the default value. We could habdle this with with the existing Sync method, however is many cases we will be starting a new watcher just to close it after the first event is written to the channel IMO this is innapropriate. Another option is to store all 'discarded' states incase they are needed (or become the highest priority via a deletion) but again this is overly complex for an event that will not happen too often. The infrequency of these update events also suggests that in the example described above flagd would not have a flag value for flag-a for a long time

Thank you. I now understand the scenario you want to handle. This shows the impact of caching and the difficulty to maintain them.

I am still in favor of not having a refill strategy for removals to minimize the complexity of store and runtime components. But may be others have different opinions and this could be a desirable feature.

[beeme1mr]

Could you please add a section in the docs that describes the merge behavior. It can basically be a summary of the conversations we've had in this thread.

[beeme1mr]

Please add some debug logging when a duplicate flag key is detected from another source. While the behavior defined in this PR makes sense, it would be very difficult to troubleshoot without any indication that merging occurred.