Comments (31)
Looking at the code samples, and how the various opinions on this have shaken out, I propose that each SDK tackle this in their preferred way. This seems like the path of least resistance, since contributors to each particular language seem to agree with a direction. The spec is intentionally not prescriptive about this and it makes sense that implementing languages might make their own stylistic choices here (thread safety isn't even a thing in some languages).
- It sounds like @benjiro and @kinyoklion are in agreement on dotnet using an immutable context
- I won't speak for @justinabrahms , but I gleaned that he is OK with my interface proposal for some forward compatibility in Java...
- @skyerus seems to favor an immutable context, perhaps he and @james-milligan can decide what they prefer for Go
- JavaScript: what even is a thread??
from go-sdk.
In the javasdk, I'm leaning towards immutable eval context objects. Mutable state is just a headache and I'd love to avoid the complexity (at the expense of a few extra objects) if we can.
from go-sdk.
@justinabrahms @kinyoklion @skyerus @james-milligan Here's an example of what I described above. We could even do this in Go, I think. If we wanted.
I will take a look. I also have a POC for using builders in dotnet:
open-feature/dotnet-sdk#77
from go-sdk.
@benjiro It buys us time and also forwards compatibility. If we decide that immutability makes things painful or generates too much GC thrash, we can release an alternative implementation without having to break our API contracts.
I know this isn't a concern in go bc go has awesome interfaces.. but some of us use java. :)
from go-sdk.
The global singletons (e.g. provider, evaluationContext, logger) all share a mutex to ensure thread safety, however a client doesn't use a mutex so there's a concern if two processes want to set an evaluation context for example.
This was my main concern (and still a concern in the Java SDK I think).
As for the evaluation context:
I think that a user mutating a map in multiple threads should be aware of the consequences of that. If we want to protect users from themselves, one other way could perhaps encapsulate the Attribues
field and make the evaluation context a more robust struct with methods for adding values - this is similar to how the EvaluationContext
works in Java. These methods could allow value addition in a threadsafe manner, using a mutex or some other threadsafe primitives behind the scenes.
I'm not sure if this solution is any better than yours. It does allow it to be modified after construction, but there's more code to maintain with my proposal I think.
This would of course be a breaking change. cc @beeme1mr
from go-sdk.
i personally lean more towards @toddbaert's approach, it feels cleaner to have an
EvaluationContext
type with aSetAttribute
/SetAttributes
method(s)
I kinda hate my approach but I worry that it might be the best one.
Although, I hadn't considered setAttribues
(plural) as @james-milligan mentions where we could take an entire map. That's not really as simple in Java but doable in go I think. I think that would be quite convenient.
from go-sdk.
Would allowing the the map to be modified after construction violate this spec requirement?
Requirement 4.1.4
The evaluation context MUST be mutable only within the before hook.
That point is probably a bit misleading. We should improve the wording.
The actual point here is that only before hooks should return an evaluation context (one that's merged with the ambient context). The other hooks do not return the evaluation context. Whether mutating them is possible or not is more of a language and implementation concern.
from go-sdk.
I think you've just found another reason why we should think about thread safety though @skyerus 😅
from go-sdk.
If other hooks have as much control over mutating the
EvaluationContext
as the Before hook does then it raises the question as to whether it remains a property specific to the Before hook thinking
I get your point, and you're right. The point should be changed to say something that indicates that only the before
hook will merge a RETURNED context. Side effects mutating the referenced context in any hook are probably beyond the ability of the spec to dictate. I think there's too many variables across languages for us to reasonably police that. The fact before
hooks allow authors to return a context makes it very explicit that using the before
hooks to change context is safe and valid, since the SDK will carefully merge it for you.
from go-sdk.
type EvaluationContext struct {
mx *sync.Mutex
targetingKey string
attributes map[string]interface{}
}
func (e *EvaluationContext) SetAttribute(key string, value interface{}) {
e.mx.Lock()
...
}
func (e *EvaluationContext) SetAttributes(attrs map[string]interface{}) {
e.mx.Lock()
...
}
func (e *EvaluationContext) SetTargetingKey(key string) {
e.mx.Lock()
...
}
func NewEvaluationContext(targetingKey string, attributes map[string]interface{}) EvaluationContext {
// copy attributes to new map to avoid reference being externally available
}
Loosely I'm thinking of something like this, does this align with what you had in mind?
from go-sdk.
In the javasdk, I'm leaning towards immutable eval context objects. Mutable state is just a headache and I'd love to avoid the complexity (at the expense of a few extra objects) if we can.
I think this is a valid solution. And it's basically analogous to what @skyerus proposed.... but it's a significant and breaking change, right? We would have to remove all the add
methods?
from go-sdk.
In go perspective we can have a constructor (as depicted above) and omit the exported setters, this would enforce immutability (albeit somewhat uncomfortably so).
I'm also in the immutable is preferable camp, it reduces the surface area for issues imo.
from go-sdk.
A drawback to the interface solution is that it could be confusing to application authors using the client's API for the first time and see EvaluationContext
as an interface. It's not immediately obvious what should be placed there, which makes it imo less friendly than being able to input a concrete struct. Perhaps this tradeoff is worth it for the sake of forwards compatibility.
from go-sdk.
@toddbaert I've created an implementation in go similar to your java implementation
from go-sdk.
i personally lean more towards @toddbaert's approach, it feels cleaner to have an EvaluationContext
type with a SetAttribute
/ SetAttributes
method(s)
from go-sdk.
If this is the approach, it must be a priority. To stay on schedule, this change would need to be made available tomorrow.
from go-sdk.
Would allowing the the map to be modified after construction violate this spec requirement?
Requirement 4.1.4
The evaluation context MUST be mutable only within the before hook.
from go-sdk.
If other hooks have as much control over mutating the EvaluationContext
as the Before hook does then it raises the question as to whether it remains a property specific to the Before hook 🤔
from go-sdk.
If this is the approach, it must be a priority. To stay on schedule, this change would need to be made available tomorrow.
I'm happy to drop what I'm currently working on and to pick this up now if we're happy with this approach.
from go-sdk.
I would love to have @kinyoklion 's opinion, since he was the genesis of this discussion... unfortunately he is on the West Coast, I think. I suppose if you start with a basic POC of the idea it would at least give us some data on if it feels right and solves our issues.
I vote you at start on that with an "experimental" mindset for now, until we get feedback from @kinyoklion and perhaps @justinabrahms .
from go-sdk.
yep! I just hope I haven't missed anything silly.
from go-sdk.
looks good to me
from go-sdk.
I agree with @justinabrahms, when possible immutable objects are a better solution. I think a locking solution would work, but I personally would only use it after exhausting other options. It will protect from fundamental threading issues, but not logic/consistency issues around how an application developer may expect the system to behave.
It is preferable that once the context is produced it is either decoupled from the application developer entirely (as in a copy), or that it is immutable.
from go-sdk.
Like I said before:
I kinda hate my approach but I worry that it might be the best one.
I'd like to see what the immutable evaluation context API would look like in Java and Dotnet. I'm in favor of it if it can be done simply!
from go-sdk.
Created a POC PR with the changes we initially decided upon, I can remove the setters if we come to a decision on the immutability.
from go-sdk.
One idea that might allow us to defer this issue for a while:
At least in Java and Dotnet, we could extract the read/merge functionality of EvaluationContext
into an interface, and then have a few implementations... some that are mutable and not threadsafe, some that are mutable and threadsafe, some that are immutable, etc. We wouldn't need to implement them all now. This will be compatible with whatever solutions we want to use in the future, and allow application authors to make their own decisions about thread safety and performance.
For now, it would be a simple change... we'd just create an interface (maybe called EvaluationContext
and then create an implementation (MutableEvaluationContext
, which would probably just be the current EvaluationContext
).
This would allow us to differ this issue while preserving flexibility.
What do you guys think @kinyoklion @justinabrahms ?
from go-sdk.
@justinabrahms @kinyoklion @skyerus @james-milligan Here's an example of what I described above. We could even do this in Go, I think. If we wanted.
from go-sdk.
I'm leaning with @kinyoklion and @justinabrahms on making the EC immutable. Using mutexes can slow down the feature flag evaluation considerably under heavy concurrency(Think a high throughput web API). The more elegant solution would be to avoid mutex locking altogether by ensuring the EC is immutable.
What's the benefit of making the EC an interface other than buying time? I think we should only have a concert implementation of the EvalutionContext that is sealed(can't be extended). Seems like an unnecessary complexity with little benefit. Thoughts?
from go-sdk.
@skyerus my vote is to take the immutability approach, i agree that using an interface may be confusing for developers
from go-sdk.
@toddbaert, what was the conclusion for the Go SDK? immutability through #91?
from go-sdk.
@toddbaert, what was the conclusion for the Go SDK? immutability through #91?
In short, yes. If you have any additional concerns though, please don't hesitate to open a new issue.
from go-sdk.
Related Issues (20)
- Update README to be consistent with other SDKs
- Changes to support spec `v0.6.0` compliance HOT 4
- implement named client support: open-feature/spec@4cf8229 HOT 1
- implement initialization/shutdown: open-feature/spec@a4ffec3
- Implement event support HOT 1
- Conform to spec `v0.7.0` HOT 6
- event handlers should run immediately if the associated provider is in the state associated with that handler already
- [FEATURE] Follow official Go guidance on how to organize a module? HOT 9
- [FEATURE] Follow up for 227 HOT 2
- [BUG] Panic when following getting started tutorial HOT 2
- [FEATURE] Blocking provider mutator HOT 1
- [FEATURE] Expose domain specific provider metadata getter HOT 1
- [FEATURE] Upgrade to Go version 1.20
- [FEATURE] Implement transaction context HOT 5
- [FEATURE] Implement domain scoping HOT 2
- [FEATURE] Make provider interface "stateless", SDK maintains provider state
- Should retrieving a value really provide an error? HOT 5
- [FEATURE] Move to log/slog logging HOT 2
- [BUG] Awkward usage when trying to evaluate a boolean flag in a complex conditional HOT 8
- [FEATURE] New test-oriented client HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from go-sdk.