-
Notifications
You must be signed in to change notification settings - Fork 31
feat(java-sdk): Add a server-side stateless interface #284
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
d0d4433
to
f32533e
Compare
- Create PostHogStateless base class - Add PostHogStatelessInterface - Refactor PostHog to extend PostHogStateless - Update all method signatures to use stateless pattern - Fix compatibility issues
f32533e
to
72e8663
Compare
private var isPersonProcessingEnabled: Boolean = false | ||
get() { | ||
synchronized(personProcessingLock) { | ||
if (!isPersonProcessingLoaded) { | ||
isPersonProcessingEnabled = getPreferences().getValue(PERSON_PROCESSING) as? Boolean | ||
?: false | ||
isPersonProcessingLoaded = true | ||
} | ||
} | ||
return field | ||
} | ||
set(value) { | ||
synchronized(personProcessingLock) { | ||
// only set if it's different to avoid IO since this is called more often | ||
if (field != value) { | ||
field = value | ||
getPreferences().setValue(PERSON_PROCESSING, value) | ||
} | ||
} | ||
} |
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 is moved, unchanged, from PostHog.isPersonProcessingEnabled
protected fun mergeGroups(givenGroups: Map<String, String>?): Map<String, String>? { | ||
val preferences = getPreferences() | ||
|
||
@Suppress("UNCHECKED_CAST") | ||
val groups = preferences.getValue(GROUPS) as? Map<String, String> | ||
val newGroups = mutableMapOf<String, String>() | ||
|
||
groups?.let { | ||
newGroups.putAll(it) | ||
} | ||
|
||
givenGroups?.let { | ||
newGroups.putAll(it) | ||
} | ||
|
||
return newGroups.ifEmpty { null } | ||
} |
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 is moved, unchanged, from PostHog.mergeGroups
@Suppress("DEPRECATION") | ||
protected fun buildEvent( | ||
event: String, | ||
distinctId: String, | ||
properties: MutableMap<String, Any>, | ||
): PostHogEvent? { | ||
// sanitize the properties or fallback to the original properties | ||
val sanitizedProperties = config?.propertiesSanitizer?.sanitize(properties)?.toMutableMap() ?: properties | ||
val postHogEvent = PostHogEvent(event, distinctId, properties = sanitizedProperties) | ||
var eventChecked: PostHogEvent? = postHogEvent | ||
|
||
val beforeSendList = config?.beforeSendList ?: emptyList() | ||
|
||
for (beforeSend in beforeSendList) { | ||
try { | ||
eventChecked = beforeSend.run(postHogEvent) | ||
if (eventChecked == null) { | ||
config?.logger?.log("Event $event was rejected in beforeSend function") | ||
return null | ||
} | ||
} catch (e: Throwable) { | ||
config?.logger?.log("Error in beforeSend function: $e") | ||
return null | ||
} | ||
} | ||
|
||
return eventChecked | ||
} |
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 is moved, unchanged, from PostHog.buildEvent
protected fun requirePersonProcessing( | ||
functionName: String, | ||
ignoreMessage: Boolean = false, | ||
): Boolean { | ||
if (config?.personProfiles == PersonProfiles.NEVER) { | ||
if (!ignoreMessage) { | ||
config?.logger?.log("$functionName was called, but `personProfiles` is set to `never`. This call will be ignored.") | ||
} | ||
return false | ||
} | ||
isPersonProcessingEnabled = true | ||
return 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.
This is moved, unchanged, from PostHog.requirePersonProcessing
private fun hasPersonProcessing(): Boolean { | ||
return !( | ||
config?.personProfiles == PersonProfiles.NEVER || | ||
( | ||
config?.personProfiles == PersonProfiles.IDENTIFIED_ONLY && | ||
!isPersonProcessingEnabled | ||
) | ||
) | ||
} |
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 is moved with a slight logical change (isIdentified
is no longer checked) from PostHog.hasPersonProcessing
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.
so the caller should check isIdentified
then otherwise it would be different than before
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.
Thanks. isIdentified
is being called again (see #284 (comment)).
|
||
props["\$process_person_profile"] = hasPersonProcessing() |
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 was also removed by the previous branch. Is this correct?
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.
no idea, it should not be gone afaik
we also have on iOS https://github.com/PostHog/posthog-ios/blob/f381818bc153f63fbe13d6822e65cc7233ba4647/PostHog/PostHogSDK.swift#L293
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.
Thanks. I moved all the logic for person processing flags and person profiles back into their original locations (72cdf86). Looking at other server-side/stateless SDKs, it doesn't look like this needs to be a consideration.
} | ||
} | ||
|
||
private fun buildProperties( |
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 implementation does differ from the method in PostHog.kt
. To my eyes it looked to remove behavior related to session replays, which seems correct.
override fun isFeatureEnabled( | ||
key: String, | ||
defaultValue: Boolean, | ||
): Boolean { |
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.
Re-adding this may have been a mistake during rebase: c5fc361
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.
Yes, this was not intended to be added back in. I've fixed this in d4485b2.
public fun alias (Ljava/lang/String;)V | ||
public fun capture (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;)V | ||
public fun close ()V | ||
public fun debug (Z)V |
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 did we have the debug one?
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.
It moved into PostHogStateless
. It should still be callable from this class given we now inherit from PostHogStateless
.
I've not tested anything, one thing to consider is that these changes should not affect react native and flutter |
interface Looking at other server-side SDKs, these don't look to be considered.
@marandaneto Thanks for the insights, they've been super helpful. I was able to compile against this branch in both |
@dustinbyrne https://github.com/PostHog/posthog-android/blob/main/RELEASING.md |
This is a revival of #225:
Additionally, the goal here is to avoid introducing any functional changes to the Android SDK or public interfaces.
Related to PostHog/posthog#16419
💡 Motivation and Context
To expand the core library to support stateless server-side Java applications. Additional Java interoperability changes should follow in another release.
💚 How did you test it?
According to the previous PR:
I've added additional unit tests to validate stateless behavior.
📝 Checklist