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.

Sign up for GitHub

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: report prerequisite relations in AllFlagState #19

Merged
merged 7 commits into from
Oct 23, 2024
Merged

feat: report prerequisite relations in AllFlagState #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c37c81
feat: support prerequisite relationships in AllFlagsState
cwaldren-ld Oct 22, 2024
598adcb
updating equality comparison of FlagState
cwaldren-ld Oct 22, 2024
3937e3f
more tests
cwaldren-ld Oct 22, 2024
7402975
more tests, contract tests
cwaldren-ld Oct 22, 2024
9f5f76f
don't allocate prereq list by default
cwaldren-ld Oct 23, 2024
f64bfa8
empty doc tags
cwaldren-ld Oct 23, 2024
4c300ca
make prerequisites an IReadOnlyList, and add a long comment explainin…
cwaldren-ld Oct 23, 2024
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
3 changes: 2 additions & 1 deletion pkgs/sdk/server/contract-tests/TestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public class Webapp
"tags",
"inline-context",
"anonymous-redaction",
"evaluation-hooks"
"evaluation-hooks",
"client-prereq-events"
};

public readonly Handler Handler;
Expand Down
112 changes: 92 additions & 20 deletions pkgs/sdk/server/src/FeatureFlagsState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ public FeatureFlagsState Build()
/// </summary>
/// <param name="valid">true if valid, false if invalid (default is valid)</param>
/// <returns>the same builder</returns>
[Obsolete("Unused, construct a FeatureFlagState with valid/invalid state directly")]
public FeatureFlagsStateBuilder Valid(bool valid)
{
_valid = valid;
Expand All @@ -167,8 +168,21 @@ public FeatureFlagsStateBuilder Valid(bool valid)
/// </summary>
/// <param name="flagKey">the flag key</param>
/// <param name="result">the evaluation result</param>
/// <returns></returns>
/// <returns>the same builder</returns>
public FeatureFlagsStateBuilder AddFlag(string flagKey, EvaluationDetail<LdValue> result)
{
return AddFlag(flagKey, result, new List<string>());
}


/// <summary>
/// Adds the result of a flag evaluation, including direct prerequisites.
/// </summary>
/// <param name="flagKey">the flag key</param>
/// <param name="result">the evaluation result</param>
/// <param name="prerequisites">the direct prerequisites evaluated for this flag</param>
/// <returns>the same builder</returns>
public FeatureFlagsStateBuilder AddFlag(string flagKey, EvaluationDetail<LdValue> result, List<string> prerequisites)
{
return AddFlag(flagKey,
result.Value,
Expand All @@ -177,13 +191,14 @@ public FeatureFlagsStateBuilder AddFlag(string flagKey, EvaluationDetail<LdValue
0,
false,
false,
null);
null,
prerequisites);
}

// This method is defined with internal scope because metadata fields like trackEvents aren't
// relevant to the main external use case for the builder (testing server-side code)
internal FeatureFlagsStateBuilder AddFlag(string flagKey, LdValue value, int? variationIndex, EvaluationReason reason,
int flagVersion, bool flagTrackEvents, bool trackReason, UnixMillisecondTime? flagDebugEventsUntilDate)
int flagVersion, bool flagTrackEvents, bool trackReason, UnixMillisecondTime? flagDebugEventsUntilDate, List<string> prerequisites)
{
bool flagIsTracked = flagTrackEvents || flagDebugEventsUntilDate != null;
var flag = new FlagState
Expand All @@ -194,14 +209,15 @@ internal FeatureFlagsStateBuilder AddFlag(string flagKey, LdValue value, int? va
Reason = trackReason || (_withReasons && (!_detailsOnlyIfTracked || flagIsTracked)) ? reason : (EvaluationReason?)null,
DebugEventsUntilDate = flagDebugEventsUntilDate,
TrackEvents = flagTrackEvents,
TrackReason = trackReason
TrackReason = trackReason,
Prerequisites = prerequisites
};
_flags[flagKey] = flag;
return this;
}
}

internal struct FlagState
internal struct FlagState : IEquatable<FlagState>
Copy link
Member

Choose a reason for hiding this comment

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

Non-nullable prerequisites feel a bit strange to me.

Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair. Is that from a "what a library consumer would expect from a iterate-able thing in C#" kind of way, or a "how structs work" way?

Copy link
Member

Choose a reason for hiding this comment

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

So, it feels strange in the language to me. We have null-state analysis, so we should be able to safely mark things null-able and use them. https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#null-state-analysis

From a struct perspective, ideally structs only contain other value types, but that isn't always possible. Next is for structs to only contain immutable reference types in addition to value types. Least favorable is this mix, but also not always avoidable. A struct with mostly not references, with value semantics, is better than a class here probably. But it doesn't behave like a struct. In that if you alias it and mutate the list, then both lists mutate.

