feat!: Implement builders and immutable contexts. #77

kinyoklion · 2022-10-04T20:02:11Z

Proof of concept implementing builders for EvaluationContext and Structure types to address context threading concerns.

var sample = Structure.Builder().Set("key", "val").Build();
var context = EvalContext.Builder().Set("my-struct", sample).Build();

This allows for the context provided to the client to be immutable, and for immutability for the duration of a hook.

See: #56

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion · 2022-10-04T22:29:59Z

I also have a branch where I implemented interfaces for the builders, structure, and eval context: https://github.com/open-feature/dotnet-sdk/pull/new/rlamb/immutable-with-builders-interfaces

Working through this I wasn't as sure about implementing interfaces. Because if they are interfaces, then they are effectively unconstrained with their threading guarantees. Though possibly that is fine. It is even easier to document that if you implement the interface directly, then all bets are off.

The nice thing about concrete types though is you can seal them be and be sure within reason they are your types.

benjiro

@kinyoklion awesome work, I left a few comments, but everything looks solid. 🏆

src/OpenFeatureSDK/Model/EvaluationContext.cs

src/OpenFeatureSDK/Model/HookContext.cs

src/OpenFeatureSDK/Model/Structure.cs

src/OpenFeatureSDK/Model/Value.cs

src/OpenFeatureSDK/OpenFeatureSDK.csproj

Co-authored-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

…ies for AsDictionary. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

codecov-commenter · 2022-10-05T00:19:05Z

Codecov Report

Merging #77 (7d35490) into main (b59750b) will decrease coverage by 0.78%.
The diff coverage is 90.96%.

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   94.13%   93.34%   -0.79%     
==========================================
  Files          18       20       +2     
  Lines         426      481      +55     
  Branches       36       36              
==========================================
+ Hits          401      449      +48     
- Misses         13       20       +7     
  Partials       12       12

