-
Notifications
You must be signed in to change notification settings - Fork 50
feat(multi): add tracking, functional options #446
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.
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
base: main
Are you sure you want to change the base?
Conversation
Changes: - Add WithProvider option for registering named providers with optional hooks - Remove ProviderMap type in favor of variadic functional options - Change NamedProvider from struct to interface - Update NewProvider signature to take strategy first, then options - Add Track method support to MultiProvider for tracking events - Update tests, documentation and examples to reflect new API Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
- remove inboundEvents channel in favor of direct pipe channel - fix race condition in updateProviderStateFromEvent with lock - optimize Shutdown to only spawn goroutines for StateHandler providers - ensure outboundEvents channel is closed after all workers complete Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Summary of ChangesHello @toddbaert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces substantial improvements to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@erka I'm so sorry, I was reviewing and accidentally pushed your branch with no changes. When that happen GH closes the PR automatically :( I added all your commits back, but I can't put them back on your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and beneficial refactoring of the multi-provider. The API is improved by introducing functional options for provider configuration, which is more idiomatic in Go. The internal implementation is also more robust, with fixes for potential race conditions and better event handling logic. Additionally, it adds support for the Tracker interface. My review includes a few suggestions to improve logging clarity in the shutdown process, fix a potentially flaky test, and correct a syntax error in the README documentation. Overall, this is a great improvement to the package.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
- Coverage 83.23% 83.15% -0.09%
==========================================
Files 27 27
Lines 2034 2101 +67
==========================================
+ Hits 1693 1747 +54
- Misses 299 305 +6
- Partials 42 49 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant and valuable improvements to the multi-provider implementation. The transition to functional options for provider registration (WithProvider) and the change of NamedProvider from a struct to an interface make the API more flexible and idiomatic. The addition of the Track method and context-aware Init and Shutdown functions are also great enhancements. The code is well-structured, and the updates to tests and documentation are thorough.
I have found one critical race condition in the new ShutdownWithContext function that needs to be addressed. I've also noted a minor inconsistency in context usage for logging. Apart from these points, the changes are excellent.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is an excellent pull request that significantly refactors the multi-provider API to be more idiomatic and robust. The move to functional options with WithProvider is a great improvement over the previous ProviderMap approach, making the API cleaner and preserving provider order. The introduction of Track support, along with context-aware initialization and shutdown, adds valuable functionality and improves the provider's lifecycle management. The code quality is high, and the test coverage is thorough, including the new features. I have one suggestion regarding context propagation in logging functions to enhance traceability, but overall, this is a very strong contribution.
| slog.String(MetadataProviderName, e.providerName), | ||
| slog.String(MetadataProviderType, e.ProviderName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these log attrs supposed to have the same values? As in, should MetadataProviderType have value e.ProviderName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there’s a naming issue here - I’ve struggled with it too. providerName is a user-defined name in the WithProvider option in namedEvent, while ProviderName is the defined name in of.Event.
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
openfeature/multi/multiprovider.go
Outdated
| // Name returns the unique name assigned to the provider. | ||
| Name() string | ||
| // unwrap returns the underlying [of.FeatureProvider] instance. | ||
| unwrap() of.FeatureProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this interface is exported, then unwrap should also be exported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t want to export it. I’ve reworked it, but it might look a bit messy. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my question is: would it make more sense to unexport the NamedProvider interface and export instead the namedProvider struct, so that the interface is used internally, while the struct is exposed? What we've got now is also fine, but I prefer to minimize exporting interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t have a strong opinion on this. Let’s see what the other members think.
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Replay of #444 (these contributions are not mine, but @erka 's)
This PR