refactor(ffi): refactor engine for streaming #430

markphelps · 2024-10-08T11:58:31Z

Opening this up for feedback. It hasnt yet been tested with streaming server side changes yet, however I feel like this PR is large enough already that we should go ahead and open it.

What

Refactors the flipt-ffi-engine module with the following major changes:

Renames HTTPParser to HTTPFetcher
Decouples fetching from the evaluation store
Removes generic parameter P from evaluator as it will always use the HTTPFetcher (could prob do the same for Snapshot store in the future
Removes unneeded method repace_store to just use replace_snapshot on state change
Switches to use Rust channels to communicate state changes, which makes switching to leverage streaming easier as well
Adds streaming mode, described below

Polling / Streaming Flow

This adds initial support for flipt-io/flipt#3458

The client will be able to be configured in either streaming or polling mode. Default will be polling, but they will be able to enable streaming via engine options (yet to be decided how this is exposed to user).

Right now this only is a refactor of the existing polling code, as the streaming functionality is never called.

TODO

Test
Wire up ability to put in streaming mode via engine opts

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: build(deps): bump org.junit:junit-bom in /flipt-client-java (#429) build(deps-dev): bump rollup in /flipt-client-browser (#428) build(deps-dev): bump rollup in /flipt-client-react (#427) build(deps-dev): bump globals in /flipt-client-react (#426) build(deps-dev): bump eslint-plugin-react in /flipt-client-react (#425)

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

codecov-commenter · 2024-10-08T15:47:59Z

Codecov Report

Attention: Patch coverage is 58.56481% with 179 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (1f357f8) to head (1d01a26).

Files with missing lines	Patch %	Lines
flipt-engine-ffi/src/http.rs	61.07%	116 Missing ⚠️
flipt-engine-ffi/src/lib.rs	0.00%	60 Missing ⚠️
flipt-evaluation/src/store.rs	88.23%	2 Missing ⚠️
flipt-evaluation/src/models/source.rs	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
- Coverage   87.42%   84.09%   -3.33%     
==========================================
  Files           9        8       -1     
  Lines        3737     3936     +199     
==========================================
+ Hits         3267     3310      +43     
- Misses        470      626     +156

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: feat(browser): allow caller to know if the snapshot was updated (#431)

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

flipt-engine-ffi/src/http.rs

flipt-engine-ffi/src/lib.rs

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: build(deps-dev): bump @eslint/js in /flipt-client-react (#447) build(deps-dev): bump @types/react in /flipt-client-react (#445) build(deps-dev): bump globals in /flipt-client-react (#443) build(deps-dev): bump typescript in /flipt-client-browser (#442) build(deps-dev): bump undici in /flipt-client-browser (#441) build(deps-dev): bump black in /flipt-client-python (#440) build(deps-dev): bump @types/node in /flipt-client-node (#439)

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

markphelps · 2024-10-15T18:21:20Z

@GeorgeMac I think this is ready for a re-review

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

GeorgeMac

Nice one.

Do we want to add comments to all these new Streaming enum types to make it clear this is experimental / cloud only for now?

markphelps · 2024-10-16T14:29:02Z

Nice one.

Do we want to add comments to all these new Streaming enum types to make it clear this is experimental / cloud only for now?

yah will do

erka · 2024-10-16T16:32:05Z

flipt-engine-ffi/src/http.rs

+    ) -> Result<(), Error> {
+        let stream_result = self.fetch_stream().await;
+
+        match stream_result {


Maybe it's too late. Could serde_json::StreamDeserializer be used here?

maybe, this works however. do you know if serde_json::StreamDeserializer would provide a benefit?

I am worry about bytes and \n on utf-8 string. But it could be done with refactoring.

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: feat: expose if refresh changed state in node sdk (#450)

markphelps · 2024-10-16T21:49:36Z

Im going to add more tests coverage and document the new streaming option before merging

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

markphelps added 12 commits October 7, 2024 15:52

refactor: decouple evaluator and parser to support streaming

fa65c46

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: use channels for message passing

2c94978

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: handle errors from http

24ee891

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: return response from fetcher

924cf53

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

feat: impl streaming

3a6d4fb

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: rm streaming file

ad2bf0c

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

Merge branch 'main' into refactor-engine-for-streaming

f669881

chore: fix async tests

670cf7a

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: handle json parsing error

c0c3b44

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: rm unneeded test

d2f8912

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: rm tokio from main deps

5c7f1e1

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

markphelps added 13 commits October 8, 2024 15:09

chore: fix sending async

9be83ba

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: minor cleanup

4c04c6e

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

Merge branch 'main' into refactor-engine-for-streaming

0220f22

chore: add some logging

fd5e7db

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

Merge branch 'refactor-engine-for-streaming' of https://github.com/fl…

fac25b6

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: feat(browser): allow caller to know if the snapshot was updated (#431)

chore: try to fix thread lifecycle management

d1cce89

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: put polling in loop

f689aff

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: add some debug logging

eebe09d

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: dont reset snapshot on error state

2dadeec

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: dont print error on shutdown

812eb4e

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: handle wait for async code to finish

43d1ce5

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: use println for debug

77b4543

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: more debug info

f21bff8

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

GeorgeMac reviewed Oct 9, 2024

View reviewed changes

flipt-engine-ffi/src/http.rs Outdated Show resolved Hide resolved

GeorgeMac reviewed Oct 9, 2024

View reviewed changes

flipt-engine-ffi/src/lib.rs Outdated Show resolved Hide resolved

markphelps added 2 commits October 9, 2024 11:01

chore: perform initial fetch on engine start

bfa55b3

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: refactor initial fetch

4039d19

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

markphelps added 2 commits October 11, 2024 11:58

Merge branch 'main' into refactor-engine-for-streaming

cfa0e4f

Merge branch 'main' into refactor-engine-for-streaming

4747b15

markphelps changed the title ~~refactor: refactor engine for streaming~~ refactor(ffi): refactor engine for streaming Oct 14, 2024

markphelps added 11 commits October 14, 2024 14:52

chore: fix url for streaming

313d808

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

Merge branch 'main' into refactor-engine-for-streaming

245463a

chore: fix streaming test

f86b18d

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: expose fetch mode to other languages

264a807

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: fix java build

43d30d0

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: rename file cause java

89f53ca

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: handle stream chunk parsing; rm timeout

c7dc529

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: fmt java

7288f7f

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: ruby fmt

6fa3e23

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: fmt python

088d3d6

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: retry requests, ensure consistency in error handling

1801370

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

GeorgeMac approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'main' into refactor-engine-for-streaming

28953e9

erka reviewed Oct 16, 2024

View reviewed changes

markphelps added 3 commits October 16, 2024 12:50

Merge branch 'main' into refactor-engine-for-streaming

3453519

chore: assert expected retries for fetch error

3ca4298

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

Merge branch 'refactor-engine-for-streaming' of https://github.com/fl…

57a3166

…ipt-io/flipt-client-sdks into refactor-engine-for-streaming * 'refactor-engine-for-streaming' of https://github.com/flipt-io/flipt-client-sdks: feat: expose if refresh changed state in node sdk (#450)

markphelps added 2 commits October 17, 2024 10:48

chore: add more tests and documentation

a314ee7

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

chore: java fmt

1d01a26

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>

markphelps added the automerge label Oct 17, 2024

kodiakhq bot merged commit 40022f7 into main Oct 17, 2024
18 checks passed

kodiakhq bot deleted the refactor-engine-for-streaming branch October 17, 2024 15:20

markphelps mentioned this pull request Oct 17, 2024

feat: support streaming in node JS SDK #452

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ffi): refactor engine for streaming #430

refactor(ffi): refactor engine for streaming #430

markphelps commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading

markphelps commented Oct 15, 2024

GeorgeMac left a comment

markphelps commented Oct 16, 2024

erka Oct 16, 2024

markphelps Oct 16, 2024

erka Oct 16, 2024

markphelps commented Oct 16, 2024

refactor(ffi): refactor engine for streaming #430

refactor(ffi): refactor engine for streaming #430

Conversation

markphelps commented Oct 8, 2024 • edited Loading

What

Polling / Streaming Flow

TODO

codecov-commenter commented Oct 8, 2024 • edited Loading

Codecov Report

markphelps commented Oct 15, 2024

GeorgeMac left a comment

Choose a reason for hiding this comment

markphelps commented Oct 16, 2024

erka Oct 16, 2024

Choose a reason for hiding this comment

markphelps Oct 16, 2024

Choose a reason for hiding this comment

erka Oct 16, 2024

Choose a reason for hiding this comment

markphelps commented Oct 16, 2024

markphelps commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading