feat(flagd): Add code to sync context from Flagd for the InProcess resolver #515
Conversation
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
kylejuliandev
changed the title
|
/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 support for syncing context from Flagd for the in-process resolver, which is a great feature to align with other flagd providers. The changes are well-structured, decoupling the resolvers from the provider's event channel by using a callback mechanism. The introduction of FlagdProviderEvent and SyncMetadataHook is clean. The new functionality is also well-covered by unit tests.
I have a couple of suggestions to improve maintainability by reducing code duplication. One is in FlagdProvider where event handling logic is repeated, and the other is regarding the protobuf value conversion logic which is duplicated between InProcessResolver and RpcResolver.
| var context = EvaluationContext.Builder(); | ||
| foreach (var item in payload.SyncMetadata.AsDictionary()) | ||
| { | ||
| context.Set(item.Key, item.Value); | ||
| } | ||
| this._enrichedContext = context.Build(); |
There was a problem hiding this comment.
This block of code for building the EvaluationContext is duplicated in the ProviderReady case (lines 159-164). To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method or a local function.
| private static Value ExtractValue(Google.Protobuf.WellKnownTypes.Value value) | ||
| { | ||
| _mtx.WaitOne(); | ||
| _eventStreamRetryBackoff = Math.Min(_eventStreamRetryBackoff * 2, MaxEventStreamRetryBackoff); | ||
| if (connected) | ||
| switch (value.KindCase) | ||
| { | ||
| connected = false; | ||
| _eventChannel.Writer.TryWrite(new ProviderEventPayload { Type = ProviderEventTypes.ProviderError, ProviderName = _providerMetadata.Name }); | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.BoolValue: | ||
| return new Value(value.BoolValue); | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.NumberValue: | ||
| return new Value(value.NumberValue); | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.StringValue: | ||
| return new Value(value.StringValue); | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.StructValue: | ||
| { | ||
| var val = Structure.Builder(); | ||
| foreach (var item in value.StructValue.Fields) | ||
| { | ||
| val.Set(item.Key, ExtractValue(item.Value)); | ||
| } | ||
| return new Value(val.Build()); | ||
| } | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.ListValue: | ||
| return new Value(value.ListValue.Values.Select(ExtractValue)); | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.NullValue: | ||
| case Google.Protobuf.WellKnownTypes.Value.KindOneofCase.None: | ||
| break; | ||
| } | ||
| _mtx.ReleaseMutex(); | ||
| await Task.Delay(_eventStreamRetryBackoff * 1000).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private void HandleProviderChangeEvent() | ||
| { | ||
| _mtx.WaitOne(); | ||
| if (connected) | ||
| { | ||
| _eventChannel.Writer.TryWrite(new ProviderEventPayload { Type = ProviderEventTypes.ProviderConfigurationChanged, ProviderName = _providerMetadata.Name }); | ||
| } | ||
| _mtx.ReleaseMutex(); | ||
| return new Value(); | ||
| } |
There was a problem hiding this comment.
The ExtractValue method is very useful. However, similar logic for converting Google.Protobuf.WellKnownTypes.Value to OpenFeature.Model.Value exists in RpcResolver.cs (spread across ConvertToValue, ConvertObjectToValue, and ConvertToPrimitiveValue methods). To improve code reuse and maintainability, consider moving this conversion logic to a shared utility class that both InProcessResolver and RpcResolver can use.
|
@kylejuliandev Is this related to #478? |
I don't think so, that appears to be related to some environment variable changes in the flagd testbed |
4 participants
This PR
Initial PR to add support for syncing context state from Flagd for use by the in process resolver. This was discovered while I was adding support for the newer flagd-testbed Gherkin tests.
Related Issues
Not sure we have any related issues for this at the moment, but this should bring the dotnet flagd provider in line with the others.
Notes
Follow-up Tasks
How to test