Skip to content

feat: implement multiprovider #669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 70 commits into from
Apr 25, 2025
Merged

feat: implement multiprovider #669

merged 70 commits into from
Apr 25, 2025

Conversation

jblacker
Copy link
Contributor

@jblacker jblacker commented Apr 24, 2025 •
edited
Loading

This PR

Adds an implementation of multi-provider for combining multiple providers into a single provider.

Related Issues

Implements #599

Notes

I implemented this based on the JavaScript version of the OpenFeature SDK. More strategies can be added at any time. Additionally, I branched off of @bbland1 so some of their commits are also part of this branch.

This PR supersedes #668

Follow-up Tasks

  • Implement Hooks support (although the inner provider's hooks will still be triggered)
  • Implement proper eventing support
  • Finish implementing slog support
  • Write e2e tests

How to test

Create multiple FeatureProviders and pass them to the NewMultiProvider method along with a strategy and any options.
Verify the results are as expected based on the strategy selected.

bbland1 and others added 30 commits April 23, 2025 21:59
@bbland1 @jblacker
feat(multi-provider): set up multi-provider directory
c6249d4 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
feat: set up func to create new multiprovider & register passed provi…
446b543 
…ders

Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
feat: added Metadata Method to the multiprovider
1fae6f0 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
feat: added the based of the Init method that will process the inital…
aeb8bd3 
…ize of the providers

Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
moved providers into pkg directory
0605d94 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added Init method to handle any providers with intialization
51c33bc 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added the Shutdown method
1d7eed5 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added getter methods to the unexported fields
9ddd19b 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added tests to check unique name for providers passed
09a988f 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added test checking if error thrown for unique name if not unique
7df8cba 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added test to check for metadata, currently fails
dcbc5d7 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added test for getter methods of the struct provider entries
f06e745 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added test for MP Init method with mock providers that have Init methods
676bcee 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added shutdown test confirming method run in MP when called
4634789 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added the empty bases of the evaluations and hooks methods
af13736 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added test for init errors by a provider and reduced the mock provide…
fcc1f7c 
…r struct made

Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
removed memprovider since NewTestProvider uses it
dc539b9 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added metadata aggregate to the method and updated test to properly use
2b926df 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added ready and error state to the init method for the multi provider
b0f777d 
Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
added a func to determine which eval strategy to use and added it to …
89f1f8d 
…where it should be when strategies pushed

Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
creating the base of a base strategy similar to js version that can b…
10b8d74 
…e used to define custom strategies

Signed-off-by: bbland1 <[email protected]>
@bbland1 @jblacker
fleshing out the BaseStrategy to have the help funcs to be used in st…
c5f5f14 
…rategies similar to TS and passing that type into the provider

Signed-off-by: bbland1 <[email protected]>
@jblacker
feat: Define strategy interface & strategy name type
ecb7170 
Helper methods added

Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
feat: Add FirstSuccessStrategy Implementation
e0cc5ad 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
doc: Add README for multiprovider
af79893 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
feat: Configure strategy in provider initializer
542e5d2 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
feat: wire it all up
8c78f3e 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
refactor: use strategy's own name
aa2a0d1 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
fix: handle post-rebase conflicts
ed349d3 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
refactor: Rename types, reorganize package structure
ef2514c 
- Rename UniqueNameProvider -> NamedProvider: More accurate description of type as it's not enforced
- Updated signature of NewMultiProvider function: Using a map enforces unique names, use options pattern & config object for future extension
- Relocated NamedProvider to strategies as it's not needed externally
- Update shape of MultiProvider: Added logging, made all fields private, used map internally
- Moved aggregate-errors into errors package
- Added alias for ProviderMap shape & added helper functions
- Use pointers since we're passing stuff around and want to avoid unnecessary clones of objects & extra garbage collection
- Updated other various signatures throughout as needed
- Update tests & remove tests that no longer apply

Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
Copy link
Contributor Author

It looks like the test failures are all coming from different providers. What's the process for handling this situation?

@beeme1mr
Copy link
Member

It looks like the test failures are all coming from different providers. What's the process for handling this situation?

I'm going to rerun the test to see if it's flaky.

@toddbaert
Copy link
Member

I'm looking into the e2e failures, they might be unrelated to your change.

@beeme1mr
Copy link
Member

Hey @jblacker, I wasn't able to merge this today due to the failing tests. My theory is that your pr bumps the OpenFeature sdk to the version that added state management. I think some of the provider tests do not handle this properly. Unfortunately, I didn't have a chance to test this theory today.

@jblacker
Copy link
Contributor Author

Oh interesting. I would not have expected that different modules with their own go.mod & go.sum would allow running with a different dependency version. Not sure how that's possible, but I did dig through the logs to look into your theory and I think you're right.

go clean -testcache && go list -f '{{.Dir}}/...' -m | xargs -I{} go test -tags=e2e {}
go: downloading go.opentelemetry.io/otel v1.34.0
go: downloading github.com/open-feature/go-sdk v1.14.1

This is the third line called after make e2e is called. That's well before it gets to the multi-provider module, so it's running those flagd tests with v1.14.1, when the go.mod specifies 1.9.0. I saw that all other modules are set to v1.11.0. I can downgrade this module to keep things moving.

jblacker added 6 commits April 25, 2025 19:21
@jblacker
refactor: downgrade go-sdk
74f5ad1 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
chore: go mod tidy
3c3abc7 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
chore: fix makefile & rebuild mocks
8846c52 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
fix: repair code post downgrade
225a1bc 
- Replace openfeature's test provider from v1.14 with the `InMemoryProvider`
- Comment out ProviderFatalCode from `strategies.cleanErrorMessage`

Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
doc: Update documentation
3e092fc 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker
refactor: Simplify ProviderMap.Size() impl
71a9f29 
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker jblacker force-pushed the 599/implement-multiprovider branch from c412ed2 to 71a9f29 Compare April 25, 2025 23:21
@beeme1mr
Copy link
Member

It looks like that helped but now there's an issue with the multi-provider e2e testing. I'm surprised that this change affects other packages. We'll need to invest time into improving our tooling.

@jblacker
Copy link
Contributor Author

jblacker commented Apr 25, 2025 •
edited
Loading

Forgot to sign off and had to clean it up. And fixed that last broken test. We should be good to go now. I ended up downgrading the SDK to the highest version another module was using, which was v1.13.1

@beeme1mr beeme1mr merged commit d35f4d6 into open-feature:main Apr 25, 2025
5 checks passed
@github-actions github-actions bot mentioned this pull request Apr 25, 2025
Merged
@beeme1mr
Copy link
Member

Thanks @jblacker! Would you be interested in joining the org? There's no obligation but it's the first step in the contributor ladder and it makes it easier to collaborate on GitHub.

@jblacker
Copy link
Contributor Author

Yeah, definitely! Let me know what I need to do to get started

@jblacker jblacker deleted the 599/implement-multiprovider branch April 26, 2025 00:09
@beeme1mr beeme1mr mentioned this pull request Apr 26, 2025
Merged
@beeme1mr beeme1mr linked an issue Apr 26, 2025 that may be closed by this pull request
Implement Multi-Provider #599
Open
@toddbaert toddbaert mentioned this pull request Apr 26, 2025
Open
@jblacker jblacker mentioned this pull request May 15, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbland1 bbland1 bbland1 left review comments

@beeme1mr beeme1mr beeme1mr approved these changes

@jonathannorris jonathannorris Awaiting requested review from jonathannorris

@thomaspoignant thomaspoignant Awaiting requested review from thomaspoignant

@toddbaert toddbaert Awaiting requested review from toddbaert

Assignees

@beeme1mr beeme1mr

@thomaspoignant thomaspoignant

@toddbaert toddbaert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Multi-Provider
9 participants
@jblacker @beeme1mr @toddbaert @bbland1 @c4milo @bacherfl @kinyoklion @Kavindu-Dodan @thomaspoignant