feat!: flagSetMetadata in OFREP/ResolveAll, core refactors #1540
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,13 +64,13 @@ func WithEvaluator(name string, evalFunc func(interface{}, interface{}) interfac | |
|
|
||
| // JSON evaluator | ||
| type JSON struct { | ||
| store *store.State | ||
| Logger *logger.Logger | ||
| jsonEvalTracer trace.Tracer | ||
| Resolver | ||
| } | ||
|
|
||
| func NewJSON(logger *logger.Logger, s *store.State, opts ...JSONEvaluatorOption) *JSON { | ||
| logger = logger.WithFields( | ||
| zap.String("component", "evaluator"), | ||
| zap.String("evaluator", "json"), | ||
|
|
@@ -107,9 +107,9 @@ func (je *JSON) SetState(payload sync.DataSync) (map[string]interface{}, bool, e | |
| trace.WithAttributes(attribute.String("feature_flag.sync_type", payload.Type.String()))) | ||
| defer span.End() | ||
|
|
||
| var definition Definition | ||
|
|
||
| err := configToFlagDefinition(je.Logger, payload.FlagData, &definition) | ||
| if err != nil { | ||
| span.SetStatus(codes.Error, "flagSync error") | ||
| span.RecordError(err) | ||
|
|
@@ -119,15 +119,16 @@ func (je *JSON) SetState(payload sync.DataSync) (map[string]interface{}, bool, e | |
| var events map[string]interface{} | ||
| var reSync bool | ||
|
|
||
| // TODO: We do not handle metadata in ADD/UPDATE operations. These are only relevant for grpc sync implementations. | ||
| switch payload.Type { | ||
| case sync.ALL: | ||
| events, reSync = je.store.Merge(je.Logger, payload.Source, payload.Selector, definition.Flags, definition.Metadata) | ||
| case sync.ADD: | ||
| events = je.store.Add(je.Logger, payload.Source, payload.Selector, definition.Flags) | ||
| case sync.UPDATE: | ||
| events = je.store.Update(je.Logger, payload.Source, payload.Selector, definition.Flags) | ||
| case sync.DELETE: | ||
| events = je.store.DeleteFlags(je.Logger, payload.Source, definition.Flags) | ||
|
Comment on lines
+122
to
+131
There was a problem hiding this comment. ADD/UPDATE are only relevant to hypothetical gRPC server implementations, and were a premature optimization to allow gRPC servers to tell flagd "just add these flags, or just change this flag" instead of sending a whole payload. I have never seen this implemented, and it's not relevant to any of the other sync types. I think we should cease to enhance them and deprecate them in the protobuf. |
||
| default: | ||
| return nil, false, fmt.Errorf("unsupported sync type: %d", payload.Type) | ||
| } | ||
|
|
@@ -156,14 +157,16 @@ func NewResolver(store store.IStore, logger *logger.Logger, jsonEvalTracer trace | |
| return Resolver{store: store, Logger: logger, tracer: jsonEvalTracer} | ||
| } | ||
|
|
||
| func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) ([]AnyValue, | ||
|
There was a problem hiding this comment. [Q] is this the function that relates to this proto command (from evaluation.proto): There was a problem hiding this comment. Yes, this is the inner "domain layer" function satisfying that rpc. More directly, this function is the actual rpc endpoint. |
||
| model.Metadata, error, | ||
| ) { | ||
| _, span := je.tracer.Start(ctx, "resolveAll") | ||
| defer span.End() | ||
|
|
||
| var err error | ||
| allFlags, flagSetMetadata, err := je.store.GetAll(ctx) | ||
| if err != nil { | ||
| return nil, flagSetMetadata, fmt.Errorf("error retreiving flags from the store: %w", err) | ||
| } | ||
|
|
||
| values := []AnyValue{} | ||
|
|
@@ -195,7 +198,7 @@ func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context | |
| values = append(values, NewAnyValue(value, variant, reason, flagKey, metadata, err)) | ||
| } | ||
|
|
||
| return values, flagSetMetadata, nil | ||
| } | ||
|
|
||
| func (je *Resolver) ResolveBooleanValue( | ||
|
|
@@ -312,9 +315,7 @@ func resolve[T constraints](ctx context.Context, reqID string, key string, conte | |
| func (je *Resolver) evaluateVariant(ctx context.Context, reqID string, flagKey string, evalCtx map[string]any) ( | ||
| variant string, variants map[string]interface{}, reason string, metadata map[string]interface{}, err error, | ||
| ) { | ||
| flag, metadata, ok := je.store.Get(ctx, flagKey) | ||
| if !ok { | ||
| // flag not found | ||
| je.Logger.DebugWithID(reqID, fmt.Sprintf("requested flag could not be found: %s", flagKey)) | ||
|
|
@@ -447,8 +448,8 @@ func loadAndCompileSchema(log *logger.Logger) *gojsonschema.Schema { | |
| return compiledSchema | ||
| } | ||
|
|
||
| // configToFlagDefinition convert string configurations to flags and store them to pointer newFlags | ||
| func configToFlagDefinition(log *logger.Logger, config string, definition *Definition) error { | ||
| compiledSchema := loadAndCompileSchema(log) | ||
|
|
||
| flagStringLoader := gojsonschema.NewStringLoader(config) | ||
|
|
@@ -467,33 +468,16 @@ func configToFlags(log *logger.Logger, config string, newFlags *Flags) error { | |
| return fmt.Errorf("transposing evaluators: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal([]byte(transposedConfig), &definition) | ||
| if err != nil { | ||
| return fmt.Errorf("unmarshalling provided configurations: %w", err) | ||
| } | ||
|
|
||
| return validateDefaultVariants(definition) | ||
| } | ||
|
|
||
| // validateDefaultVariants returns an error if any of the default variants aren't valid | ||
| func validateDefaultVariants(flags *Definition) error { | ||
| for name, flag := range flags.Flags { | ||
| if _, ok := flag.Variants[flag.DefaultVariant]; !ok { | ||
| return fmt.Errorf( | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[Q] did you miss this TODO or should it rather be a normal comment? reading your comment below the TODO would be to deprecate these cases (ADD/UPDATE/DELETE) for sync impls, or did I misunderstand it?
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.
You are understanding correctly. I left it as a TODO because I wanted to open an issue to discuss the deprecation of these, which I still intend to do.