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.

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

Differences in $os/$os_XX property handling between web and react-native #244

Closed
2 of 4 tasks
isAdrisal opened this issue Aug 1, 2024 · 8 comments
Closed
2 of 4 tasks
Labels
bug Something isn't working

Comments

@isAdrisal
Copy link

isAdrisal commented Aug 1, 2024

Bug description

There are differences between the handling of $os-related properties on the web (posthog.js) and React Native versions of the PostHog library.

As examples:

posthog-web

An event like $pageview has the following:

{
  "properties": {
    "$os": "Mac OS X",
    "$os_version": "10.15.7",
    "$set": {
      "$os": "Mac OS X",
      "$os_version": "10.15.7"
    },
    "$set_once": {
      "$initial_os": "Mac OS X",
      "$initial_os_version": "10.15.7"
    }
  }
}

posthog-react-native

An event like $screen has the following:

{
  "properties": {
    "$os_name": "iOS",
    "$os_version": "17.5.1",
    "$set": {
      "$os_version": "17.5.1"
    },
    "$set_once": {
      "$initial_os_version": "17.5.1"
    }
  }
}

There may be other discrepancies, but from what I can see:

  1. Naming of $os versus $os_name
  2. $os_name (or $os) is missing from the $set and $set_once fields

Point 2 is the more pressing issue as it makes it difficult to report on app usage between users on various operating systems as the person properties are not available for initial or latest OS the way it is for web.

How to reproduce

  1. Install both the web and react-native libraries
  2. Create a $pageview and $screen event — or any other event
  3. Note the discrepancies described above

Related sub-libraries

  • All of them
  • posthog-web
  • posthog-node
  • posthog-react-native

Additional context

Versions in use:

posthog-js (not -lite): latest (?) — it's loaded via the default embed script which doesn't appear to specify a version
posthog-react-native: 2.11.6 (release notes don't seem to suggest this is changed in v3+)

@isAdrisal isAdrisal added the bug Something isn't working label Aug 1, 2024
@marandaneto
Copy link
Member

@PostHog/team-product-analytics I think it'd make more sense to fall back to $os_name when $os isn't present, so all the other features would work, including the $initial_os property creation.
Fixing that on the SDK side would either cause sending a duplicate property or doing a breaking change (and no back compatibility).
WDYT?

Thanks @isAdrisal for raising this.

@Twixes
Copy link
Member

Twixes commented Aug 1, 2024

Hey @isAdrisal and @marandaneto! $initial_ handling is an ingestion-level operation, so $intial_os is not going to be correct without an SDK change. The situation is similar for the "Latest OS" person property – if we went for a fallback approach, a person's $os property would forever take precedence over $os_name, even if $os_name were the newer value.

As for the $os & $os_name event properties, we can definitely implement a straightforward fallback at the HogQL layer. However, that would make querying $os on events work differently from querying it on persons, which could be quite unexpected.

It looks like an SDK update is essential. We can definitely still continue sending $os_name, just with $os and $initial_os also included.

@marandaneto
Copy link
Member

marandaneto commented Aug 1, 2024

@Twixes what would be the correct one then, $os or $os_name? AFAIK SDKs send either one of them, what is canonical?
My suggestion was during the ingestion level, whenever creating $initial_os, if there's a $os use it, otherwise check for $os_name, seems reasonable, but I'm not sure if I understood you correctly as it's not possible.

@Twixes
Copy link
Member

Twixes commented Aug 1, 2024

I would say $os is more generally recognized, looking at https://github.com/PostHog/posthog.
I think I see what you mean with $initial_os better now. Although that sounds like we should still update the SDK, which anyway makes an ingestion-level change rather redundant with an increase in ingestion complexity.

@marandaneto
Copy link
Member

marandaneto commented Aug 1, 2024

@Twixes

Although that sounds like we should still update the SDK, which anyway makes an ingestion-level change rather redundant with an increase in ingestion complexity.

I don't disagree, but it's likely just an extra ternary Operator in Python, I'd rather prioritize the UX here rather than the clean code correctness, so it'd work for newer and older versions of all SDKs, rather than causing a breaking change in multiple SDKs (at least 5 AFAIK), just my 2cents.

@isAdrisal
Copy link
Author

Hey @marandaneto and @Twixes! Thanks for looking into this so quickly. Is it possible we could go with the ingestion-side fix as a short-term thing to (selfishly) resolve my issue, and then the more-correct SDK fix can be done in a follow-up?

@Twixes
Copy link
Member

Twixes commented Aug 5, 2024

Yeah, I see what you mean. Here's the ingestion-level change: PostHog/posthog#24184. Just needs a review from the pipeline team now.

@isAdrisal
Copy link
Author

Thanks @Twixes, I can see the change live now in PostHog :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants