Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept older temporary period archives #11972

Closed
wants to merge 1 commit into from
Closed

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Aug 23, 2017

When browser archiving is disabled, and it is eg shortly after midnight but no new period archive has been processed yet, the reports for week / month / year will all show zeros. This is because we only accept temporary archives from the same day.

However, if it is just after midnight, or an hour after midnight, any archive that is like only a few hours old is still accurate enough and potentially even more accurate then any archive that was fetched eg at 4pm and generated at 8am (which we would still consider acceptable currently as it is generated on the same day).

I went with 12 hours for now. Could also only allow up to 6 hours back but someone might actually configure larger processing intervals via #11965 in the future and therefore 12 should be better. Ideally in #11965, if someone sets eg 24 hours for "yearly periods", then we would adjust this number to 24 hours back.

I tested it locally and worked for me but not sure re any possible side effects.

When browser archiving is disabled, and it is eg shortly after midnight but no new period archive has been processed yet, the reports for week / month / year will all show zeros. This is because we only accept temporary archives from the same day. 

However, if it is just after midnight, or an hour after midnight, any archive that is like only a few hours old is still accurate enough and potentially even more accurate then any archive that was fetched eg at 4pm and generated at 8am (which we would still consider acceptable currently as it is generated on the same day).

I went with 12 hours for now. Could also only allow up to 6 hours back but someone might actually configure larger processing intervals via #11965 in the future and therefore 12 should be better. Ideally in #11965, if someone sets eg 24 hours for "yearly periods", then we would adjust this number to 24 hours back.
@tsteur tsteur added this to the 3.1.0 milestone Aug 23, 2017
@tsteur
Copy link
Member Author

tsteur commented Aug 23, 2017

Moved it into 3.1.0 but feel free to move it to 3.2.0

@tsteur tsteur added Needs Review PRs that need a code review Bug For errors / faults / flaws / inconsistencies etc. labels Aug 23, 2017
@tsteur
Copy link
Member Author

tsteur commented Aug 23, 2017

Looking at the code again it should be quite a save merge as it is only executed when archiving is disabled anyway.

@mattab
Copy link
Member

mattab commented Sep 5, 2017

-> I cannot reproduce the issue locally, in my tests a 2 day old temporary month archive is loaded anyway. Wondering if i'm doing something wrong or if the issue we experienced of a "no data report" after midnight is due to some other problem?

It would have made total sense that this should work but somehow I couldn't figure it out and then realised that this getMinTimeProcessedForTemporaryArchive function is only called for a process whcih can trigger archiving:

getMinTimeProcessedForTemporaryArchive called by
getMinTimeArchiveProcessed called by
loadExistingArchiveIdFromDb called by
prepareArchive called by
cacheArchiveIdsAfterLaunching
which is only called when archiving is enabled for current request:

            if (!Rules::isArchivingDisabledFor($this->params->getIdSites(), $this->params->getSegment(), $this->getPeriodLabel())) {
                $this->cacheArchiveIdsAfterLaunching($archiveGroups, $plugins);
            } else {
                $this->cacheArchiveIdsWithoutLaunching($plugins);
            }

In the code path for "Browser archiving is disabled", then the code already loads the last available archive and it could even be a few days old it seems?!

@tsteur
Copy link
Member Author

tsteur commented Sep 5, 2017

I totally get what you mean. It seems to never even go into that part of the method. I think when I tested it, I set in that method archivingDisabled=false and then it worked but when disabling archiving properly it does not even go in there.

I wonder if the merge still makes sense. I tested also with browser archiving enabled and then it doesn't go into that path either because archiving needs to be first enabled. I think it may go in there though if somehow:

  • Archiving is disabled but a range is requested and archiving_range_force_on_browser_request is enabled. (couldn't reproduce that though)
  • Archiving is enabled but a range is requested and archiving_range_force_on_browser_request is disabled. (couldn't reproduce that though)
  • If somehow for some periods archiving is enabled but not for others eg in here: https://github.com/piwik/piwik/blob/3.1.0-rc1/core/Archive.php#L669-L674 as we loop through all sites and all periods

Probs be save to just close the issue for now.

FYI: Tried to find out why it was initially added but couldn't find it in eg dd2140c

@mattab mattab removed this from the 3.3.0 milestone Oct 6, 2017
@mattab mattab added this to the Priority Backlog (Help wanted) milestone Oct 19, 2017
@tsteur
Copy link
Member Author

tsteur commented Jul 22, 2019

No longer needed since we will be removing temporary archives.

@tsteur tsteur closed this Jul 22, 2019
@tsteur tsteur deleted the temparchiveaccept branch July 22, 2019 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants