feat: implement isolated API instances#492
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
- Coverage 84.07% 83.93% -0.15%
==========================================
Files 27 27
Lines 2079 2147 +68
==========================================
+ Hits 1748 1802 +54
- Misses 292 299 +7
- Partials 39 46 +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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements isolated API instances in the OpenFeature Go SDK, fulfilling requirements 1.8.1, 1.8.2, and 1.8.4. Key changes include the addition of a NewAPI factory function and a global registry to manage provider-to-instance bindings, preventing a single provider from being used across multiple instances. Review feedback highlights a critical risk of runtime panics when using interfaces as map keys and suggests a more efficient locking mechanism for unbinding providers. Additionally, it is recommended to update the documentation to emphasize the importance of calling Shutdown on isolated instances to prevent memory leaks.
marcozabel
force-pushed
the
feat/isolated-api-instances
branch
3 times, most recently
from
f9a372a to
51f73a0
Compare
| } | ||
|
|
||
| // Requirement 1.8.1 (independence): State set on the singleton MUST NOT affect isolated instances. | ||
| func TestIsolatedAPI_SingetonDoesNotAffectInstance(t *testing.T) { |
There was a problem hiding this comment.
| func TestIsolatedAPI_SingetonDoesNotAffectInstance(t *testing.T) { | |
| func TestIsolatedAPI_SingletonDoesNotAffectInstance(t *testing.T) { |
| // provider resources and free the provider bindings held by the global registry. | ||
| // | ||
| // Use [IEvaluation.GetClient] or [IEvaluation.GetNamedClient] to create clients bound to this instance. | ||
| func NewAPI() IEvaluation { |
There was a problem hiding this comment.
I want to avoid returning an interface here. I suggest we export [evaluationAPI] and return this concrete type instead of the interface.
| // referenced by this API instance. Must be called with api.mu held. | ||
| // All comparisons use pointer identity (via providerBindingKey) to avoid panics from unhashable | ||
| // FeatureProvider implementations that contain maps or slices. | ||
| func (api *evaluationAPI) maybeUnbindOldProvider(oldProvider FeatureProvider) { |
There was a problem hiding this comment.
The word "maybe" seems odd in this method name. Perhaps unbindIfUnreferenced?
| // Callers MUST invoke [IEvaluation.Shutdown] when the instance is no longer needed to release | ||
| // provider resources and free the provider bindings held by the global registry. | ||
| // | ||
| // Use [IEvaluation.GetClient] or [IEvaluation.GetNamedClient] to create clients bound to this instance. |
There was a problem hiding this comment.
A couple of things: section 1.8 is experimental in the spec, so worth noting that in the doc comment (e.g. // Experimental: ... at the top). Also, the spec recommends (1.8.3) that the factory live in a distinct module/package for discoverability; in Go this would typically be a sub-package like openfeature/isolated. WDYT?
There was a problem hiding this comment.
Thanks for adding the experimental note! Just bumping the second half of this; any thoughts on moving NewAPI() to a sub-package like openfeature/isolated? Per 1.8.3 the factory should be in a distinct module/package so it's intentionally less discoverable than the singleton. WDYT?
|
|
||
| // Requirement 1.8.4: A provider instance SHOULD NOT be bound to more than one API instance at a time. | ||
| func TestRequirement_1_8_4_CrossInstanceBinding(t *testing.T) { | ||
| t.Cleanup(func() { |
There was a problem hiding this comment.
The provider isolation tests are solid and I like that both directions are covered. Worth adding similar tests for hooks, evaluation context, and events between isolated instances to match the coverage in other SDKs.
|
I don't see that we implement The developer's experience isn't great after all in my opinion. If someone tries a new isolated API they will have to rewrite the code. It's not just plug and play @toddbaert |
marcozabel
force-pushed
the
feat/isolated-api-instances
branch
2 times, most recently
from
9d61b22 to
4003d8b
Compare
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
marcozabel
force-pushed
the
feat/isolated-api-instances
branch
from
ac5a289 to
1e8704c
Compare
4 participants
This PR
This PR introduces support for creating isolated OpenFeature API instances, each with their own providers, hooks, context, and event handling - enabling multi-tenant or side-by-side usage without shared global state.
Related Issues
#486
Notes
Follow-up Tasks
How to test