-
Notifications
You must be signed in to change notification settings - Fork 62
feat(statsig)!: Migrate to Java Core #1656
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?
Conversation
Signed-off-by: liran2000 <[email protected]>
Summary of ChangesHello @liran2000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant upgrade of the Statsig Java SDK within the OpenFeature provider, transitioning from the legacy Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 successfully migrates the Statsig provider to the new Java Core SDK. The changes are comprehensive, updating the provider logic, configuration, and tests to align with the new API, such as using builders, handling async operations with CompletableFuture, and using a non-static SDK instance. The tests have been notably improved by using mocking for SDK classes. My review includes suggestions to remove leftover debugging statements and commented-out code, fix a minor typo in the README, and enhance a test case for better robustness.
providers/statsig/README.md
Outdated
| ## Release Notes | ||
|
|
||
| ### 0.3.0 | ||
| Migrated to according to [Migration guide](https://docs.statsig.com/server-core/migration-guides/java#java-migration-steps). |
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.
There's a minor grammatical error in the release notes. It should be "Migrated according to..." instead of "Migrated to according to...".
| Migrated to according to [Migration guide](https://docs.statsig.com/server-core/migration-guides/java#java-migration-steps). | |
| Migrated according to [Migration guide](https://docs.statsig.com/server-core/migration-guides/java#java-migration-steps). |
| // if (!assumeFailure(featureGate)) { | ||
| // evaluatedValue = featureGate.getValue(); | ||
| // } |
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.
providers/statsig/src/main/java/dev/openfeature/contrib/providers/statsig/StatsigProvider.java
Outdated
Show resolved
Hide resolved
...ers/statsig/src/test/java/dev/openfeature/contrib/providers/statsig/StatsigProviderTest.java
Outdated
Show resolved
Hide resolved
...ers/statsig/src/test/java/dev/openfeature/contrib/providers/statsig/StatsigProviderTest.java
Outdated
Show resolved
Hide resolved
...ers/statsig/src/test/java/dev/openfeature/contrib/providers/statsig/StatsigProviderTest.java
Show resolved
Hide resolved
...ers/statsig/src/test/java/dev/openfeature/contrib/providers/statsig/StatsigProviderTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: liran2000 <[email protected]>
lindner
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.
Overall LGTM, just some minor nits, since I'm starting with JavaCore. Others who are already using this provider may have issues if the transitive deps have changed significantly.
| StatsigOptions statsigOptions = new StatsigOptions(); | ||
| statsigOptions.setLocalMode(true); | ||
| StatsigOptions statsigOptions = new StatsigOptions.Builder() | ||
| // .setLocalMode(true) |
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.
Remove? Or make it such that the test passes with/without?
| <version>1.18.1</version> | ||
| <artifactId>javacore</artifactId> | ||
| <version>0.12.1</version> | ||
| <classifier>uber</classifier> |
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.
Why is this uber? Does this include all of the transitive compiled versions for various platforms?
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.
From docs:
The uber JAR is recommended for most use cases as it includes native libraries for all popular platforms and simplifies deployment. Use platform-specific JARs only if you need to minimize JAR size or have specific dependency requirements.
Recommended: Using the Uber JAR (All-in-One)
Since version 0.4.0, Statsig provides an “uber” JAR that contains both the core library and native libraries for popular supported platforms in a single package. This is the recommended approach for most users.
Since we are like a library here for multi-platform, this includes all, and consumers can control it more precisely if wanted.
providers/statsig/README.md
Outdated
| ## Release Notes | ||
|
|
||
| ### 0.3.0 | ||
| Migrated to Java Core according to [Migration guide](https://docs.statsig.com/server-core/migration-guides/java#java-migration-steps). |
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.
Definitely mention that this is a major breaking change and that there is migration needed. Also mention that the last version of non-javacore requires a downgrade.
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.
marked as a breaking change at the PR title for release notes and added this to the readme:
The provider usage is basically unchanged, the underlying implementation is changed.
As the initialization code may change, can refer to the migration guide for details.
| mutableContext.add("name", dynamicConfig.getName()); | ||
| mutableContext.add("value", Structure.mapToStructure(dynamicConfig.getValue())); | ||
| mutableContext.add("ruleID", dynamicConfig.getRuleID()); | ||
| mutableContext.add("groupName", dynamicConfig.getGroupName()); |
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.
are these provided in a different way? If so document in the README.md
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.
documented
| log.info("shutdown"); | ||
| Statsig.shutdown(); | ||
| log.info("shutdown begin"); | ||
| CompletableFuture<Void> shutdownFuture = statsig.shutdown(); |
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.
check if statsig != null?
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Migrated to Java Core according to Migration guide.
solves #1627.
@lindner and others you are welcome to review