Copy link
Member

Choose a reason for hiding this comment

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

If we make an immutable list, and then assign the entire list, then it at least makes it more toward the normal side. It could also be exposed as an IReadOnlyList<T>.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is blocking though, as it isn't part of the external API.

{
internal LdValue Value { get; set; }
internal int? Variation { get; set; }
Expand All @@ -211,24 +227,38 @@ internal struct FlagState
internal UnixMillisecondTime? DebugEventsUntilDate { get; set; }
internal EvaluationReason? Reason { get; set; }

public override bool Equals(object other)
internal IReadOnlyList<string> Prerequisites { get; set; }


public bool Equals(FlagState o)
{
if (other is FlagState o)
{
return Variation == o.Variation &&
Version == o.Version &&
TrackEvents == o.TrackEvents &&
TrackReason == o.TrackReason &&
DebugEventsUntilDate.Equals(o.DebugEventsUntilDate) &&
Object.Equals(Reason, o.Reason);
}
return false;
return Variation == o.Variation &&
Version == o.Version &&
TrackEvents == o.TrackEvents &&
TrackReason == o.TrackReason &&
DebugEventsUntilDate.Equals(o.DebugEventsUntilDate) &&
Object.Equals(Reason, o.Reason) &&
Prerequisites.SequenceEqual(o.Prerequisites);
}
public override bool Equals(object obj)
{
return obj is FlagState other && Equals(other);
}

public static bool operator ==(FlagState lhs, FlagState rhs)
{
return lhs.Equals(rhs);
}

public static bool operator !=(FlagState lhs, FlagState rhs)
{
return !(lhs == rhs);
}

public override int GetHashCode()
{
return new HashCodeBuilder().With(Variation).With(Version).With(TrackEvents).With(TrackReason).
With(DebugEventsUntilDate).With(Reason).Value;
return new HashCodeBuilder().With(Variation).With(Version).With(TrackEvents).With(TrackReason)
.With(DebugEventsUntilDate).With(Reason).With(Prerequisites).Value;
}
}

