-
Notifications
You must be signed in to change notification settings - Fork 38
feat: auto-attach $screen_name to all events #530
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
Open
turnipdabeets
wants to merge
10
commits into
feat/logs-pr3
Choose a base branch
from
feat/screen-name-on-all-events
base: feat/logs-pr3
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
9067074
feat: auto-attach $screen_name to all events
turnipdabeets 7e6d105
test: cover exception and snapshot paths for $screen_name
turnipdabeets bb9106d
Merge remote-tracking branch 'origin/feat/logs-pr3' into feat/screen-…
turnipdabeets f838ebe
Merge remote-tracking branch 'origin/feat/logs-pr3' into feat/screen-…
turnipdabeets e133aee
docs: expand captureScreenViews comment
turnipdabeets 39ce3ae
docs: reframe screen() KDoc around its primary purpose
turnipdabeets 99c628a
feat: caller empty/whitespace $screen_name falls back to cache + docs
turnipdabeets 3a5ab09
feat: drop screen() calls with blank titles
turnipdabeets afdc75b
fix: trim screen() title; clarify Flutter-passthrough on $screen_name…
turnipdabeets 47a966b
style: spotless format PostHogScreenNameTest
turnipdabeets File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| --- | ||
| "posthog": minor | ||
| "posthog-android": minor | ||
| --- | ||
|
|
||
| Auto-attach `$screen_name` to every captured event after `PostHog.screen()` has been called (manually or via Activity-lifecycle auto-capture). Cached value is cleared by `reset()` and `close()`. Closes #119. | ||
|
|
||
| **To opt out of `$screen_name` stamping entirely**, set `PostHogAndroidConfig.captureScreenViews = false` **and** stop calling `PostHog.screen()` manually. Disabling `captureScreenViews` alone is not sufficient — a single manual `PostHog.screen("Home")` call will re-enable stamping. | ||
|
|
||
| ## Behavior changes | ||
|
|
||
| These all affect what your events carry on the wire. Review your dashboards/insights/HogQL queries: | ||
|
|
||
| - **Cross-event stamping.** `$exception`, `$identify`, `$autocapture`, `$create_alias`, `$groupidentify`, `$feature_flag_called`, custom events, etc. will start carrying `$screen_name` whenever a screen has been recorded in the session. Previously only `$screen` events carried it. `$snapshot` events are excluded. | ||
| - **`PostHog.screen("")` (and whitespace-only titles) are silently dropped.** No `$screen` event is emitted and the cached value is untouched, so the last useful screen name survives. Previously these emitted a `$screen` event with an empty/whitespace `$screen_name`. Customers using empty-string as a sentinel in dashboards will see those rows disappear. | ||
| - **Empty/whitespace `$screen_name` in `properties` falls back to the cache.** Passing `properties = mapOf("$screen_name" to "")` (or whitespace-only) on a `capture(...)` call no longer ships an empty `$screen_name` — the cached value wins. A meaningful caller-supplied value still wins. | ||
| - **No change for `screen()` override semantics.** `PostHog.screen("Home", properties = mapOf("$screen_name" to "Override"))` continues to ship `$screen_name = "Override"` on the `$screen` event itself (Kotlin's `putAll` already let the caller override). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -502,6 +502,19 @@ public class PostHog private constructor( | |
| props.putAll(it) | ||
| } | ||
|
|
||
| // Stamp $screen_name from the cache only if the caller didn't supply a | ||
| // meaningful value. Runs after putAll so a real caller override (incl. | ||
| // posthog-flutter, which sets $screen_name in properties on capture) | ||
| // still takes precedence. Treat caller empty/blank as absent (almost | ||
| // always accidental) so the cache wins. | ||
| if (appendSharedProps) { | ||
| val cachedName = lastScreenName | ||
| val callerName = (props["\$screen_name"] as? String)?.trim() | ||
| if (!cachedName.isNullOrEmpty() && callerName.isNullOrEmpty()) { | ||
| props["\$screen_name"] = cachedName | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flutter also captures $screen_name, and uses the android sdk, so its better to check if we are not overwriting the flutter value here |
||
| } | ||
| } | ||
|
|
||
| // only Session replay needs distinct_id also in the props | ||
| // remove after https://github.com/PostHog/posthog/pull/18954 gets merged | ||
| val propDistinctId = props["distinct_id"] as? String | ||
|
|
@@ -743,6 +756,18 @@ public class PostHog private constructor( | |
| return config?.optOut ?: true | ||
| } | ||
|
|
||
| /** | ||
| * Records a screen view by capturing a `$screen` event with [screenTitle]. | ||
| * | ||
| * The title is also cached and automatically attached as `$screen_name` to | ||
| * every subsequent event (until [reset] or [close] clears it). | ||
| * | ||
| * To override the auto-attached value on a specific event, pass `$screen_name` | ||
| * in that event's `properties` on the next [capture] call. | ||
| * | ||
| * @param screenTitle the screen name to record | ||
| * @param properties additional properties to attach to this `$screen` event | ||
| */ | ||
| public override fun screen( | ||
| screenTitle: String, | ||
| properties: Map<String, Any>?, | ||
|
|
@@ -751,11 +776,15 @@ public class PostHog private constructor( | |
| return | ||
| } | ||
|
|
||
| // Cache for capture-time context snapshot on log records. | ||
| this.lastScreenName = screenTitle | ||
| val trimmedTitle = screenTitle.trim() | ||
| if (trimmedTitle.isEmpty()) { | ||
| return | ||
| } | ||
|
|
||
| this.lastScreenName = trimmedTitle | ||
|
|
||
| val props = mutableMapOf<String, Any>() | ||
| props["\$screen_name"] = screenTitle | ||
| props["\$screen_name"] = trimmedTitle | ||
|
|
||
| properties?.let { | ||
| props.putAll(it) | ||
|
|
||
219 changes: 219 additions & 0 deletions
219
posthog/src/test/java/com/posthog/PostHogScreenNameTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,219 @@ | ||
| package com.posthog | ||
|
|
||
| import com.posthog.internal.PostHogMemoryPreferences | ||
| import com.posthog.internal.PostHogThreadFactory | ||
| import org.junit.Rule | ||
| import org.junit.rules.TemporaryFolder | ||
| import java.io.File | ||
| import java.util.concurrent.Executors | ||
| import kotlin.test.AfterTest | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals | ||
| import kotlin.test.assertFalse | ||
|
|
||
| internal class PostHogScreenNameTest { | ||
| @get:Rule | ||
| val tmpDir = TemporaryFolder() | ||
|
|
||
| private val queueExecutor = Executors.newSingleThreadScheduledExecutor(PostHogThreadFactory("TestQueue")) | ||
| private val replayQueueExecutor = Executors.newSingleThreadScheduledExecutor(PostHogThreadFactory("TestReplayQueue")) | ||
| private val remoteConfigExecutor = Executors.newSingleThreadScheduledExecutor(PostHogThreadFactory("TestRemoteConfig")) | ||
| private val cachedEventsExecutor = Executors.newSingleThreadScheduledExecutor(PostHogThreadFactory("TestCachedEvents")) | ||
| private lateinit var config: PostHogConfig | ||
|
|
||
| private val captured = mutableListOf<PostHogEvent>() | ||
|
|
||
| @Suppress("DEPRECATION") | ||
| private fun getSut(): PostHogInterface { | ||
| val storage = tmpDir.newFolder().absolutePath | ||
| config = | ||
| PostHogConfig(API_KEY, "http://localhost").apply { | ||
| flushAt = 1 | ||
| storagePrefix = File(storage, "events").absolutePath | ||
| replayStoragePrefix = File(storage, "snapshots").absolutePath | ||
| preloadFeatureFlags = false | ||
| cachePreferences = PostHogMemoryPreferences() | ||
| addBeforeSend( | ||
| PostHogBeforeSend { event -> | ||
| captured.add(event) | ||
| null | ||
| }, | ||
| ) | ||
| } | ||
| return PostHog.withInternal( | ||
| config, | ||
| queueExecutor, | ||
| replayQueueExecutor, | ||
| remoteConfigExecutor, | ||
| cachedEventsExecutor, | ||
| reloadFeatureFlags = true, | ||
| ) | ||
| } | ||
|
|
||
| @AfterTest | ||
| fun `set down`() { | ||
| tmpDir.root.deleteRecursively() | ||
| } | ||
|
|
||
| private fun PostHogInterface.captureAndAwait( | ||
| event: String, | ||
| properties: Map<String, Any>? = null, | ||
| ) { | ||
| capture(event, DISTINCT_ID, properties = properties) | ||
| queueExecutor.awaitExecution() | ||
| } | ||
|
|
||
| @Test | ||
| fun `event captured before screen has no screen_name`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.captureAndAwait(EVENT) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertFalse(theEvent.properties!!.containsKey("\$screen_name")) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `event captured after screen carries screen_name`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.captureAndAwait(EVENT) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `caller-supplied screen_name overrides cached value`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.captureAndAwait(EVENT, properties = mapOf("\$screen_name" to "Override")) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Override", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `caller-supplied screen_name from posthog-flutter is preserved`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("AndroidScreen") | ||
| sut.captureAndAwait(EVENT, properties = mapOf("\$screen_name" to "FlutterHome")) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("FlutterHome", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `screen with whitespace-padded title is trimmed for cache and event payload`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen(" Home ") | ||
| sut.captureAndAwait(EVENT) | ||
|
|
||
| val screenEvent = captured.first { it.event == PostHogEventName.SCREEN.event } | ||
| assertEquals("Home", screenEvent.properties!!["\$screen_name"]) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `screen with blank title is dropped and does not touch the cache`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.screen("") | ||
| sut.screen(" ") | ||
| sut.captureAndAwait(EVENT) | ||
|
|
||
| // Blank screen() calls leave the cache intact at the last useful name. | ||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
| assertFalse( | ||
| captured.any { it.event == PostHogEventName.SCREEN.event && (it.properties?.get("\$screen_name") as? String).isNullOrBlank() }, | ||
| ) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `caller-supplied empty screen_name falls back to cached value`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.captureAndAwait(EVENT, properties = mapOf("\$screen_name" to "")) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `caller-supplied whitespace screen_name falls back to cached value`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.captureAndAwait(EVENT, properties = mapOf("\$screen_name" to " ")) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `reset clears screen_name from subsequent events`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.reset() | ||
| sut.captureAndAwait(EVENT) | ||
|
|
||
| val theEvent = captured.first { it.event == EVENT } | ||
| assertFalse(theEvent.properties!!.containsKey("\$screen_name")) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `exception event carries screen_name`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.captureException(RuntimeException("boom")) | ||
| queueExecutor.awaitExecution() | ||
|
|
||
| val theEvent = captured.first { it.event == "\$exception" } | ||
| assertEquals("Home", theEvent.properties!!["\$screen_name"]) | ||
|
|
||
| sut.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `snapshot event does not carry screen_name`() { | ||
| val sut = getSut() | ||
|
|
||
| sut.screen("Home") | ||
| sut.capture("\$snapshot", DISTINCT_ID, properties = mapOf("\$session_id" to "test-session-id")) | ||
| queueExecutor.awaitExecution() | ||
|
|
||
| val theEvent = captured.first { it.event == "\$snapshot" } | ||
| assertFalse(theEvent.properties!!.containsKey("\$screen_name")) | ||
|
|
||
| sut.close() | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
see https://github.com/PostHog/posthog-android/pull/530/changes#r3292518529