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

Send enrollment dates in ET, and align conversion windows with ET days. #2950

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

sammacbeth
Copy link
Collaborator

@sammacbeth sammacbeth commented Feb 12, 2025

Reviewer:

Description:

Our A/B/N framework assumes that:

  1. Enrollment dates are sent in ET (UTC-5).
  2. Conversation windows are calculated over calendar days (in ET), rather than rolling 24-hour windows from enrollment.

Our initial implementation used UTC dates (default for javascript Date objects), and using rolling windows, so this PR corrects that by:

  1. Converting the stored enrollment timestamps to a date in ET when sending.
  2. Setting the enrollment date to midnight in ET when joining an experiment. This means that conversion windows will get day-aligned without us having to change the rolling-window implementation.

This also contains a fix for a potential race with the app_use metric. If it is triggered too early, we may not yet have the config processed and ready to handle metrics.

Steps to test this PR:

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@sammacbeth sammacbeth requested a review from kzar February 13, 2025 10:36
Copy link
Collaborator

@kzar kzar left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but just want to check about daylight savings.

@kzar
Copy link
Collaborator

kzar commented Feb 13, 2025

Do we need to worry about devices that are already enrolled as well?

@sammacbeth
Copy link
Collaborator Author

Do we need to worry about devices that are already enrolled as well?

No, before we release this we'll shut down the current experiment, then start a new one, so no need to fix existing enrollments.

@sammacbeth sammacbeth requested a review from kzar February 17, 2025 13:04
@sammacbeth sammacbeth merged commit 4f93b96 into main Feb 17, 2025
25 checks passed
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