Skip to content
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

feat: Update in-process resolver to support flag metadata #1102 #1122

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

import static dev.openfeature.contrib.providers.flagd.resolver.process.model.FeatureFlag.EMPTY_TARGETING_STRING;

import java.util.Map;
import java.util.function.Consumer;
import java.util.function.Supplier;

import dev.openfeature.contrib.providers.flagd.FlagdOptions;
import dev.openfeature.contrib.providers.flagd.resolver.Resolver;
import dev.openfeature.contrib.providers.flagd.resolver.common.ConnectionEvent;
Expand Down Expand Up @@ -37,8 +41,9 @@ public class InProcessResolver implements Resolver {
private final Consumer<ConnectionEvent> onConnectionEvent;
private final Operator operator;
private final long deadline;
private final ImmutableMetadata metadata;
private final ImmutableMetadata fallBackMetadata;
private final Supplier<Boolean> connectedSupplier;
private final String scope;

/**
* Resolves flag values using https://buf.build/open-feature/flagd/docs/main:flagd.sync.v1. Flags
Expand All @@ -48,20 +53,22 @@ public class InProcessResolver implements Resolver {
* @param connectedSupplier lambda providing current connection status from caller
* @param onConnectionEvent lambda which handles changes in the connection/stream
*/
public InProcessResolver(
FlagdOptions options,
final Supplier<Boolean> connectedSupplier,
Consumer<ConnectionEvent> onConnectionEvent) {
public InProcessResolver(FlagdOptions options, final Supplier<Boolean> connectedSupplier,
Consumer<ConnectionEvent> onConnectionEvent) {
this.flagStore = new FlagStore(getConnector(options));
this.deadline = options.getDeadline();
this.onConnectionEvent = onConnectionEvent;
this.operator = new Operator();
this.connectedSupplier = connectedSupplier;
this.metadata = options.getSelector() == null
? null
: ImmutableMetadata.builder()
.addString("scope", options.getSelector())
.build();
if (options.getSelector() == null) {
this.scope = null;
this.fallBackMetadata = null;
} else {
this.scope = options.getSelector();
this.fallBackMetadata = ImmutableMetadata.builder()
.addString("scope", this.scope)
.build();
}
}

/** Initialize in-process resolver. */
Expand Down Expand Up @@ -109,8 +116,14 @@ public void shutdown() throws InterruptedException {
onConnectionEvent.accept(new ConnectionEvent(false));
}

/** Resolve a boolean flag. */
public ProviderEvaluation<Boolean> booleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) {
/**
* Resolve a boolean flag.
*/
public ProviderEvaluation<Boolean> booleanEvaluation(
String key,
Boolean defaultValue,
EvaluationContext ctx
) {
return resolve(Boolean.class, key, ctx);
}

Expand Down Expand Up @@ -161,6 +174,7 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC
return ProviderEvaluation.<T>builder()
.errorMessage("flag: " + key + " not found")
.errorCode(ErrorCode.FLAG_NOT_FOUND)
.flagMetadata(fallBackMetadata)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a corollary to the inheritance of the flag set metadata, for error cases we should make a "best effort" to return whatever metadata we can - so here we should still return the flag set metadata. We've added this to the spec: https://github.com/open-feature/flagd/pull/1505/files#diff-28ba4b4041b6c39c4b9deb542276312bebdd08f45b39b29d860849d9e8b81a6eR297

The simplest way might be to store the flag set metadata on the flag store and expose it there.

This will greatly help in telemetry and debugging, since we'll have some idea of the flag set, it's version, etc in error reporting and metrics.

.build();
}

Expand All @@ -169,6 +183,7 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC
return ProviderEvaluation.<T>builder()
.errorMessage("flag: " + key + " is disabled")
.errorCode(ErrorCode.FLAG_NOT_FOUND)
.flagMetadata(getFlagMetadata(flag))
.build();
}

Expand Down Expand Up @@ -215,13 +230,51 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC
throw new TypeMismatchError(message);
}

final ProviderEvaluation.ProviderEvaluationBuilder<T> evaluationBuilder = ProviderEvaluation.<T>builder()
return ProviderEvaluation.<T>builder()
.value((T) value)
.variant(resolvedVariant)
.reason(reason);
.reason(reason)
.flagMetadata(getFlagMetadata(flag))
.build();
}

private ImmutableMetadata getFlagMetadata(FeatureFlag flag) {
if (flag == null) {
return fallBackMetadata;
}

ImmutableMetadata.ImmutableMetadataBuilder metadataBuilder = ImmutableMetadata.builder();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have a mergeMetdata or addMetadata method in the ImmutableMetadataBuilder

if (scope != null) {
metadataBuilder.addString("scope", scope);
}

for (Map.Entry<String, Object> entry : flag.getMetadata().entrySet()) {
Object value = entry.getValue();
if (value instanceof Number) {
if (value instanceof Long) {
metadataBuilder.addLong(entry.getKey(), (Long) value);
continue;
} else if (value instanceof Double) {
metadataBuilder.addDouble(entry.getKey(), (Double) value);
continue;
} else if (value instanceof Integer) {
metadataBuilder.addInteger(entry.getKey(), (Integer) value);
continue;
} else if (value instanceof Float) {
metadataBuilder.addFloat(entry.getKey(), (Float) value);
continue;
}
} else if (value instanceof Boolean) {
metadataBuilder.addBoolean(entry.getKey(), (Boolean) value);
continue;
} else if (value instanceof String) {
metadataBuilder.addString(entry.getKey(), (String) value);
continue;
}
throw new IllegalArgumentException("The type of the Metadata entry with key " + entry.getKey()
+ " and value " + entry.getValue() + " is not supported");
}

return this.metadata == null
? evaluationBuilder.build()
: evaluationBuilder.flagMetadata(this.metadata).build();
return metadataBuilder.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,21 @@ public class FeatureFlag {
private final String defaultVariant;
private final Map<String, Object> variants;
private final String targeting;
private final Map<String, Object> metadata;

/** Construct a flagd feature flag. */
@JsonCreator
public FeatureFlag(
@JsonProperty("state") String state,
@JsonProperty("defaultVariant") String defaultVariant,
@JsonProperty("variants") Map<String, Object> variants,
@JsonProperty("targeting") @JsonDeserialize(using = StringSerializer.class) String targeting) {
public FeatureFlag(@JsonProperty("state") String state,
@JsonProperty("defaultVariant") String defaultVariant,
@JsonProperty("variants") Map<String, Object> variants,
@JsonProperty("targeting") @JsonDeserialize(using = StringSerializer.class) String targeting,
@JsonProperty("metadata") Map<String, Object> metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Currently, we do need to provide an empty map everywhere to be backward compatible. Could we also omit this parameter and make it optional? more a question out of curiosity ;)

) {
this.state = state;
this.defaultVariant = defaultVariant;
this.variants = variants;
this.targeting = targeting;
this.metadata = metadata;
}

/** Get targeting rule of the flag. */
Expand Down
Loading
Loading