Skip to content
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.

Sign up for GitHub

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

chore: add support to set screen name to all event #181

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore: add support to set screen name to all event #181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2c2d86
chore: add support to set screen name to all event
thisames Sep 23, 2024
8bf3765
chore: added screen processor
thisames Sep 26, 2024
1858fcd
chore: add unit tests and fix Processor classes modules
thisames Nov 28, 2024
e75d565
fix format
thisames Nov 28, 2024
b765efb
chore: generate api
thisames Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions posthog/src/main/java/com/posthog/PostHog.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public class PostHog private constructor(
private var isIdentifiedLoaded: Boolean = false
private var isPersonProcessingLoaded: Boolean = false

private lateinit var currentScreen: String
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach leaves room for too many false positives.
eg I call screen(x) for screen X, but I don't call screen(y) for screen Y, when events are captured in the screen y, the screen name is x.

Copy link
Member

@marandaneto marandaneto Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better approach would be using https://github.com/PostHog/posthog-android/blob/main/posthog-android/src/main/java/com/posthog/android/internal/PostHogActivityLifecycleCallbackIntegration.kt
So we can cache the current Activity based on the screen's lifecycle and infer the name from it.
If screen is called either manually, it has precedence over the current activity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you are correct. The problem with using PostHogActivityLifecycleCallbackIntegration.kt is that we would have to add method signature in PostHogInterface, and i dont know if it make sense.

see theses changes
thisames@7d4a6f5

do you think this make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a new concept to the SDK that allows enriching the event after being captured, for example, an event processor.
I will make an example and you can expand from there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this example https://github.com/PostHog/posthog-android/compare/chore/processors?expand=1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, you made it much easier for me. I will update this PR with the changes and then you check and give me the ok, if it works I will do the unit tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to use the super properties for the screen name

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

Lines 783 to 802 in 3a0ede8

public override fun register(
key: String,
value: Any,
) {
if (!isEnabled()) {
return
}
if (ALL_INTERNAL_KEYS.contains(key)) {
config?.logger?.log("Key: $key is reserved for internal use.")
return
}
getPreferences().setValue(key, value)
}
public override fun unregister(key: String) {
if (!isEnabled()) {
return
}
getPreferences().remove(key)
}

both would work the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the solution with processors, it's working well


public override fun <T : PostHogConfig> setup(config: T) {
synchronized(setupLock) {
try {
Expand Down Expand Up @@ -317,6 +319,10 @@ public class PostHog private constructor(
}
}

currentScreen.let {
props["\$screen_name"] = currentScreen
}

userProperties?.let {
props["\$set"] = it
}
Expand Down Expand Up @@ -506,6 +512,8 @@ public class PostHog private constructor(
val props = mutableMapOf<String, Any>()
props["\$screen_name"] = screenTitle

currentScreen = screenTitle

properties?.let {
props.putAll(it)
}
Expand Down
40 changes: 40 additions & 0 deletions posthog/src/test/java/com/posthog/PostHogTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,6 +1044,46 @@ internal class PostHogTest {
sut.close()
}

@Test
fun `captures screen event and alias event with screen_name`() {
val http = mockHttp()
val url = http.url("/")

val sut = getSut(url.toString(), preloadFeatureFlags = false)

val screenName = "HomeScreen"

val alias = "UserAlias"
sut.screen(screenName)

sut.alias(alias)

queueExecutor.shutdownAndAwaitTermination()

var request = http.takeRequest()

assertEquals(2, http.requestCount)

var content = request.body.unGzip()
var batch = serializer.deserialize<PostHogBatchEvent>(content.reader())

var theEvent = batch.batch.first()

assertEquals(screenName, theEvent.properties!!["\$screen_name"])

request = http.takeRequest()

assertEquals(2, http.requestCount)

content = request.body.unGzip()
batch = serializer.deserialize<PostHogBatchEvent>(content.reader())

theEvent = batch.batch.first()
assertEquals(alias, theEvent.properties!!["alias"])
assertEquals(screenName, theEvent.properties!!["\$screen_name"])
sut.close()
}

@Test
fun `reset session id when reset is called`() {
val http = mockHttp()
Expand Down