Skip to content

LG-10546: make created at unix at form#8922

Merged
mdiarra3 merged 5 commits intomainfrom
LG-10374-make-created-at-unix
Aug 10, 2023
Merged

LG-10546: make created at unix at form#8922
mdiarra3 merged 5 commits intomainfrom
LG-10374-make-created-at-unix

Conversation

@mdiarra3
Copy link
Contributor

@mdiarra3 mdiarra3 commented Aug 2, 2023

🎫 Ticket

LG-10546

🛠 Summary of changes

Change the log to unix time instead of default datetime format since that is not easily queryable in AWS

Screenshot

Screen Shot 2023-08-02 at 10 54 43 AM

@mdiarra3 mdiarra3 requested a review from a team August 2, 2023 14:13
@soniaconnolly
Copy link
Contributor

This looks like a change to specific analytics fields. I thought you were proposing a more general change that would convert all timestamps. Is this a test for the more general change?

@aduth
Copy link
Contributor

aduth commented Aug 3, 2023

This looks like a change to specific analytics fields. I thought you were proposing a more general change that would convert all timestamps. Is this a test for the more general change?

I'm not familiar with what had been discussed in the engineering huddle, but if we wanted this to be universal, we could consider something which converts date objects to timestamps as part of the serialization logic (in place of .to_json "simple" serialization). Not sure if that's worth it vs. just having a convention to do it manually.

@mdiarra3
Copy link
Contributor Author

mdiarra3 commented Aug 3, 2023

Yea this PR is just a fix for what we specifically were dealing with as a proof of concept for general adoption in a later PR

@mdiarra3 mdiarra3 requested a review from zachmargolis August 7, 2023 16:52
@mdiarra3
Copy link
Contributor Author

mdiarra3 commented Aug 7, 2023

@zachmargolis wanted your opinion on this PR to see if changing dates to unix could be a way forward for us. with storing DateTime in analytic events, that makes it easier query based on the dates.

@mdiarra3 mdiarra3 force-pushed the LG-10374-make-created-at-unix branch from af552b4 to 6b5296a Compare August 10, 2023 14:54
@mdiarra3
Copy link
Contributor Author

Talked to @aduth And I think for now we can just do opt in option and if we want to make it universal that should be a separate ticket

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM

@mdiarra3 mdiarra3 merged commit dc34671 into main Aug 10, 2023
@mdiarra3 mdiarra3 deleted the LG-10374-make-created-at-unix branch August 10, 2023 19:03
@jmdembe jmdembe mentioned this pull request Aug 15, 2023
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.

5 participants