Expand Down Expand Up @@ -271,6 +301,14 @@ public override void Write(Utf8JsonWriter w, FeatureFlagsState state, JsonSerial
w.WritePropertyName("reason");
EvaluationReasonConverter.WriteJsonValue(meta.Reason.Value, w);
}
if (meta.Prerequisites.Count > 0) {
w.WriteStartArray("prerequisites");
foreach (var p in meta.Prerequisites)
{
w.WriteStringValue(p);
}
w.WriteEndArray();
}
w.WriteEndObject();
}
w.WriteEndObject();
Expand All @@ -282,6 +320,7 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon
{
var valid = true;
var flags = new Dictionary<string, FlagState>();

for (var topLevelObj = RequireObject(ref reader); topLevelObj.Next(ref reader);)
{
var key = topLevelObj.Name;
Expand All @@ -295,7 +334,15 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon
for (var flagsObj = RequireObject(ref reader); flagsObj.Next(ref reader);)
{
var subKey = flagsObj.Name;
var flag = flags.ContainsKey(subKey) ? flags[subKey] : new FlagState();
Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Slightly awkward: have to new the prerequisite list here in the deserializer, and also on line 375. I'd rather just have this in the constructor so it's never a question, but looks like structs in our language version can't define constructors.

Another option is to make a null list be a possibility. Logically that'd be fine, but.. I'd rather the consumers never need to question if it's null or not. I could be convinced otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the constructor idea or something similar. Faced a similar issue in Java wrt to nullity, but luckily the array constructor with capacity 0 uses a static instance under the hood for performance.

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically we should have struct constructors, but not parameter-less constructors. Ideally we would be creating a fully populated readonly struct instead of poking stuff into it. (containing an immutable list of prerequisites, or null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the way we are deserializing JSON, we'd need to have some kind of builder pattern (or temp variables) to achieve that (creating the readonly struct in one go.)

Copy link
Member

Choose a reason for hiding this comment

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

If the temp variables are references, then they aren't really temps, the struct holds a reference. Though it maybe it problematic to adjust. It isn't at an assignment level though. Assign to a shared list and then assign to the populated list.


var flag = flags.ContainsKey(subKey)
? flags[subKey]
: new FlagState
{
// Most flags have no prerequisites, don't allocate capacity unless we need to.
Prerequisites = new List<string>(0)
kinyoklion marked this conversation as resolved.
Show resolved Hide resolved
};

for (var metaObj = RequireObject(ref reader); metaObj.Next(ref reader);)
{
switch (metaObj.Name)
Expand All @@ -318,14 +365,39 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon
flag.Reason = reader.TokenType == JsonTokenType.Null ? (EvaluationReason?)null :
EvaluationReasonConverter.ReadJsonValue(ref reader);
break;
case "prerequisites":
// Note: there is an assumption in this code that a given flag key could already
// have been seen before: specifically in the "values" section of the data
// (where it's a simple map of flag key -> evaluated value), but *also* if we
// have duplicate flag keys under the $flagState key.
//
// The first case is expected, but the second is not. LaunchDarkly SaaS / SDKs
// should never generate JSON that has duplicate keys. If this did happen,
// we don't want to 'merge' prerequisites in an arbitrary order: it's important
// that they remain the order they were serialized originally.
//
// Therefore, the behavior here is that the last seen value for a key will 'win'
// and overwrite any previous value.
var prereqList = new List<string>();
for (var prereqs = RequireArray(ref reader); prereqs.Next(ref reader);)
{
prereqList.Add(reader.GetString());

}
flag.Prerequisites = prereqList;
break;
}
}
flags[subKey] = flag;
}
break;

default:
var flagForValue = flags.ContainsKey(key) ? flags[key] : new FlagState();
var flagForValue = flags.ContainsKey(key) ? flags[key] : new FlagState
{
// Most flags have no prerequisites, don't allocate capacity unless we need to.
Prerequisites = new List<string>(0)
};
flagForValue.Value = LdValueConverter.ReadJsonValue(ref reader);
flags[key] = flagForValue;
break;
Expand Down
6 changes: 3 additions & 3 deletions pkgs/sdk/server/src/Internal/Evaluation/EvaluatorTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ internal EvalResult(EvaluationDetail<LdValue> result, IList<PrerequisiteEvalReco
internal struct PrerequisiteEvalRecord
{
internal readonly FeatureFlag PrerequisiteFlag;
internal readonly string PrerequisiteOfFlagKey;
internal readonly string FlagKey;
internal readonly EvaluationDetail<LdValue> Result;

internal PrerequisiteEvalRecord(FeatureFlag prerequisiteFlag, string prerequisiteOfFlagKey,
internal PrerequisiteEvalRecord(FeatureFlag prerequisiteFlag, string flagKey,
EvaluationDetail<LdValue> result)
{
PrerequisiteFlag = prerequisiteFlag;
PrerequisiteOfFlagKey = prerequisiteOfFlagKey;
FlagKey = flagKey;
Result = result;
}
}
Expand Down
17 changes: 11 additions & 6 deletions pkgs/sdk/server/src/LdClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Cryptography;
using LaunchDarkly.Logging;
Expand Down Expand Up @@ -371,8 +372,7 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[

var builder = new FeatureFlagsStateBuilder(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These weren't used at all in this function. They are used internal to the FeatureFlagsStateBuilder, presumably they didn't get cleaned up when that was introduced.

var clientSideOnly = FlagsStateOption.HasOption(options, FlagsStateOption.ClientSideOnly);
var withReasons = FlagsStateOption.HasOption(options, FlagsStateOption.WithReasons);
var detailsOnlyIfTracked = FlagsStateOption.HasOption(options, FlagsStateOption.DetailsOnlyForTrackedFlags);

KeyedItems<ItemDescriptor> flags;
try
{
Expand All @@ -397,6 +397,11 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[
{
EvaluatorTypes.EvalResult result = _evaluator.Evaluate(flag, context);
bool inExperiment = EventFactory.IsExperiment(flag, result.Result.Reason);

var directPrerequisites = result.PrerequisiteEvals.Where(
e => e.FlagKey == flag.Key)
.Select(p => p.PrerequisiteFlag.Key).ToList();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converting to a list here, rather than leaving as an IEnumerable, because the intent is that order is preserved.


builder.AddFlag(
flag.Key,
result.Result.Value,
Expand All @@ -405,16 +410,16 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[
flag.Version,
flag.TrackEvents || inExperiment,
inExperiment,
flag.DebugEventsUntilDate
);
flag.DebugEventsUntilDate,
directPrerequisites);
}
catch (Exception e)
{
LogHelpers.LogException(_evalLog,
string.Format("Exception caught for feature flag \"{0}\" when evaluating all flags", flag.Key),
e);
EvaluationReason reason = EvaluationReason.ErrorReason(EvaluationErrorKind.Exception);
builder.AddFlag(flag.Key, new EvaluationDetail<LdValue>(LdValue.Null, null, reason));
builder.AddFlag(flag.Key, new EvaluationDetail<LdValue>(LdValue.Null, null, reason), new List<string>());
}
}
return builder.Build();
Expand Down Expand Up @@ -477,7 +482,7 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[
foreach (var prereqEvent in evalResult.PrerequisiteEvals)
{
_eventProcessor.RecordEvaluationEvent(eventFactory.NewPrerequisiteEvaluationEvent(
prereqEvent.PrerequisiteFlag, context, prereqEvent.Result, prereqEvent.PrerequisiteOfFlagKey));
prereqEvent.PrerequisiteFlag, context, prereqEvent.Result, prereqEvent.FlagKey));
}
}
var evalDetail = evalResult.Result;
Expand Down
61 changes: 55 additions & 6 deletions pkgs/sdk/server/test/FeatureFlagsStateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,13 @@ public void CanConvertToValuesMap()
public void CanSerializeToJson()
{
var state = FeatureFlagsState.Builder(FlagsStateOption.WithReasons)
.AddFlag("key1", LdValue.Of("value1"), 0, EvaluationReason.OffReason, 100, false, false, null)
.AddFlag("key2", LdValue.Of("value2"), 1, EvaluationReason.FallthroughReason, 200, true, false, UnixMillisecondTime.OfMillis(1000))
.AddFlag("key3", LdValue.Null, null, EvaluationReason.ErrorReason(EvaluationErrorKind.MalformedFlag), 300, false, false, null)
.AddFlag("key1", LdValue.Of("value1"), 0, EvaluationReason.OffReason, 100, false, false, null,
new List<string>())
.AddFlag("key2", LdValue.Of("value2"), 1, EvaluationReason.FallthroughReason, 200, true, false,
UnixMillisecondTime.OfMillis(1000), new List<string>())
.AddFlag("key3", LdValue.Null, null, EvaluationReason.ErrorReason(EvaluationErrorKind.MalformedFlag),
300, false, false, null, new List<string>()
)
.Build();

var expectedString = @"{""key1"":""value1"",""key2"":""value2"",""key3"":null,
Expand All @@ -92,18 +96,63 @@ public void CanSerializeToJson()
JsonAssertions.AssertJsonEqual(expectedString, actualString);
}

[Fact]
public void CanSerializeFlagPrerequisites()
{
var state = FeatureFlagsState.Builder(FlagsStateOption.WithReasons)
.AddFlag("prereq1", LdValue.Of("value1"), 0, EvaluationReason.OffReason, 100, false, false, null,
new List<string>())
.AddFlag("prereq2", LdValue.Of("value2"), 1, EvaluationReason.FallthroughReason, 200, true, false,
UnixMillisecondTime.OfMillis(1000), new List<string>())
.AddFlag("toplevel", LdValue.Null, null,
EvaluationReason.ErrorReason(EvaluationErrorKind.MalformedFlag), 300, false, false, null,
new List<string>
{
"prereq1", "prereq2"
})
.Build();


var expectedString = @"{""prereq1"":""value1"",""prereq2"":""value2"",""toplevel"":null,
""$flagsState"":{
""prereq1"":{
""variation"":0,""version"":100,""reason"":{""kind"":""OFF""}
},""prereq2"":{
""variation"":1,""version"":200,""reason"":{""kind"":""FALLTHROUGH""},""trackEvents"":true,""debugEventsUntilDate"":1000
},""toplevel"":{
""version"":300,""reason"":{""kind"":""ERROR"",""errorKind"":""MALFORMED_FLAG""},""prerequisites"":[""prereq1"",""prereq2""]
}
},
""$valid"":true
}";
var actualString = LdJsonSerialization.SerializeObject(state);
JsonAssertions.AssertJsonEqual(expectedString, actualString);
}


[Fact]
public void CanDeserializeFromJson()
{
var state = FeatureFlagsState.Builder(FlagsStateOption.WithReasons)
.AddFlag("key1", LdValue.Of("value1"), 0, EvaluationReason.OffReason, 100, false, false, null)
.AddFlag("key2", LdValue.Of("value2"), 1, EvaluationReason.FallthroughReason, 200, true, false, UnixMillisecondTime.OfMillis(1000))
.AddFlag("key1", LdValue.Of("value1"), 0, EvaluationReason.OffReason, 100, false, false, null,
new List<string>())
.AddFlag("key2", LdValue.Of("value2"), 1, EvaluationReason.FallthroughReason, 200, true, false,
UnixMillisecondTime.OfMillis(1000), new List<string> { "key1" })
.Build();

var jsonString = LdJsonSerialization.SerializeObject(state);
var state1 = LdJsonSerialization.DeserializeObject<FeatureFlagsState>(jsonString);

var jsonString2 = LdJsonSerialization.SerializeObject(state1);

// Ensure a roundtrip state -> json -> json is equal.
Assert.Equal(jsonString, jsonString2);

// Ensure a roundtrip state -> json -> state is equal.
Assert.Equal(state, state1);
}
}
}



}
Loading
Loading