chore: java sdk based on the android core sdk #129

marandaneto · 2024-05-02T13:06:29Z

💡 Motivation and Context

Move away from https://github.com/postHog/posthog-java
Start using the https://mvnrepository.com/artifact/com.posthog/posthog library
Fix issues such as Java SDK Update posthog-java#50, Stop shading and relocating dependencies posthog-java#51 and stop printing to sdtout/stderr posthog-java#53
Support feature flags and payloads

Not super happy with the abstractions (PostHogFeatureFlagsInterface, PostHogFileInterface) but that was the easiest way to get something working fast and not doing breaking changes in the public APIs and changing the world, there's room for improvement though.

TODOs:

Local evaluation (not on this PR - not for now, just using decide)
Fix methods and classes comments (aka javadocs)
Fix tests and add new ones for the new use cases
Check and add method overloads for people using Java instead of Kotlin otherwise it will be super verbose (optional parameters in Kotlin are not optional in Java)
Probably rename repo from posthog-android to posthog-jvm or posthog-kotlin, something more generic that will be a monorepo based on the Kotlin core.

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
I updated the docs if needed.
No breaking change or entry added to the changelog.

github-actions · 2024-05-02T13:06:52Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Next" section. Make sure the entry includes this PR's number.
Example:

## Next
- java sdk based on the android core sdk ([#129](https://github.com/PostHog/posthog-android/pull/129))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 7d757b7

marandaneto · 2024-05-07T12:43:29Z

posthog/src/main/java/com/posthog/PostHogInterface.kt

+     * @param groups the group properties, set as a "$groups" property, Docs https://posthog.com/docs/product-analytics/group-analytics
     */
    public fun capture(
        event: String,
        distinctId: String? = null,
        properties: Map<String, Any>? = null,
        userProperties: Map<String, Any>? = null,
        userPropertiesSetOnce: Map<String, Any>? = null,
-        groupProperties: Map<String, Any>? = null,
+        groups: Map<String, Any>? = null,


That was a bug

marandaneto · 2024-05-07T13:27:12Z

posthog-android/src/main/java/com/posthog/android/PostHogAndroidConfig.kt

+        isClientSDK = true
+        featureFlagsRequestTimeoutSeconds = 10
+        flushAt = 20
+        maxQueueSize = 1000
+        maxBatchSize = 50
+        disableGeoIP = false
+        preloadFeatureFlags = true


Those are the defaults for Android

marandaneto · 2024-05-07T13:28:18Z

posthog-android/src/test/java/com/posthog/android/PostHogFake.kt

@@ -22,7 +22,7 @@ public class PostHogFake : PostHogInterface {
        properties: Map<String, Any>?,
        userProperties: Map<String, Any>?,
        userPropertiesSetOnce: Map<String, Any>?,
-        groupProperties: Map<String, Any>?,
+        groups: Map<String, Any>?,


I wonder if this should have been Map<String, String> since group(type: String, key: String) are Strings only

marandaneto · 2024-05-07T13:29:59Z

posthog-android/src/test/java/com/posthog/android/PostHogFake.kt

+    override fun getAllFeatureFlagsAndPayloads(
+        distinctId: String?,
+        groups: Map<String, Any>?,
+    ): Pair<Map<String, Any>?, Map<String, Any?>?> {


Not a big fan of Pair, but returning a map with 2 keys like in python:
return {"featureFlags": None, "featureFlagPayloads": None} isn't ideal either, people would need to know the key.

marandaneto · 2024-05-07T13:31:01Z

posthog-android/src/test/java/com/posthog/android/PostHogFake.kt

@@ -76,17 +96,22 @@ public class PostHogFake : PostHogInterface {
        type: String,
        key: String,
        groupProperties: Map<String, Any>?,
+        distinctId: String?,


distinctId is optional for Android since its cached in the disk/memory and required for Java, this has to be over documentation/method docs, otherwise I'd need to create another class (PostHogAndroid, PostHogJava, PostHogCore) and dupe a lot of code just because of this facility, is it worth it?

"is it worth it"... 🤔

it's a dev experience question I guess....

what does it mean for what I have to type?
what does it mean for how easy it is to make a mistake?
if i make a mistake how do I find out? will i get helpful errors

If you call capture without distinctId on a Java backend app, it'll log in the console:

PostHog.capture("event")
vs
PostHog.capture("event", distinctId = "123")

"capture call not allowed, distinctId is invalid: $var."

I agree, I also prefer better dev UX but since both libs (core and Android) share the same interface, this is the issue we get hence my comment above.
If we prioritize better dev UX having an interface each lib we'd need to make major breaking changes, so we'd need to cut Android v4 as well, I was not aware or was an oversight from my side that distinctId has to be always passed during capture calls on Java since its stateless, this could be solved with scope management - thread local storage (if you set distinctId on thread/request X its always X, etc) but that's another big change not intended to be done now anyway.

marandaneto · 2024-05-07T13:38:44Z

posthog/src/main/java/com/posthog/internal/PostHogQueue.kt

            }
        }

        var deleteFiles = true
        try {
+            // TODO: python checks if the payload isnt bigger than 900kb


The tricky part here is that we serialize/stream from the disk directly to the network because of performance reasons (android specifically), so we don't load everything in memory.
if we wanna check the size before I'd need to change that and have one implementation per platform or pay the performance price on android.
Since we don't have this check yet on Android either (events are small anyways), I will keep as like this and implement whenever is needed

events are small anyways

...wellllll until someone puts something massive in a property 🙈

posthog-js truncates properties to a configurable max length (and doesn't enforce a max number of properties) but doesn't otherwise check the size so it must be ok to let capture fail here...

#130 at least reduces the batch size on the next retry so it's only gonna drop the event entirely if a single event is bigger than our limits

marandaneto · 2024-05-07T13:42:01Z

posthog/src/main/java/com/posthog/internal/PostHogQueue.kt

+            // TODO: python retries up to 3 times with a 500ms interval
+            // if the API returns between 400 and 500


This probably has to be implemented in case we think its important to retry up to a few times even if the API returns an error.
Right now we drop the events since we prefer to not be shooting the API every time in case there's an issue with the backend.

pauldambra · 2024-05-07T17:37:30Z

posthog/src/main/java/com/posthog/PostHogConfig.kt

+    public var preloadFeatureFlags: Boolean = false,
    /**
     * Number of minimum events before they are sent over the wire
     * Defaults to 20
     */
-    public var flushAt: Int = 20,
+    public var flushAt: Int = 100,
    /**
     * Number of maximum events in memory and disk, when the maximum is exceed, the oldest
     * event is deleted and the new one takes place
     * Defaults to 1000
     */
-    public var maxQueueSize: Int = 1000,
+    public var maxQueueSize: Int = 10000,
    /**
     * Number of maximum events in a batch call
     * Defaults to 50
     */
-    public var maxBatchSize: Int = 50,
+    public var maxBatchSize: Int = 100,


values and comments are out of sync 🙈

Fix methods and classes comments (aka javadocs)

yes, its part of the TODO list on the PR description already

pauldambra · 2024-05-07T17:38:30Z

posthog/src/main/java/com/posthog/PostHogConfig.kt

@@ -136,6 +139,9 @@ public open class PostHogConfig(
    @PostHogInternal
    public var snapshotEndpoint: String = "/s/"

+    @PostHogInternal
+    public var isClientSDK: Boolean = false


since this is public config...

I don't actually know what client sdk means in this context...

(does it mean "android" or "java"? in which case is it better to be those strings?)

To tack on top - is there a way to create a stateless + stateful core that inherits from stateless for the SDK code here? Similar to what we do in node.

It feels a bit awkward to have this interface where we add stateful bits based on isClientSDK in otherwise stateless functions. Makes it way too easy to introduce bugs + hard to reason about what can happen.

Alternatively, maybe we can extract the stateful parts out of the pure java SDK and into the android one? And have a clean interface for java that allows passing in whatever data you want, buuut doesn't depend on knowing whether it's client side or not.

I understand the confusion since the JVM runs everywhere.
client means any app running on the user's device eg a Desktop app, an Android app, etc.
Sever means, well running on a server, this concept is known in the JVM world I guess unless I made it up years ago and kept it using lol (think of HTTP Server and HTTP Client), also its annotated with @PostHogInternal so no one should touch that besides us, with java docs this will be clearer.
Sadly Kotlin props visibility isn't great for multipackage libs, it has to be public to be reused.

I agree with the abstractions not being great, the goal here was a quick win as described in the PR description, not breaking the current interface so we don't have to cut Android v4 and offer a migration plan again.
The reason is mainly because of that.

miguelhrocha · 2024-06-02T16:14:53Z

Hey @marandaneto!

I just noticed this PR 😅 and now I understand my proposals most likely won't get merged. However, I hope it helps for bringing the JVM version to life faster.

PostHog/posthog-java#54

I think things like the POJOs, and the FeatureFlagPoller should be relevant enough in Kotlin world. One should be able to paste this files in IntelliJ and it would do the conversion to Kotlin.

Let me know if I can be of any further help!

marandaneto · 2024-06-03T14:40:19Z

Hey @marandaneto!

I just noticed this PR 😅 and now I understand my proposals most likely won't get merged. However, I hope it helps for bringing the JVM version to life faster.

PostHog/posthog-java#54

I think things like the POJOs, and the FeatureFlagPoller should be relevant enough in Kotlin world. One should be able to paste this files in IntelliJ and it would do the conversion to Kotlin.

Let me know if I can be of any further help!

Yep, most likely, the idea is to kill posthog-java and replace with the core parts of posthog-android since the Kotlin code is JVM compatible anyway and it has all the needed features but the feature flag poller.

True that, a lot of FeatureFlagPoller can be reused, thanks for that.

This is paused for now because, after the code review, we agreed that some breaking changes would need to be made to have a separate and friendly Java BE and Android FE interface, so there's no need for a few hacky solutions such as those mentioned in the comments above (public but internal fields, optional distinctId, etc), the goal is to get closer to the interfaces defined in the Node/RN SDK which is working well.

We might need a new major Android version very soon, so then we can do a single breaking change with proper migration steps at once, instead of 2 or more in such a short period, if that makes sense.

Thanks for the PR though.

chore: java sdk based on the android core sdk

5d5b280

marandaneto added 8 commits May 2, 2024 15:11

fixes

7b484cd

add new attributes

156576b

fix

1c4b085

remove sample api

c01ba87

fixes

c10cc3c

fixes

8c055ee

sync flags

87f9554

ref

14c1ffc

marandaneto commented May 7, 2024

View reviewed changes

marandaneto added 4 commits May 7, 2024 14:59

new apis

b45884a

compile tests

8f3ac36

compile android tests

001fec6

add todo

7d757b7

marandaneto requested review from neilkakkar and a team May 7, 2024 13:26

marandaneto commented May 7, 2024

View reviewed changes

marandaneto requested review from a team and removed request for neilkakkar May 7, 2024 13:56

pauldambra reviewed May 7, 2024

View reviewed changes

marandaneto mentioned this pull request Jun 3, 2024

fix: rename groupProperties to groups for capture methods #139

Merged

4 tasks

marandaneto mentioned this pull request Nov 13, 2024

Java SDK Update PostHog/posthog-java#50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: java sdk based on the android core sdk #129

chore: java sdk based on the android core sdk #129

marandaneto commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

marandaneto May 7, 2024

marandaneto May 7, 2024

marandaneto May 7, 2024

marandaneto May 7, 2024

marandaneto May 7, 2024 •

edited

Loading

pauldambra May 7, 2024

marandaneto May 8, 2024

marandaneto May 7, 2024

pauldambra May 7, 2024

marandaneto May 8, 2024

marandaneto May 7, 2024

pauldambra May 7, 2024

marandaneto May 8, 2024

pauldambra May 7, 2024

neilkakkar May 7, 2024

marandaneto May 8, 2024 •

edited

Loading

miguelhrocha commented Jun 2, 2024

marandaneto commented Jun 3, 2024

		// TODO: python retries up to 3 times with a 500ms interval
		// if the API returns between 400 and 500

chore: java sdk based on the android core sdk #129

Are you sure you want to change the base?

chore: java sdk based on the android core sdk #129

Conversation

marandaneto commented May 2, 2024 • edited Loading

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

github-actions bot commented May 2, 2024 • edited Loading

Instructions and example for changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto May 8, 2024 • edited Loading

Choose a reason for hiding this comment

miguelhrocha commented Jun 2, 2024

marandaneto commented Jun 3, 2024

marandaneto commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

marandaneto May 7, 2024 •

edited

Loading

marandaneto May 8, 2024 •

edited

Loading