feat(flagd): add new context enrichment appraoch for in-process provider #730
Conversation
aepfli
force-pushed
the
feat/contextValues
branch
from
fed3e36 to
fa7a42d
Compare
Signed-off-by: Simon Schrottner <[email protected]>
aepfli
force-pushed
the
feat/contextValues
branch
from
fa7a42d to
30a0194
Compare
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
|
/gemini review |
![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 introduces a context enrichment mechanism for the in-process provider, which is a valuable feature. The implementation is mostly on the right track, but there are a few important issues to address. There is a race condition in the in_process service when accessing contextValues, which needs to be fixed with a mutex. The ContextValues function should also return a copy of the map to prevent mutation of the service's internal state. Additionally, there are a couple of issues in the tests, including a race condition due to mutation of shared state in a parallel test, and an incomplete map comparison. Addressing these points will make the implementation more robust and reliable.
| WithContextEnricher(func(m map[string]any) *of.EvaluationContext { | ||
| m["test2"] = "test2" | ||
| context := of.NewTargetlessEvaluationContext(m) | ||
| return &context | ||
| }), |
There was a problem hiding this comment.
The ContextEnricher function in this test case mutates its input map m. Since the tests are marked with t.Parallel(), and the enrichment map is shared across test cases, this will lead to a race condition and unpredictable test failures. The enricher function should not modify its input; it should work on a copy if modifications are needed.
For example:
WithContextEnricher(func(m map[string]any) *of.EvaluationContext {
newMap := make(map[string]any, len(m)+1)
for k, v := range m {
newMap[k] = v
}
newMap["test2"] = "test2"
context := of.NewTargetlessEvaluationContext(newMap)
return &context
})| func (i *InProcess) ContextValues() map[string]any { | ||
| return i.contextValues | ||
| } |
There was a problem hiding this comment.
The contextValues field is accessed concurrently without synchronization, leading to a race condition. It's written in the sync goroutine (lines 115-117) and read by evaluation goroutines via this ContextValues method. Furthermore, this method returns a direct reference to the internal map, which allows callers to mutate the service's internal state.
To resolve this, you should introduce a sync.RWMutex to protect accesses to contextValues and return a copy of the map from this function. Remember to also protect the write operation on lines 115-117.
Example implementation:
func (i *InProcess) ContextValues() map[string]any {
i.mtx.RLock()
defer i.mtx.RUnlock()
if i.contextValues == nil {
return nil
}
// Return a copy to prevent mutation of internal state
contextValuesCopy := make(map[string]any, len(i.contextValues))
for k, v := range i.contextValues {
contextValuesCopy[k] = v
}
return contextValuesCopy
}| for k, v := range test.expectContextEnrichment { | ||
| if enriched[k] != v { | ||
| t.Errorf("incorrect context_enrichment attribute, expected %v, got %v", v, enriched[k]) | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop only checks that the keys from test.expectContextEnrichment exist in enriched with the correct values. It doesn't check if enriched has extra keys, or if some keys are missing. For a more robust check, you should compare the maps for equality, for example using reflect.DeepEqual. This will also make the test assertion more concise.
if !reflect.DeepEqual(test.expectContextEnrichment, enriched) {
t.Errorf("incorrect context_enrichment attribute, expected %v, got %v", test.expectContextEnrichment, enriched)
}
|
/gemini summary |
Summary of ChangesThis pull request enhances the in-process flagd provider by introducing a flexible context enrichment mechanism. It allows users to define custom functions to modify or extend the evaluation context dynamically, ensuring that flag evaluations can incorporate additional, derived, or external data. This is achieved through a new "ContextEnricher" configuration option and its integration via a "SyncContextHook", providing a powerful way to customize flag evaluation behavior. Highlights
Changelog
Activity
|
trying to add context value enrichment approach for in-process mode.
Long time no golang, so this might be a little bit of sub optimal approach open for feedback/insights