Impacted Files	Coverage Δ
...c/OpenFeatureSDK/Model/EvaluationContextBuilder.cs	`83.33% <83.33%> (ø)`
src/OpenFeatureSDK/Model/Value.cs	`75.60% <85.71%> (-0.07%)`	⬇️
src/OpenFeatureSDK/Model/StructureBuilder.cs	`91.48% <91.48%> (ø)`
src/OpenFeatureSDK/Model/EvaluationContext.cs	`77.27% <91.66%> (+0.48%)`	⬆️
src/OpenFeatureSDK/Hook.cs	`100.00% <100.00%> (ø)`
src/OpenFeatureSDK/Model/HookContext.cs	`100.00% <100.00%> (ø)`
src/OpenFeatureSDK/Model/Structure.cs	`93.10% <100.00%> (-6.90%)`	⬇️
src/OpenFeatureSDK/OpenFeature.cs	`100.00% <100.00%> (ø)`
src/OpenFeatureSDK/OpenFeatureClient.cs	`99.25% <100.00%> (+0.03%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kinyoklion · 2022-10-05T00:35:30Z

GC thrashing was brought up as a possibility. This could potentially be susceptible to that.

One way to reduce that would be to change the types of Value, and possibly Struct to be structs. Then they would be value types and mostly inhabit the stack, or be inline in other objects. Because there are collections, those are still inherently references, but you would only encounter the cost if using them.

That would be more code than this currently is though. It would also use a bit more memory per value, but the dozens of bytes range. Because each of the types would exist in the struct. and only be used if the type was that type. So you have an enum and a number of extra fields.

A lot of this could change internally, and code would still compile. A breaking change at the ABI level though, for people just dropping in assemblies.

toddbaert · 2022-10-05T14:15:28Z

Looks great!

toddbaert · 2022-10-05T18:19:32Z

src/OpenFeatureSDK/Model/StructureBuilder.cs

+        public StructureBuilder Remove(string key)
+        {
+            this._attributes.Remove(key);
+            return this;
+        }


Is this method needed now? Is is realistic that a builder would add and then remove something before having build() called on it?

I could do without the remove methods. I think it makes sense to just put stuff in that you want.

Removed the Removes.

toddbaert · 2022-10-05T18:20:03Z

src/OpenFeatureSDK/Model/EvaluationContextBuilder.cs

+        public EvaluationContextBuilder Remove(string key)
+        {
+            this._attributes.Remove(key);
+            return this;


see: https://github.com/open-feature/dotnet-sdk/pull/77/files#r988214395

toddbaert · 2022-10-05T18:23:06Z

src/OpenFeatureSDK/OpenFeature.cs

@@ -11,7 +11,7 @@ namespace OpenFeatureSDK
    /// <seealso href="https://github.com/open-feature/spec/blob/main/specification/flag-evaluation.md#flag-evaluation-api"/>
    public sealed class OpenFeature
    {
-        private EvaluationContext _evaluationContext = new EvaluationContext();
+        private EvaluationContext _evaluationContext = EvaluationContext.Empty;
        private FeatureProvider _featureProvider = new NoOpFeatureProvider();
        private readonly List<Hook> _hooks = new List<Hook>();


While we're addressing thread safety, do you think we need to worry about the hook list here being mutated in multiple threads?

(Same applies to hooks in client)

Yeah, that is true. I can take a look at that as well. Likely, to finish up thread safety, we may need some changes more than just the immutable builder.

Yeah, the hooks are a problem.

Currently I think there is a problem with the global context as well. A threading issue, but also I think right now your client will have a global context based on when you get the client. So if you update the global context it doesn't affect clients you have already gotten from the SDK.

I suspect that is supposed to be like hooks, where adding a hook affect subsequent evaluations from existing clients.

I think right now your client will have a global context based on when you get the client.

I don't think this is explicitly spelled out in the spec, but it's implied. I think I should add something about it. It doesn't necessarily need to be addressed in this PR.

The same should be true with regards to the configured provider - the currently registered one should be used for all clients, regardless of when they were created.

cc @beeme1mr

Another question, are hooks supposed to be executed in some specific order based on the order they were registered? There is an order of the sources of hooks, but multiple hooks registered at the API level for instance.

It affects the type of concurrency I would provide here. The C# ConcurrentStack has all the correct functionality, but the contents are reversed. (I can enumerable reverse this, so the cost shouldn't be that much.)

ConcurrentQueue puts things in the right order, but doesn't have a clear method. So you cannot atomically remove everything.

Or just making a synchronized collection, I prefer the concurrent solutions as they are generally lockless.

Personally I didn't want to provide guarantees of hook ordering within a stage. The spec never dictates any behavior regarding that, and in a few discussions it seemed like it would only cause problems.

ConcurrentQueue puts things in the right order, but doesn't have a clear method. So you cannot atomically remove everything.

A means for removing hooks is also not specified. Most implementations provide one b/c it helps with testing, but you could make this a package private method if you want. I don't think there's a real compelling use case for allowing authors to remove hooks, though I could be wrong about that.

kinyoklion · 2022-10-05T18:23:56Z

I could look at value types some if we are interested in tackling that now. It may take me a little bit though. I am not as certain how much of a problem that will be.

toddbaert · 2022-10-05T18:24:37Z

I could look at value types some if we are interested in tackling that now. It may take me a little bit though. I am not as certain how much of a problem that will be.

If it's not a breaking change, my gut feeling is to defer it.

kinyoklion · 2022-10-05T18:32:46Z

I could look at value types some if we are interested in tackling that now. It may take me a little bit though. I am not as certain how much of a problem that will be.

If it's not a breaking change, my gut feeling is to defer it.

I think that the Value type would be the place with the most possible benefit, because those are what you have a lot of. It might be a small break. Specifically you cannot have explicit parameter-less constructors for for structs. So the current parameterless constructor for null wouldn't work.

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

toddbaert · 2022-10-06T13:45:30Z

Another point: in Go, Java, and JS, the provider is not a member field in the client. Instead, the currently registered provider is retrieved from the API. From a developer experience perspective, this means that existing clients are "updated" when a new provider is set, which I think is a better experience...?

Again, this isn't in the spec, but perhaps it should be (interested in feedback here). The challenge is that this does introduce more need for locking. In Java I would probably use a read/write lock for this, so n threads could read the provider without contention, and the lock is only unavailable when the provider is updated.

We can also continue to leave this particular case undefined in the spec, with each SDK handling this edge case differently. At a minimum we should document this behavior in each SDK though.

EDIT:

Another reason we probably want to change this behavior, is that, ala OpenTelemetry, one of the long-term goals was to have 3rd party libraries integrate the SDK. They could create clients in their code that first party developers could use. This works optimally if clients created in those third part libraries can use the globally registered provider after their instantiation.

kinyoklion · 2022-10-06T15:36:30Z

@benjiro @toddbaert How about I do another PR to address the hooks and a couple other thread safety issues? Leaving this one just the builders.

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

toddbaert · 2022-10-06T15:37:33Z

@benjiro @toddbaert How about I do another PR to address the hooks and a couple other thread safety issues? Leaving this one just the builders.

Works for me! I don't mind tackling them either if you need to move onto other things. Let me know.

kinyoklion · 2022-10-06T15:42:06Z

~~It seems that GitHub is unhappy with me somehow. Code coverage report not uploading.~~

I re-ran the build and it was appeased.

kinyoklion · 2022-10-06T15:42:41Z

@benjiro @toddbaert How about I do another PR to address the hooks and a couple other thread safety issues? Leaving this one just the builders.

Works for me! I don't mind tackling them either if you need to move onto other things. Let me know.

I have some of this in a branch, but if it looks like I won't get to it today, then I will let you know.

toddbaert · 2022-10-06T15:50:55Z

@benjiro any objections to merging this?

benjiro

LGTM

🤖 I have created a release *beep* *boop* --- ## [0.4.0](v0.3.0...v0.4.0) (2022-10-12) ### ⚠ BREAKING CHANGES * Thread safe hooks, provider, and context (#79) * Implement builders and immutable contexts. (#77) ### Features * Implement builders and immutable contexts. ([#77](#77)) ([d980a94](d980a94)) * Thread safe hooks, provider, and context ([#79](#79)) ([609016f](609016f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

kinyoklion added 3 commits October 4, 2022 12:49

First pass.

37cb08c

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Add doucmentation and do code cleanup.

3556ac7

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Better encapsulate the builder. Update the readme with an example.

ea41589

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion changed the title ~~POC: Implement builders and immutable contexts.~~ feat!: Implement builders and immutable contexts. Oct 4, 2022

Format files.

698ec0c

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion changed the title ~~feat!: Implement builders and immutable contexts.~~ chore: poc, Implement builders and immutable contexts. Oct 4, 2022

kinyoklion mentioned this pull request Oct 4, 2022

[FEATURE] Ensure thread safety of sdk open-feature/go-sdk#90

Closed

kinyoklion closed this Oct 4, 2022

kinyoklion reopened this Oct 4, 2022

benjiro reviewed Oct 4, 2022

View reviewed changes

kinyoklion and others added 3 commits October 4, 2022 17:03

Update src/OpenFeatureSDK/Model/HookContext.cs

b7da2f7

Co-authored-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Update src/OpenFeatureSDK/Model/Structure.cs

832a779

Co-authored-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Use a range for System.Collections.Immutable. Use immutable dictionar…

6c7c88b

…ies for AsDictionary. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion changed the title ~~chore: poc, Implement builders and immutable contexts.~~ feat!: Implement builders and immutable contexts. Oct 5, 2022

toddbaert self-requested a review October 5, 2022 18:13

kinyoklion marked this pull request as ready for review October 5, 2022 18:18

toddbaert reviewed Oct 5, 2022

View reviewed changes

kinyoklion added 2 commits October 5, 2022 11:33

Make value sealed.

bda2ff8

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Make 'Empty' static component be a property.

7d35490

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

Remove the Remove method from the struct and context builder.

b1386cd

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

toddbaert self-requested a review October 6, 2022 15:37

toddbaert approved these changes Oct 6, 2022

View reviewed changes

toddbaert requested a review from benjiro October 6, 2022 17:42

benjiro approved these changes Oct 6, 2022

View reviewed changes

toddbaert merged commit d980a94 into main Oct 6, 2022

github-actions bot mentioned this pull request Oct 6, 2022

chore(main): release 0.4.0 #78

Merged

This was referenced Jan 18, 2023

chore(main): release 1.0.0 #103

Closed

chore(main): release 1.0.0 #104

Closed

chore(main): release 1.0.0 #105

Closed

chore(main): release 1.0.0 #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Implement builders and immutable contexts. #77

feat!: Implement builders and immutable contexts. #77

kinyoklion commented Oct 4, 2022 •

edited

Loading

kinyoklion commented Oct 4, 2022

benjiro left a comment

codecov-commenter commented Oct 5, 2022 •

edited

Loading

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 5, 2022

toddbaert Oct 5, 2022 •

edited

Loading

kinyoklion Oct 5, 2022

kinyoklion Oct 6, 2022

toddbaert Oct 5, 2022

toddbaert Oct 5, 2022 •

edited

Loading

kinyoklion Oct 5, 2022

kinyoklion Oct 5, 2022

toddbaert Oct 5, 2022

kinyoklion Oct 5, 2022

toddbaert Oct 6, 2022 •

edited

Loading

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 5, 2022

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 6, 2022 •

edited

Loading

kinyoklion commented Oct 6, 2022

toddbaert commented Oct 6, 2022

kinyoklion commented Oct 6, 2022 •

edited

Loading

kinyoklion commented Oct 6, 2022

toddbaert commented Oct 6, 2022

benjiro left a comment

feat!: Implement builders and immutable contexts. #77

feat!: Implement builders and immutable contexts. #77

Conversation

kinyoklion commented Oct 4, 2022 • edited Loading

kinyoklion commented Oct 4, 2022

benjiro left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 5, 2022 • edited Loading

Codecov Report

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 5, 2022

toddbaert Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

kinyoklion Oct 5, 2022

Choose a reason for hiding this comment

kinyoklion Oct 6, 2022

Choose a reason for hiding this comment

toddbaert Oct 5, 2022

Choose a reason for hiding this comment

toddbaert Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

kinyoklion Oct 5, 2022

Choose a reason for hiding this comment

kinyoklion Oct 5, 2022

Choose a reason for hiding this comment

toddbaert Oct 5, 2022

Choose a reason for hiding this comment

kinyoklion Oct 5, 2022

Choose a reason for hiding this comment

toddbaert Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 5, 2022

kinyoklion commented Oct 5, 2022

toddbaert commented Oct 6, 2022 • edited Loading

kinyoklion commented Oct 6, 2022

toddbaert commented Oct 6, 2022

kinyoklion commented Oct 6, 2022 • edited Loading

kinyoklion commented Oct 6, 2022

toddbaert commented Oct 6, 2022

benjiro left a comment

Choose a reason for hiding this comment

kinyoklion commented Oct 4, 2022 •

edited

Loading

codecov-commenter commented Oct 5, 2022 •

edited

Loading

toddbaert Oct 5, 2022 •

edited

Loading

toddbaert Oct 5, 2022 •

edited

Loading

toddbaert Oct 6, 2022 •

edited

Loading

toddbaert commented Oct 6, 2022 •

edited

Loading

kinyoklion commented Oct 6, 2022 •

edited

Loading