@tsteur opened this Pull Request on October 29th 2021 Member

Description:

While we looked into the goal range issue this looked potentially like a bug to me.

There is a return false; but at the end of the method also a return false; so I was wondering why the check was there in the first place as it would return false anyway.

Then looked at the check $minDatetimeArchiveProcessedUTC && Date::factory($tsArchived)->isEarlier($minDatetimeArchiveProcessedUTC) and if I read this right, if the archive was created before the minimum date, then it doesn't force archive invalidation but probably it should.

Could maybe someone have a look that this PR looks correct? Could think about adding a test but this might be tricky. I haven't even tried to reproduce the issue due to lack of time. I would think that

  • If browser archiving is enabled
  • And you view an older period
  • And you are viewing today
  • And there is already an archive that is older than the configured [General]time_before_today_archive_considered_outdated setting

Then I would expect that maybe Matomo falsely isn't archiving again meaning effectively Matomo will only archive again if the report is being requested the next day again. But only max once per day maybe.

Review

@sgiehl commented on October 29th 2021 Member

From a logically point of view that change makes sense to me. maybe @diosmosis can quickly confirm that

@diosmosis commented on October 29th 2021 Member

@sgiehl yes, makes sense to me too :+1:

@tsteur commented on October 31st 2021 Member

I was actually trying to reproduce that issue but couldn't.

The minArchiveTime was already handled in https://github.com/matomo-org/matomo/blob/4.6.0-b2/core/DataAccess/ArchiveSelector.php#L114-L118

As you can see the first parameter is false for $idArchive in that case. However, this method shouldForceInvalidatedArchive will be only called when $idArchive is not empty in https://github.com/matomo-org/matomo/blob/4.6.0-b2/core/ArchiveProcessor/Loader.php#L128-L132

Meaning when the archive is older than the min time, it will actually never go into that code.

None of the previous return statements in Loader.php return a non-empty idArchive so I'm thinking we could even remove that if statement where I made the change. Or we could just merge as a fallback as it might make sense.

It was added in https://github.com/matomo-org/matomo/pull/17379/files for ranges. Also tried to reproduce it for ranges where it includes today, but failed to reproduce to get it into that method.

Also tested eg with various time_before_range_archive_considered_outdated = 1|900|-1 settings.

I suppose we could just merge it and it'll be safe but not entirely sure why it is there.

@sgiehl commented on November 2nd 2021 Member

Guess having it as fallback might not hurt. We could maybe add a comment there, so someone looking at that code might directly notice it might be unneeded.
@tsteur could we maybe add some logging on cloud within that if, to check if it is ever triggered there?

@tsteur commented on November 2nd 2021 Member

Good idea @sgiehl added it. Will check in the evening if anything came up.

@tsteur commented on November 3rd 2021 Member

No log so far, will check again tomorrow

@tsteur commented on November 3rd 2021 Member

@sgiehl ran this for almost 24 hours and it was not triggered once. I'll check again tomorrow

@tsteur commented on November 4th 2021 Member

@sgiehl last 24 hours it was again not logged. I can check again next Monday. On Cloud this would go in there only for ranges. Possible they aren't being invalidated that often or so.

@tsteur commented on November 4th 2021 Member

AKA it might be more reproducible that the code is needed on a Matomo where browser archiving is enabled. Which it isn't on Cloud

@tsteur commented on November 7th 2021 Member

Still nothing in the logs @sgiehl but then we also don't have browser archiving enabled.

we could either remove the if check (as it currently does nothing anyway) or merge the PR.

as it the check was added I'm assuming there was a reason for it and we'd need to change to return true but I don't really have any preference there.

@sgiehl commented on November 8th 2021 Member

I'll simply merge it.

This Pull Request was closed on November 8th 2021
Powered by GitHub Issue Mirror