Fix initOverTime's calculation of last slot timestamp #1312
Conversation
icyflame
force-pushed
the
update-init-overtime
branch
from
dcad87e to
da8f94f
Compare
|
Thank you for your contribution and sorry for the delay in reviewing. Things are moving a lot slower with the attention being closer to a war that is going on only a few hours by car from here. I'll come back to you PR and do a proper review when time allows. |
initOverTime calculate's the last slot timestamp with the assumption that `OVERTIME_INTERVAL` will allow atleast `OVERTIME_SLOTS` number of slots between now and the last slot in the current hour. This leads it to calculate the timestamp of the last slot, and then assign timestamps to each slot counting down from that timestamp. This logic is not compatible with changes in the OVERTIME_INTERVAL and the MAXLOGAGE variables. Here is a demonstration of a case where this can be shown clearly: ``` OVERTIME_INTERVAL = 60s (Each slot stores 1 minute of data) MAXLOGAGE = 600s (Store date for only 10 minutes) OVERTIME_SLOTS = 11 (MAXLOGAGE/OVERTIME_INTERVAL) ``` - Current time: 10:35 - Last slot time: 10:59:30 - First slot time: 10:48:30 Note that this logic *does* work as expected with the default minimum of 1 hour for the MAXLOGAGE configuration variable. Signed-off-by: Siddharth Kannan <mail@siddharthkannan.in>
icyflame
force-pushed
the
update-init-overtime
branch
from
da8f94f to
06b0606
Compare
|
Thank you for the review, I fixed the indentation, and pushed again with GPG verification for each commit. For piHole 6.0's development / plan / roadmap / timeline, I was wondering where I could follow the development. Is it everything on the |
It's in |
| time_t first_slot_ts = now - now % OVERTIME_INTERVAL + (OVERTIME_INTERVAL / 2); | ||
| time_t last_slot_ts = first_slot_ts + (OVERTIME_SLOTS-1) * OVERTIME_INTERVAL; |
There was a problem hiding this comment.
The logic in this PR is broken, unfortunately, I didn't see this before.
The issue here is, basically, that first_slot_ts should be the one 24 hours in the past, however, it is a timestamp centered at the current timestamp (as the comment correctly says). The last_slot_ts is then 24 hours in the future which is wrong.
This wasn't an issue in the code before the change as the old code was going backwards from now. The new code tries to go forward but start at now causing the history to incorrectly live in the future rather than in the past.
Signed-off-by: DL6ER <dl6er@dl6er.de>
Fix overtime computation logic broken in PR #1312
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/pi-hole-ftl-v5-15-web-v5-12-and-core-v5-10-released/54987/1 |
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/found-database-entries-in-the-future/55333/3 |
By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:
Please submit all pull requests against the
developmentbranch. Failure to do so will delay or deny your requestHow familiar are you with the codebase?: 2
initOverTime calculate's the last slot timestamp with the assumption that
OVERTIME_INTERVALwillallow atleast
OVERTIME_SLOTSnumber of slots between now and the last slot in the currenthour. This leads it to calculate the timestamp of the last slot, and then assign timestamps to each
slot counting down from that timestamp.
This logic is not compatible with changes in the OVERTIME_INTERVAL and the MAXLOGAGE variables. Here is a demonstration of a case where this can be shown clearly:
Current logic:
This PR's proposal:
Note that this logic does work as expected with the default minimum of 1 hour for the MAXLOGAGE
configuration variable.
However, I believe that there is no need to make this part of the code dependent on this minimum. If this part of the code is changed, the macros for OVERTIME_INTERVAL and MAXLOGAGE can be freely changed by users.
This template was created based on the work of
udemy-dl.