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

chore: more session replay metadata #176

Closed
wants to merge 2 commits into from
Closed

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Sep 13, 2024

💡 Motivation and Context

Ideas for https://posthog.slack.com/archives/C043VJ93L3B/p1725259136850079

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

Copy link

github-actions bot commented Sep 13, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Next" section. Make sure the entry includes this PR's number.
Example:

## Next
- more session replay metadata ([#176](https://github.com/PostHog/posthog-android/pull/176))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against d1d8d2b

Comment on lines 17 to +18
"\$snapshot_source" to "mobile",
"\$snapshot_mode" to mode.mode,
Copy link
Member Author

@marandaneto marandaneto Sep 13, 2024

Choose a reason for hiding this comment

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

maybe source and mode should only be part of the full snapshot?

technically the mode can also be inferred server side since we only send wireframe.type=screenshot if it's the screenshot mode.

the SDK and platform can also be inferred from $lib, so there is no need for something else here, eg posthog-android, posthog-ios, posthog-react-native, etc.

Copy link
Member

Choose a reason for hiding this comment

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

maybe source and mode should only be part of the full snapshot?

if we do the same as snapshot source then we'll take the first not-null value we receive in a session as the only value for that property. so safest to be on everything for debugging, but safe to be on only some messages

(with the assumption that very few or no sessions will span web and mobile or wireframe and snapshot)

technically the mode can also be inferred server side

ideally we'd keep making ingestion less dependent on the content of the messages - i.e. we can't extra compress the web messages right now because we need to read the contents in blobby. so ideally we'd be explicit here - doesn't help old versions obvs but better in the long run IMHO

Copy link
Member Author

Choose a reason for hiding this comment

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

What's about the naming $snapshot_mode and as a sibling of $snapshot_source, is that ok?
Are we ok to infer the SDK from $lib as well or should we be more explicit here?

Copy link
Member

Choose a reason for hiding this comment

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

yep mode is good...
so long as we're sending $lib then i think we need that to be unique anyway so we're good there

@marandaneto
Copy link
Member Author

we are optimizing the screenshot snapshots with better encoding and some other things, closing this for now.
we already send $lib, so also do not need to do anything else.

@marandaneto marandaneto closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants