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

Use milliseconds for ts and tt parameters #63

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

wzieba
Copy link
Collaborator

@wzieba wzieba commented Sep 7, 2023

Description

Closes: #61

This PR updates time units for tt (total time) and data.ts (timestamp) parameters. This will match iOS behaviour and was discussed internally here: pd3OIC-1is-p2#comment-313

Before After
Screenshot 2023-09-07 at 15 50 18 Screenshot 2023-09-07 at 15 48 21

@wzieba wzieba requested a review from ParaskP7 September 7, 2023 15:19
@ParaskP7 ParaskP7 self-assigned this Sep 8, 2023
Copy link
Collaborator

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed and tested this PR, everything works as expected, good job on bringing parity between the platforms, Android and iOS! 🌟


EXTRAS

  1. Question (❓): I understand that on iOS, the data.inc field is using seconds instead of milliseconds. Maybe we should change that as well (to milliseconds), just to have everything using the same time unit (thus forget working/sending with seconds, which might further help with duplicate events), then update that on iOS too (for parity purposes). Wdyt? 🤔
  2. Question (❓): Similar to the above, another usage of seconds is on data.pubDate. I wonder if we can/want to do such change there too? 🤔

@wzieba
Copy link
Collaborator Author

wzieba commented Sep 8, 2023

Thanks for the review @ParaskP7 !

  1. Question (❓): I understand that on iOS, the data.inc field is using seconds instead of milliseconds. Maybe we should change that as well (to milliseconds), just to have everything using the same time unit (thus forget working/sending with seconds, which might further help with duplicate events), then update that on iOS too (for parity purposes). Wdyt? 🤔

I think that for now, we should keep seconds as iOS followed patterns of JS SDK. Also, tt and inc are incremented only by seconds, so we don't lose any information here.

  1. Question (❓): Similar to the above, another usage of seconds is on data.pubDate. I wonder if we can/want to do such change there too? 🤔

Good point! I looked at iOS codebase and they use pub_date key for this value, while Android uses pub_date_tmsp - I'm not entirely sure what's the difference here (maybe units?) so I'd be extremely careful with updating this value. I'll add an issue to investigate it furter 👍

@wzieba wzieba merged commit d2d5cd9 into master Sep 8, 2023
1 check passed
@wzieba wzieba deleted the issue/61-use-milliseconds-for-ts-tt branch September 8, 2023 12:15
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.

Update time units to milliseconds
2 participants