-
Notifications
You must be signed in to change notification settings - Fork 52
feat: allow validating context to skip initialization + context changes #1300
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
base: main
Are you sure you want to change the base?
feat: allow validating context to skip initialization + context changes #1300
Conversation
3e3ed0f to
88a7d24
Compare
88a7d24 to
ab99c5c
Compare
c621cda to
b535a19
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a validateContext option for setProvider and setProviderAndWait, allowing for more granular control over provider initialization and context reconciliation. The changes are well-implemented, with logic extracted into a new initializeProviderForDomain method in the shared package for better code reuse. The new functionality is also thoroughly tested. My feedback includes a few suggestions to reduce code duplication in event emission logic, which will improve maintainability. Overall, this is a solid contribution that adds valuable flexibility to the SDK.
415258a to
d919ef0
Compare
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
d919ef0 to
96f5f42
Compare
|
Hey @MattIPv4 sorry for the delay on this one. I'll give this a review and some further thought tomorrow. |
lukas-reining
left a comment
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.
Sorry for the long delay @MattIPv4!
I really like the idea and I think it solves an existing problem for the static-context paradigm.
My main concern is that the change does not conform to the current spec in my eyes. I might be overlooking something there. Interested in the opinion of the rest of the @open-feature/technical-steering-committee.
If my concern is correct, I would favor to make this part of the spec.
Again, I think this solves a real problem and could be a good addition to the spec.
| // If there was a validation error with the context, move the newly created provider to ERROR state. | ||
| // We know we've skipped initialization if this happens, so no need to worry about the promise changing the state 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.
The spec says [1]:
The client's provider status accessor MUST indicate ERROR if the initialize function of the associated provider terminates abnormally.
The non-normative part says:
If the provider has failed to initialize, the provider status should indicate the provider is in an error state.
So I think it is acceptable to set the state to ERROR if the initialize has not been run at all due to invalid context. But I am not 100% sure @toddbaert.
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.
I think setting the state to ERROR if the context has been determined to be invalid would break the purpose of this PR, as that would not allow you to suspend until the provider is ready?
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.
Oh sure, I was mislead by
If there was a validation error with the context, move the newly created provider to ERROR state.
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.
Ah I see -- the validation throws an error, it'll move to the ERROR state. If the validation successfully returns false, initialization is just skipped.
| this._logger.debug( | ||
| `Skipping context change for domain '${domain ?? 'default'}' due to validateContext returning false.`, | ||
| ); | ||
| return; |
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.
Returning here makes provider.onContextChange not being called.
While this makes sense if the context is invalid, I think this is not conformant to the spec.
The spec says [1]:
> When the global evaluation context is set, the on context changed function MUST run.
The SDK implementation must run the on context changed function on all registered providers that use the global evaluation context whenever it is mutated.
With this implementation, the context is still mutated [2] but the handler is not run.
While this is logical this is unexpected regarding the spec.
My feeling is we would need to add it to the spec before implementing it this way but I could be on the wrong track @open-feature/technical-steering-committee
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.
Not mutating the context would be a problem too.
In that case it could happen that validateContext is set for the default domain. If there was a context set on the default domain which is invalid for the default domain, that is also expected to be used by a domain scoped provider, the validateContext of the default context could determine the context to be invalid while it is valid for domain scoped provider.
So not mutating would also not be an option from my point of view.
Having that said, I still think we would need to adapt the spec for this feature to be compliant.
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.
Hmm, yes, I suspect this may need a spec change then. I don't see a great way to do this that would be compliant with what you've outlined in the spec currently.
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.
Okay let's wait for feedback of the rest of the TC.
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Matt Cowley <me@mattcowley.co.uk>
This PR
Adds a new options argument when calling
setProviderandsetProviderAndWaitin the web SDK, allowing for a context validation method to be provided. This method can cause the initialization of the provider to be skipped when setting a new provider, leaving it NOT_READY, until a future valid context change is made. It can also cause context change reconciliation to be skipped.Related Issues
Resolves #1279
Notes
While returning false during context validation will prevent a provider from reconciling context changes, the updated context is still stored and persisted.
Follow-up Tasks
N/A
How to test
See test suite :)