`__archive_fetch` from/until intervals rounding

[albertored]

At the beginning of the function __archive_fetch here there is the following code that rounds from and until times to the granularity of the archive:

  step = archive['secondsPerPoint']
  fromInterval = int(fromTime - (fromTime % step)) + step
  untilInterval = int(untilTime - (untilTime % step)) + step

I do not understand why after this rounding we add step to the result. This will give wrong results.

Take for instance:

• fromTime = 1501639200 (02/08/2017 2:00 AM UTC)
• untilTime = 1501660800 (02/08/2017 8:00 AM UTC)

with a step of 1 hour (that is step = 3660). In this case both fromTime % step and untilTime % step gives 0 as result but since step is then added we return a result for the range 02/08/2017 3:00 AM UTC -- 02/08/2017 9:00 AM UTC

[deniszh]

Didn't check code yet, but please note that ___archive_fetch is *internal* function, and you *shouldn't* use it in your code.

…

On Wed, 2 Aug 2017 at 12:21, Alberto Sartori ***@***.***> wrote: At the beginning of the function __archive_fetch here <https://github.com/graphite-project/whisper/blob/master/whisper.py#L951> there is the following code that rounds from and until times to the granularity of the archive: step = archive['secondsPerPoint'] fromInterval = int(fromTime - (fromTime % step)) + step untilInterval = int(untilTime - (untilTime % step)) + step I do not understand why after this rounding we add step to the result. This will give wrong results. Take for instance: - fromTime = 1501639200 (02/08/2017 2:00 AM UTC) - untilTime = 1501660800 (02/08/2017 8:00 AM UTC) with a step of 1 hour (that is step = 3660). In this case both fromTime % step and untilTime % step gives 0 as result but since step is then added we return a result for the range 02/08/2017 *3:00* AM UTC -- 02/08/2017 *9:00* AM UTC — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#230>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABK51iZSGPnvQQVVTnJpWKbYYCz_lKyBks5sUE2SgaJpZM4Oq48Q> .

[albertored]

Yes, I'm not using it directly. I just tracked down the problem I have to this piece of code.

I'm using graphite smartSummarize function to get daily stats from an archive with 1 hour granularity but I get data at 1:00 AM each day instead of 0:00 AM. Commenting out that + step fixes my issue but I was asking myself the reason of that piece of code. Maybe I'm breaking other things doing this...

[deniszh]

Hmmm... This code is there for a really long time (yes, it was refactored by @DanCech month ago, but + step was there before). I'm more than sure that this variable is there for a reason. But I'm only speculating, didn't make any deep investigation, though.

[DanCech]

I saw the same thing where the from/until timestamps get moved by 1s when I was looking at the code earlier. Removing that breaks all the existing tests but I'm not entirely sure how, as it's not just moving the returned period over by 1s but seems to be more deeply involved in how the data is read from the files. I was intending to take another look at some point, but haven't had time yet.

[azhiltsov]

Some of our users are very annoyed by this behavior. Is there any new discoveries since than?
what is more interesting go-whisper implementation is mimicking it
https://github.com/go-graphite/go-whisper/blob/master/whisper.go#L985

[DanCech]

Digging into the history, the rounding up was added in 2 commits in 2008:

graphite-project/graphite-web@67ca08b#diff-2f03c6086d3709175640ecb480559814R446

graphite-project/graphite-web@c55a869#diff-2f03c6086d3709175640ecb480559814R451

Unfortunately there isn't any context about the reasoning for it, and it looks like it has simply been enshrined in the tests when they were added. I'm assuming the same behavior is present in go-whisper because it is replicating the python behavior.

The problem with changing something at such a base level in whisper that has been around for so long is that it's hard to say what impact it would have on existing users who have adapted to the current behavior or tools using whisper that expect that behavior.

[piotr1212]

I guess it would be time to introduce some api-breaking changes, or at least make it configurable.

[dumbdonkey]

I've met this problem too. I generated one metric every hour (eg: 13:00), when I try to query metrics between 13:00 and 17:00, I just missed the value of 13:00.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.