-
Notifications
You must be signed in to change notification settings - Fork 32
feat(server): Add local evaluation #299
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
Conversation
4c65dc3 to
320f16a
Compare
posthog-server/src/main/java/com/posthog/server/internal/FlagDefinitionModels.kt
Outdated
Show resolved
Hide resolved
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Show resolved
Hide resolved
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Outdated
Show resolved
Hide resolved
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Outdated
Show resolved
Hide resolved
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Outdated
Show resolved
Hide resolved
| val parsedDate = | ||
| try { | ||
| parseDateValue(value.toString()) | ||
| } catch (e: Exception) { |
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'd always use Throwable since it catches more than just Exception
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 typically try to avoid catching Throwable because it includes JVM exceptions that are generally unrecoverable like out of memory, thread death, JVM errors, etc.
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.
you do catch a Throwable when executing scheduleAtFixedRate tho.
I usually prefer it because it's a lower level that captures many more errors, but of course, it depends on the throw signature of the methods you call.
I usually prefer to swallow SDK exceptions rather than let them bubble up, and if the caller does not handle the exception. It crashes the app
Sometimes you can get an OOM exception and its not even because the SDK is using a lot of memory, but it used the last bit, but people would still complain its the SDK doing the wrong thing :(
I will let you decide on this since its more server side and might be less of an issue
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Outdated
Show resolved
Hide resolved
marandaneto
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.
huge PR pheww... :D
i left a few comments, but i'd love to get eyes from the flags team as well, i just checked kotlin-ish code and if stuff made sense but i dont know the logic behind local eval
posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt
Show resolved
Hide resolved
|
Here's the current code coverage summary as of now
|
5d7a9b1 to
8479f0f
Compare
8479f0f to
4e410f0
Compare
posthog/src/main/java/com/posthog/internal/PostHogLocalEvaluationModels.kt
Outdated
Show resolved
Hide resolved
887a3cf to
7489684
Compare
dmarticus
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.
Generally good, I approve from my end with a few recommendations (see the comments for the specific changes). I also have a few general questions:
-
When definitions are reloaded, the feature flag result cache isn't cleared. Is this intentional? Users might get stale results from the cache even after new definitions load.
-
If the initial load fails, what happens? The poller will retry, but should there be a fallback to remote evaluation immediately?
-
Should there be validation that
personalApiKeyis provided whenlocalEvaluation=true?
posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt
Outdated
Show resolved
Hide resolved
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() | ||
| config.logger.log("Interrupted while waiting for flag definitions to load") | ||
| 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.
This returns without notifying waiters afaict, which means that if it's interrupted, it'll return without setting isLoading = false, so other threads might wait.
Maybe do something like this?
synchronized(loadLock) {
while (isLoading) {
try {
loadLock.wait()
} catch (e: InterruptedException) {
Thread.currentThread().interrupt()
isLoading = false
loadLock.notifyAll()
throw InterruptedException("Interrupted while waiting")
}
}
}
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 fact that a waiting thread doesn't wake the other threads is by design. If they've got into this block, it's because another thread is executing the try block below. It'll reach a finally block to reset isLoading = false and to wake the waiting threads.
tl;dr - only the loading thread is responsible for waking the waiting threads
posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt
Show resolved
Hide resolved
posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt
Show resolved
Hide resolved
3bb5b82 to
886d3a2
Compare
This cleans things up for future `onlyEvaluateLocally` config and sending feature flags on capture
Deserialization is handled here, paired closely with the Api. This simplifies deserialization in the server SDK.
This brings method coverage to 92% overall, line 85% overall.
It'll automatically clean up in case the developer forgets to call posthog.close()
It's not necessary considering the first request to trigger local evaluation will synchronously retrieve flag definitions if they're not yet loaded.
Aggregation makes it difficult to access `featureFlags` without exposing it
This reloads feature flag definitions, and is present in other server-side SDKs
Previously the synchronization prevented flag definitions from being overwritten. This now ensures the poller and the client don't load flag definitions at the same time.
2865407 to
86e78e8
Compare
💡 Motivation and Context
This PR adds local evaluation capabilities to feature flags. See the updated USAGE.md for details on how to use it.
If you're reviewing this PR, skimming
FlagEvaluator.kt,LocalEvaluationPoller.kt, and changes toPostHogFeatureFlags.ktwould be the best places to start.This change is pretty massive. I'm sure there's still a couple of bugs lurking, but test coverage is pretty good. This PR doesn't really change the public API, so we can always follow up with smaller bug fixes.
Resolves #307
💚 How did you test it?
Lots of unit testing and updating the Java sample app to use local eval
📝 Checklist