[manager] bugfix: fix incorrect status page date calculation#3962
Conversation
|
Hi maintainers 👋 This PR fixes the incorrect date calculation on the Status Page (#3076). All hertzbeat-manager tests pass locally. |
|
Hi, @BhanuNidumolu Welcome. This PR seems to handle different issues simultaneously. We recommend that one PR only fix one problem, which will be helpful for code review. BTW, could you please provide some unit test codes or test screenshots? |
|
Thanks for the review and the suggestion! I will split them into two separate PRs to make review easier: • This PR (#3962) will only keep the Status Page date bugfix Regarding testing:
I will update this PR shortly. Thanks for the guidance! |
09611da to
2fffea0
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes critical date calculation bugs in the Status Page service that could cause incorrect time ranges at midnight, incorrect day boundaries during DST transitions, and integer overflow in timestamp sorting.
Changes:
- Replaced
LocalDateTimewithInstantandZonedDateTimefor proper timezone handling - Fixed midnight race conditions by capturing a single
Instantfor all date calculations - Corrected timestamp sorting to use
Long.compare()instead of integer casting - Improved code readability with better variable naming and formatting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ZoneId zoneId = ZoneId.systemDefault(); | ||
|
|
||
| Instant now = Instant.now(); | ||
| long nowTimestamp = now.toEpochMilli(); | ||
|
|
||
| long todayStartTimestamp = now | ||
| .atZone(zoneId) | ||
| .toLocalDate() | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
| List<StatusPageHistory> todayStatusPageHistoryList = statusPageHistoryDao | ||
| .findStatusPageHistoriesByComponentIdAndTimestampBetween(component.getId(), todayStartTimestamp, nowTimestamp); | ||
| StatusPageHistory todayStatus = combineOneDayStatusPageHistory(todayStatusPageHistoryList, component, nowTimestamp); | ||
| histories.add(todayStatus); | ||
| // query 30d component status history | ||
| LocalDateTime preTime = todayStartTime.minusDays(HISTORY_SPAN_DAYS); | ||
| long preTimestamp = preTime.toInstant(zoneOffset).toEpochMilli(); | ||
| long preTimestamp = now | ||
| .atZone(zoneId) | ||
| .toLocalDate() | ||
| .minusDays(HISTORY_SPAN_DAYS) | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
|
|
||
| List<StatusPageHistory> history = statusPageHistoryDao | ||
| .findStatusPageHistoriesByComponentIdAndTimestampBetween(component.getId(), preTimestamp, todayStartTimestamp); | ||
| LinkedList<StatusPageHistory> historyList = new LinkedList<>(history); | ||
| historyList.sort((o1, o2) -> (int) (o1.getTimestamp() - o2.getTimestamp())); | ||
| LocalDateTime endTime = todayStartTime.minusSeconds(1); | ||
| LocalDateTime startTime = endTime.withHour(0).withMinute(0).withSecond(0).withNano(0); | ||
| for (int index = 0; index < HISTORY_SPAN_DAYS; index++) { | ||
| long startTimestamp = startTime.toInstant(zoneOffset).toEpochMilli(); | ||
| long endTimestamp = endTime.toInstant(zoneOffset).toEpochMilli(); | ||
| List<StatusPageHistory> thisDayHistory = historyList.stream().filter(item -> | ||
| item.getTimestamp() >= startTimestamp && item.getTimestamp() <= endTimestamp) | ||
| .collect(Collectors.toList()); | ||
| historyList.sort((o1, o2) -> Long.compare(o1.getTimestamp(), o2.getTimestamp())); | ||
| ZonedDateTime end = Instant.ofEpochMilli(todayStartTimestamp) | ||
| .atZone(zoneId) | ||
| .minusSeconds(1); // yesterday 23:59:59 local time | ||
|
|
||
| for (int i = 0; i < HISTORY_SPAN_DAYS; i++) { | ||
| long endTimestamp = end.toInstant().toEpochMilli(); | ||
|
|
||
| long startTimestamp = end.toLocalDate() | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
| List<StatusPageHistory> thisDayHistory = historyList.stream() | ||
| .filter(h -> h.getTimestamp() >= startTimestamp && h.getTimestamp() <= endTimestamp) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| if (thisDayHistory.isEmpty()) { | ||
| StatusPageHistory statusPageHistory = StatusPageHistory.builder().timestamp(endTimestamp) | ||
| .componentId(component.getId()).state(CommonConstants.STATUS_PAGE_COMPONENT_STATE_UNKNOWN).build(); | ||
| histories.add(statusPageHistory); | ||
| histories.add(StatusPageHistory.builder() | ||
| .timestamp(endTimestamp) | ||
| .componentId(component.getId()) | ||
| .state(CommonConstants.STATUS_PAGE_COMPONENT_STATE_UNKNOWN) | ||
| .build()); | ||
| } else if (thisDayHistory.size() == 1) { | ||
| histories.add(thisDayHistory.get(0)); | ||
| } else { | ||
| StatusPageHistory statusPageHistory = combineOneDayStatusPageHistory(thisDayHistory, component, endTimestamp); | ||
| histories.add(statusPageHistory); | ||
| StatusPageHistory merged = | ||
| combineOneDayStatusPageHistory(thisDayHistory, component, endTimestamp); | ||
| histories.add(merged); | ||
| statusPageHistoryDao.deleteAll(thisDayHistory); | ||
| statusPageHistoryDao.save(statusPageHistory); | ||
| statusPageHistoryDao.save(merged); | ||
| } | ||
| startTime = startTime.minusDays(1); | ||
| endTime = endTime.minusDays(1); | ||
|
|
||
| end = end.minusDays(1); | ||
| } |
There was a problem hiding this comment.
The date calculation logic in queryComponentsStatus has been significantly refactored to fix midnight race conditions and DST issues, but there are no unit tests covering this critical logic. Consider adding tests that verify the date calculations work correctly, especially for edge cases like midnight boundaries, DST transitions, and the 29-day history window calculation.
| ZoneId zoneId = ZoneId.systemDefault(); | ||
|
|
||
| Instant now = Instant.now(); | ||
| long nowTimestamp = now.toEpochMilli(); | ||
|
|
||
| long todayStartTimestamp = now | ||
| .atZone(zoneId) | ||
| .toLocalDate() | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
| // Today | ||
| List<StatusPageHistory> todayStatusPageHistoryList = | ||
| statusPageHistoryDao.findStatusPageHistoriesByComponentIdAndTimestampBetween( | ||
| component.getId(), todayStartTimestamp, nowTimestamp); | ||
|
|
||
| StatusPageHistory todayStatus = | ||
| combineOneDayStatusPageHistory(todayStatusPageHistoryList, component, nowTimestamp); | ||
|
|
||
| histories.add(todayStatus); | ||
| // query 30d component status history | ||
| LocalDateTime preTime = todayStartTime.minusDays(HISTORY_SPAN_DAYS); | ||
| long preTimestamp = preTime.toInstant(zoneOffset).toEpochMilli(); | ||
| List<StatusPageHistory> history = statusPageHistoryDao | ||
| .findStatusPageHistoriesByComponentIdAndTimestampBetween(component.getId(), preTimestamp, todayStartTimestamp); | ||
|
|
||
| // Previous 29 days | ||
| long preTimestamp = now | ||
| .atZone(zoneId) | ||
| .toLocalDate() | ||
| .minusDays(HISTORY_SPAN_DAYS) | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
|
|
||
| List<StatusPageHistory> history = | ||
| statusPageHistoryDao.findStatusPageHistoriesByComponentIdAndTimestampBetween( | ||
| component.getId(), preTimestamp, todayStartTimestamp); | ||
|
|
||
| LinkedList<StatusPageHistory> historyList = new LinkedList<>(history); | ||
| historyList.sort((o1, o2) -> (int) (o1.getTimestamp() - o2.getTimestamp())); | ||
| LocalDateTime endTime = todayStartTime.minusSeconds(1); | ||
| LocalDateTime startTime = endTime.withHour(0).withMinute(0).withSecond(0).withNano(0); | ||
| for (int index = 0; index < HISTORY_SPAN_DAYS; index++) { | ||
| long startTimestamp = startTime.toInstant(zoneOffset).toEpochMilli(); | ||
| long endTimestamp = endTime.toInstant(zoneOffset).toEpochMilli(); | ||
| List<StatusPageHistory> thisDayHistory = historyList.stream().filter(item -> | ||
| item.getTimestamp() >= startTimestamp && item.getTimestamp() <= endTimestamp) | ||
| .collect(Collectors.toList()); | ||
| historyList.sort((o1, o2) -> Long.compare(o1.getTimestamp(), o2.getTimestamp())); | ||
|
|
||
| ZonedDateTime end = Instant.ofEpochMilli(todayStartTimestamp) | ||
| .atZone(zoneId) | ||
| .minusSeconds(1); // yesterday 23:59:59 local time | ||
|
|
||
| for (int i = 0; i < HISTORY_SPAN_DAYS; i++) { | ||
| long endTimestamp = end.toInstant().toEpochMilli(); | ||
|
|
||
| long startTimestamp = end.toLocalDate() | ||
| .atStartOfDay(zoneId) | ||
| .toInstant() | ||
| .toEpochMilli(); | ||
|
|
||
| List<StatusPageHistory> thisDayHistory = historyList.stream() | ||
| .filter(h -> h.getTimestamp() >= startTimestamp && h.getTimestamp() <= endTimestamp) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| if (thisDayHistory.isEmpty()) { | ||
| StatusPageHistory statusPageHistory = StatusPageHistory.builder().timestamp(endTimestamp) | ||
| .componentId(component.getId()).state(CommonConstants.STATUS_PAGE_COMPONENT_STATE_UNKNOWN).build(); | ||
| histories.add(statusPageHistory); | ||
| histories.add(StatusPageHistory.builder() | ||
| .timestamp(endTimestamp) | ||
| .componentId(component.getId()) | ||
| .state(CommonConstants.STATUS_PAGE_COMPONENT_STATE_UNKNOWN) | ||
| .build()); | ||
| } else if (thisDayHistory.size() == 1) { | ||
| histories.add(thisDayHistory.get(0)); | ||
| } else { | ||
| StatusPageHistory statusPageHistory = combineOneDayStatusPageHistory(thisDayHistory, component, endTimestamp); | ||
| histories.add(statusPageHistory); | ||
| StatusPageHistory merged = | ||
| combineOneDayStatusPageHistory(thisDayHistory, component, endTimestamp); | ||
| histories.add(merged); | ||
| statusPageHistoryDao.deleteAll(thisDayHistory); | ||
| statusPageHistoryDao.save(statusPageHistory); | ||
| statusPageHistoryDao.save(merged); | ||
| } | ||
| startTime = startTime.minusDays(1); | ||
| endTime = endTime.minusDays(1); | ||
|
|
||
| end = end.minusDays(1); | ||
| } |
There was a problem hiding this comment.
The date calculation logic in queryComponentStatus has been significantly refactored to fix midnight race conditions and DST issues, but there are no unit tests covering this critical logic. Consider adding tests that verify the date calculations work correctly, especially for edge cases like midnight boundaries, DST transitions, and the 29-day history window calculation.
…ervice/impl/StatusPageServiceImpl.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tomsun28 <tomsun28@outlook.com>
…ervice/impl/StatusPageServiceImpl.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tomsun28 <tomsun28@outlook.com>
…ervice/impl/StatusPageServiceImpl.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tomsun28 <tomsun28@outlook.com>
…ervice/impl/StatusPageServiceImpl.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tomsun28 <tomsun28@outlook.com>
|
hi thanks! Can you help to add more unit test for these? |
|
Thanks for the review! I will add unit tests to cover:
I’ll push the tests shortly. |
|
Added unit tests covering:
All tests pass locally. Thanks for the review! |
|
hi LGTM! thanks. |
|
License header added as requested. All checks should now pass. |




What's changed?
This PR fixes incorrect date calculation on the Status Page (#3076).
Details
Instantfor bothnowandtodayStartLocalDateTime + ZoneOffsetlogic withInstant + ZoneIdLong.compareto avoid integer overflowThis prevents:
Checklist
Add or update API