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

[ALS-5755] Switch time series processor to ISO timestamps #101

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

Luke-Sikina
Copy link
Member

  • Make service that does this
  • Isolate time series logic a bit more
  • Tests

@Luke-Sikina Luke-Sikina added the enhancement New feature or request label Jan 23, 2024
@Service
public class TimeSeriesConversionService {

public String toISOString(Long unixTimeStamp, TimeZone timeZone) {
Copy link
Contributor

@ramari16 ramari16 Jan 23, 2024

Choose a reason for hiding this comment

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

I don't love the idea of converting these to UTC, or them being time-zoned at all, but that's probably out of the scope of this ticket. It is something to think about for the future, what do these timestamps represent exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a Unix timestamp; they're UTC or we're not following the standard correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the timezone to be explicit about the output

Copy link
Contributor

Choose a reason for hiding this comment

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

I was kind of asking rhetorically, sometimes these are when the data was processed, sometimes they are when a medication was given or a test result. Which loses meaning if you don't know the local time. I'm guessing 99% of our data is in 4 time zones so currently it's not much of an issue

@Service
public class TimeSeriesConversionService {

public String toISOString(Long unixTimeStamp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want to do this -- we are losing information, the person requesting the data may be better off just getting the unix time stamp, no?

- Make service that does this
- Isolate time series logic a bit more
- Tests
@Luke-Sikina Luke-Sikina force-pushed the iso-timeseries branch 3 times, most recently from 673e15f to 35797d0 Compare March 27, 2024 19:35
@Luke-Sikina Luke-Sikina merged commit 5f08d17 into release Jul 1, 2024
5 checks passed
@Luke-Sikina Luke-Sikina deleted the iso-timeseries branch July 1, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants