[FEATURE] Support serving sync.proto

toddbaert commented 8 months ago

Requirements

Problem

Currently, flagd doesn't support the flagd.sync.proto. This means that flagd cannot directly serve in-process providers. Users can still use in process providers by implementing a server that serves this proto, which is great if they are interested in building a custom control plane, perhaps with a UI. However, it's perfectly reasonable for users to want to use flagd itself with in-process providers.

Proposal

Support serving the flagd.sync.proto in flagd. When a sync client connects, it can use its selector field to select a source flagd is already watching (an http endpoint, file, or CR instance). If no selector is specified, the client will get a merged representation of all sources flagd is watching (using the same merge logic as flagd). Note that unlike the flagd proxy, dynamically watching additional sources would not be supported. The sync endpoint would run on a separate port from the evaluation endpoint.

Benefits

support for in-process providers by flagd (https://github.com/open-feature/flagd/issues/961#issuecomment-1858994446)
- in-process providers completely eliminate I/O for each flag evaluation, bringing significant latency improvements
- in-process providers cache a ruleset, which can be used even if flagd is unavailable, which results in improved fault tolerance
we can completely delete the sync server stub used in e2e tests

Tasks

[x] implement flagd.sync.proto server in flagd
- [x] support use of selector as described above to select source (or merge all source if not supplied)
- [x] run on separate port from evaluation.proto
- [ ] use connect protocol, so support both web and server clients (optional, but suggested)
[x] associated testing
[x] associated documentation

Kavindu-Dodan commented 8 months ago

I will start working on this

toddbaert commented 8 months ago

I will start working on this

Amazing... let me know if you have any questions or want to meet up to think through anything.

One thing I'm so far not sure on is the use of connect... if it costs nothing, I'm fine with it... but if it's adds a lot of complexity we can re-evaluate cc @beeme1mr .

Kavindu-Dodan commented 8 months ago

Update - 01.03.2024

POC on buf connect works fine
Identified provider improvement requirements : Currently we only allow host & port for in-process grpc sync. We must add a target so that any arbitary URI can be used for this. This is specially true for connect as connect paths get build with a path segment (ex:-/flagd.sync.v1.FlagSyncService)
Started with a refactoring - https://github.com/open-feature/flagd/pull/1234 . This is to isolate service implementations and not having them in the core module, which seemed out of place for service implementations & flagd runtime

Kavindu-Dodan commented 8 months ago

Update - 04.03.2024

WIP PR #1237 for early builds. However, #1234 must be considered first and diff is huge as this is based on #1234

Kavindu-Dodan commented 8 months ago

After some deep dive, found out it would require client migrations if we wants to build this with connect support. This is because a connect server cannot work with a grpc client directly. This is highlighted at migration guide [1] as well as connect's solution to bridge the gap - vanguard-go [2]

So I will differ use connect protocol, so support both web and server clients (optional, but suggested) to another time

[1] - https://connectrpc.com/docs/go/grpc-compatibility#migration [2] - https://github.com/connectrpc/vanguard-go?tab=readme-ov-file#why-vanguard

toddbaert commented 8 months ago

After some deep dive, found out it would require client migrations if we wants to build this with connect support. This is because a connect server cannot work with a grpc client directly. This is highlighted at migration guide [1] as well as connect's solution to bridge the gap - vanguard-go [2]

So I will differ use connect protocol, so support both web and server clients (optional, but suggested) to another time

[1] - https://connectrpc.com/docs/go/grpc-compatibility#migration [2] - https://github.com/connectrpc/vanguard-go?tab=readme-ov-file#why-vanguard

I support this decision. Please just make it clear in any doc, etc. We COULD multiplex it in the future to even do it non breaking if we add it.

Kavindu-Dodan commented 8 months ago

After some deep dive, found out it would require client migrations if we wants to build this with connect support. This is because a connect server cannot work with a grpc client directly. This is highlighted at migration guide [1] as well as connect's solution to bridge the gap - vanguard-go [2] So I will differ use connect protocol, so support both web and server clients (optional, but suggested) to another time [1] - https://connectrpc.com/docs/go/grpc-compatibility#migration [2] - https://github.com/connectrpc/vanguard-go?tab=readme-ov-file#why-vanguard

I support this decision. Please just make it clear in any doc, etc. We COULD multiplex it in the future to even do it non breaking if we add it.

Agree, I have completed all other task goals with PR #1237

open-feature / flagd