@tzhelyazkova opened this Pull Request on August 19th 2018

This fixes issue #13047

The check does not take into consideration range period, where if you have two adjacent days, the difference is 47h59m59s and not a day.
Example: If I want to get archived data from 2018-06-10 to 2018-06-11, I need to check everything between 2018-06-10 00:00:00 and 2018-06-11 23:59:59. The check disregards the time part of the date.
I think this bug was introduced when functions Date::getDateStartUTC and Date::getDateEndUTC were deprecated. In the code they are replaced with getTimestampUTC which doesn't return the same format as the other two. The tests in ArchiveProcessorTest still use the deprecated functions though.

p.s. Apologies for not providing unit tests for this but I couldn't set up phpunit for this project. I also realize this is not the most elegant solution but I didn't want to change too many parts of the code.

@KaiNissen commented on August 20th 2018

Many thanks for digging into this, @tzhelyazkova!

@diosmosis commented on August 30th 2018 Member

Thanks for the PR and research @tzhelyazkova, it's very much appreciated! I think you definitely found a bug here, but I'm not sure it's the cause of @KaiNissen's issue.

The code here is part of the code that aggregates log data: it checks if the period is for a single day & site, and if so aggregates the log data. If it isn't, it means the period spans multiple days and the archiver will initiate archiving for each individual day and then aggregate the results for the whole period.

@KaiNissen's issue sounds like pre-archived data from pre-3.0 versions aren't being found for ranges that consist of two adjacent days, so I'm not sure what effect this will have. @KaiNissen are you able to confirm if this fixes the issue for you?

As for the change, I think it could be fixed a bit more generally by changing the <= to < (so if one day is 2018-06-10 and the end is 2018-06-11, it will know it's not a single day). That way it will also be fixed for non-range periods (plugins are able to add custom periods).

@KaiNissen commented on August 31st 2018

@diosmosis We didn't roll it out on our production system, but after applying this to a test system, I wasn't able to reproduce the issue anymore. So yes, this change seems to fix the problem.

By the way, it affects all data that is older than two weeks, not only what has been archived prior to the upgrade.

@diosmosis commented on August 31st 2018 Member

That's interesting, @KaiNissen, I'm not able to reproduce this on my own. Would you be able to check in your test system if changing the <= to < in https://github.com/matomo-org/matomo/blob/3.x-dev/core/ArchiveProcessor/Parameters.php#L214 also fixes the issue for you?

@mattab commented on October 8th 2018 Member

Hi @KaiNissen @tzhelyazkova could you please check @diosmosis answer in https://github.com/matomo-org/matomo/pull/13315#issuecomment-417787446

or @diosmosis maybe you have some ideas to could get to the bottom of this issue?

@diosmosis commented on October 10th 2018 Member

@mattab I'll create a PR w/ a test for the operator bug which we can merge in 3.6.1. If someone else reports the issue then we can debug again?

@mattab commented on October 10th 2018 Member

sounds good :+1:

@tzhelyazkova commented on October 10th 2018

Hey @mattab @diosmosis
Sorry for the late response. I tried replacing <= with < on my test environment, but it doesn't fix the issue. The matomo version is still 3.5.1.
Also when I print $secondsInPeriod it always seems to be int 0. :confused:
Thanks for looking into the issue!

@diosmosis commented on October 10th 2018 Member

@tzhelyazkova could you see what $period->getRangeString() is?

@tzhelyazkova commented on October 11th 2018

@diosmosis

@tzhelyazkova could you see what $period->getRangeString() is?

It prints a string with the right start and end date I choose as a range period,
e.g. string '2017-02-23,2017-02-24'

Powered by GitHub Issue Mirror