PostHog / posthog-android

PostHog Android SDK
https://posthog.com/docs/libraries/android
MIT License
43 stars 23 forks source link

screen_name is not set on $screen events - Using compose #150

Closed curliq closed 2 months ago

curliq commented 4 months ago

Version

3.4.2

Steps to Reproduce

  1. Initialise posthog in the application onCreate:
    val config = PostHogAndroidConfig(
    apiKey = BuildConfig.ANDROID_KEY_POSTHOG,
    host = "https://eu.i.posthog.com",
    captureScreenViews = true,
    captureApplicationLifecycleEvents = true,
    ).apply {
    sessionReplay = false
    debug = BuildConfig.DEBUG
    }
    PostHogAndroid.setup(this, config)
  2. Start an Activity (AppCompatActivity) with startActivity(Intent)
  3. Start more activities

Expected Result

$screen events logged for each activity with screen_name being the Activity::class.java.name or something related to the activity opened

Actual Result

$screen events logged for each activity with screen_name being the value from rootProject.name = ... in the root settings.gradle.kts file

I'm using multiple activities all with compose inside them.

Please let me know if more info is needed to reproduce

marandaneto commented 3 months ago

@curliq thanks for the issue.

Sadly this is an issue with Compose, since the classes are wrapped/autogenerated, not sure if there's a way to do this correctly when using compose.

See what we do here https://github.com/PostHog/posthog-android/blob/c6a5fc6196da76366d3851b28d2e196aa65b1afc/posthog-android/src/main/java/com/posthog/android/internal/PostHogAndroidUtils.kt#L125-L134

Any ideas on how to get this right for Compose if possible at all?

I know we could offer a way for you to annotate the Compose screens with an annotation but we don't offer this feature yet. Another option, for now, is to deactivate the captureScreenViews option and capture screen events manually.

Another option is implementing a Nav controller using the https://developer.android.com/reference/androidx/navigation/NavController.OnDestinationChangedListener listener

thisames commented 3 months ago

@curliq The problem is probably because you didn't define a label for your activity. I believe that compose has no relation in this case.

https://github.com/PostHog/posthog-android/blob/c6a5fc6196da76366d3851b28d2e196aa65b1afc/posthog-android/src/main/java/com/posthog/android/internal/PostHogAndroidUtils.kt#L125-L134

loadLabel() will return the android:label of that you defined in your activity XML. If no label is defined (is the most common), it will return the application android:label. # I created a project with some activities with composes, and I did these tests with labels set and not set, and everything indicates that this is the problem

I also did the same tests for a project without compose and the same thing happens.

in my solution

internal fun Activity.activityLabel(config: PostHogAndroidConfig): String? {
    return try {
        val packageManager = packageManager
        val componentName = componentName
        val activityInfo = packageManager.getActivityInfo(componentName, GET_META_DATA)
        val applicationInfo = applicationInfo

        val activityLabel =
            activityInfo.nonLocalizedLabel?.toString() ?: activityInfo.loadLabel(packageManager)
                .toString()
        val applicationLabel =
            applicationInfo.nonLocalizedLabel?.toString() ?: applicationInfo.loadLabel(
                packageManager
            ).toString()

        if (activityLabel.isNotEmpty() && activityLabel != applicationLabel) {
            activityLabel
        } else {
            null
        }
    } catch (e: Throwable) {
        config.logger.log("Error getting the Activity's label: $e.")
        null
    }
}

This ensures that the activityLabel method will only return a label if it is explicitly defined for the Activity, avoiding returning the Application's label by mistake.

One suggestion I have is that when an activity does not have a defined label, it takes the name of the activity. (the activity name will always exist)

@marandaneto If what I'm saying makes sense, I'd like to make a PR with the corrections.

marandaneto commented 3 months ago

The order of loadLabel is: android:label from the Activity. If not defined, android:label, from the application tag. Otherwise the Activity class name (absolute such as com.posthog.android.sample.MainActivity) so there's no fallback needed since this is by design already.

This ensures that the activityLabel method will only return a label if it is explicitly defined for the Activity, avoiding returning the Application's label by mistake.

This makes sense, I'd love a PR for that.

marandaneto commented 3 months ago

The issue above indeed has nothing to do with Compose, the problem with Compose is that usually, it's a single Activity (not always tho), so it's always gonna be eg "MainActivity", but the app itself has many@Composable screens. eg https://github.com/android/compose-samples/blob/main/JetNews/app/src/main/java/com/example/jetnews/ui/interests/InterestsScreen.kt Ideally, this should track the InterestsScreen but its returning MainActivity, the OnDestinationChangedListener should solve that because we track the destination instead.

marandaneto commented 3 months ago

https://github.com/PostHog/posthog.com/pull/9113 so the current fallback is documented as well.

thisames commented 3 months ago

I'm a little confused now, according to this doc, is it correct to return the android:label of the <application> if the android:label of the <activity> is not defined?

So there is no issue, correct? Everything is going as it should

marandaneto commented 3 months ago

I'm a little confused now, according to this doc, is it correct to return the android:label of the <application> if the android:label of the <activity> is not defined?

So there is no issue, correct? Everything is going as it should

nope, I'd say it's wrong, this is the current state, happy to remove that part once you make a PR and its released. it's a side effect of loadLabel I was not aware of.

thisames commented 3 months ago

I'm a little confused now, according to this doc, is it correct to return the android:label of the <application> if the android:label of the <activity> is not defined? So there is no issue, correct? Everything is going as it should

nope, I'd say it's wrong, this is the current state, happy to remove that part once you make a PR and its released. it's a side effect of loadLabel I was not aware of.

Please confirm whether it will be necessary to keep adding activityName method.

https://github.com/thisames/posthog-android/commit/b12bf287ec49e296ae0c4a44c524ec706d4adc55

or do I just keep the new functionality of activityLabel method

marandaneto commented 2 months ago

@thisames left a few comments otherwise all good.