feat!: Update ProviderEvents to match specifications #170
Conversation
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
|
@gemini /review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates ProviderEvents to align with the Open Feature specification, which is a significant and positive change. The introduction of EventDetails as a payload for events makes the eventing system more robust and extensible. The changes are well-implemented, including backward compatibility for the deprecated ProviderError structure. I've found a couple of minor issues: a leftover debug print statement in a test file, and an opportunity to improve the default error message in FlagNotFoundError to avoid confusion when a flag key is not available. Overall, this is a great step forward for the SDK's compliance with the specification.
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/exceptions/OpenFeatureError.kt
Show resolved
Hide resolved
kotlin-sdk/src/commonTest/kotlin/dev/openfeature/kotlin/sdk/DeveloperExperienceTests.kt
Show resolved
Hide resolved
nicklasl
changed the title
|
Not super happy about the breaking change here and the reason is mainly that I don't see the big use case of having the event details. Ideally I would not want the SDK user to have to care about what happens in the provider(s) at all. I would suggest either to make the events objects default to "empty Details" or maybe even optional. Is there some write up / discussion or some use cases on how the SDK or the SDK users would use these event details? |
|
@nicklasl A use case for the moment on my end is tracking flag changes upon a context update for analytics purposes. Happy to provide default values, reading the specifications more closely, the language of "MAY" and "SHOULD" make it sound like the EventDetails are optional. With that said, this would be a breaking change regardless due to the update of |
PenguinDan
changed the title
ah, of course! I will hopefully be able to take a closer look soon. And also, the PR title, if you add a |
|
@nicklasl Ah I thought I added a ! myself as a typo, didn't see you updated it. Sounds good! I need to update the Multi-Provider in this PR also but those changes should be relatively trivial |
PenguinDan
changed the title
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Show resolved
Hide resolved
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
beeme1mr
requested review from
jonathannorris,
lukas-reining,
thomaspoignant and
toddbaert
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
There was a problem hiding this comment.
I get your point regarding the breaking change @nicklasl!
For the use cases also flags changed is something that can be really interesting in some cases.
As far as I can see, there is no way of making this change non-breaking right? I am not a Kotlin expert, but I do not see a way of avoiding the breaking change if we want to include this.
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/Client.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: penguindan <daniel.seunkim@gmail.com>
4 participants
This PR
The Open Feature spec outlines Event data emitted with ProviderEvent: https://openfeature.dev/specification/types#provider-event-details
A port of the code from open-feature/swift-sdk#77
Breaking Changes
OpenFeatureProviderEventsnow contains an associated value for EventDetails. This payload includes additional information required for all event emitsRelated Issues
Fixes #107
Notes
Follow-up Tasks
How to test