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

Jump to bottom

Apply max execution query time to Live.getCounters API and queryAdjacentVisitorId method #18852

Merged
merged 10 commits into from Mar 8, 2022
Merged

Apply max execution query time to Live.getCounters API and queryAdjacentVisitorId method #18852

merged 10 commits into from Mar 8, 2022

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Feb 25, 2022 •
edited

Description:

Noticed the max execution query configuration is not applied to live counters which it is now. When the query takes too long, then it will show a dash and a tooltip explaining that it took too long to get the data. The message may vary depending if a segment is selected or not
image

For queryAdjacentVisitorId if there's a timeout we simply assume that no next or previous visitor was found.

Checked https://matomo.org/faq/how-to/how-can-i-automatically-stop-long-running-database-queries/ which doesn't need to be updated by the looks.

Review

@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Feb 25, 2022
@tsteur tsteur added this to the 4.8.0 milestone Feb 25, 2022
@tsteur tsteur changed the title Livecountertime Apply max execution query time to Live.getCounters API Feb 25, 2022
@tsteur tsteur added Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review labels Feb 26, 2022
@tsteur tsteur changed the title Apply max execution query time to Live.getCounters API Apply max execution query time to Live.getCounters API and queryAdjacentVisitorId method Feb 26, 2022
@tsteur tsteur added the Stability For issues that make Matomo more stable and reliable to run for sys admins. label Feb 26, 2022
@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Feb 27, 2022
@justinvelluppillai justinvelluppillai modified the milestones: 4.8.0, 4.9.0 Mar 1, 2022
sgiehl
sgiehl approved these changes Mar 1, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look through the code. Everything seems to make sense. New tests have been added and all tests still seem to pass. Should be good to merge.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 1, 2022
@tsteur
Copy link
Member Author

tsteur commented Mar 1, 2022

@sgiehl can you confirm you tested it and not just looked at the code? I tested it too but be good to have confirmation

@sgiehl
Copy link
Member

sgiehl commented Mar 2, 2022

@tsteur Yes, local testing is always part of a review. But I actually only had a look at the "Visits in Real-time" widget.

What could be improved would be the error handling of the "Real Time Visitor Count" widget. As it currently shows this when it exceeds the execution time:
image

And if the first request works but a subsequent request fails, it shows a global notification with the error message.

@tsteur
Copy link
Member Author

tsteur commented Mar 2, 2022

👍 That should be fixed now @sgiehl

@tsteur tsteur added the Needs Review PRs that need a code review label Mar 2, 2022
@sgiehl
Copy link
Member

sgiehl commented Mar 3, 2022

Found another widget that is affected by the change. We have a widget Visitor profile, which currently has the same behavior as the Real time visitor count before.
I guess the problem is that the API method to get the profile throws a execution time error while fetching the latest visitorid, which isn't handled in the controller here:

matomo/plugins/Live/Controller.php

Line 169 in 285bbfd

$visitorData = Request::processRequest('Live.getVisitorProfile');

@tsteur
Copy link
Member Author

tsteur commented Mar 3, 2022

@sgiehl I couldn't reproduce this one, or at least not due to this change. It might be an existing issue? We could create a new issue for it? The visitor query is handled in the private Live/Model::handleAdjacentVisitorIds() query so I don't think it can cause an issue and couldn't reproduce it for that query that was changed.

@sgiehl
Copy link
Member

sgiehl commented Mar 4, 2022

@tsteur but VisitorProfile::handleAdjacentVisitorIds() calls Live/Model::queryAdjacentVisitorId, which has been changed:

matomo/plugins/Live/VisitorProfile.php

Lines 94 to 97 in 1155273

$this->profile['nextVisitorId'] = $model->queryAdjacentVisitorId($this->idSite, $visitorId,
$latestVisitTime, $segment, $getNext = true);
$this->profile['previousVisitorId'] = $model->queryAdjacentVisitorId($this->idSite, $visitorId,
$latestVisitTime, $segment, $getNext = false);

@tsteur
Copy link
Member Author

tsteur commented Mar 7, 2022

@sgiehl this should be handled in https://github.com/matomo-org/matomo/pull/18852/files#diff-e3bdcefa5a3431d680f8e0d579f6d4f9be13cc944663b725983b7a703465d6aeR94 ? Or is that not working for you?

@sgiehl
Copy link
Member

sgiehl commented Mar 7, 2022

Had another look at that. I'm still seeing the error. Debugged it though, and the problem seems to be this:

matomo/plugins/Live/API.php

Line 220 in 8770ed0

$visitorId = $this->getMostRecentVisitorId($idSite, $segment);

image

@tsteur
Copy link
Member Author

tsteur commented Mar 7, 2022

@sgiehl that would be an existing error and not related to this PR?

@sgiehl
Copy link
Member

sgiehl commented Mar 8, 2022

@tsteur Than it might be a regression from the first PR introducing the query time limit for live reports.
Feel free to fix it here. Or merge this one and create a follow up issue.

@tsteur tsteur merged commit 05397a2 into 4.x-dev Mar 8, 2022
@tsteur tsteur deleted the livecountertime branch March 8, 2022 18:32
@tsteur tsteur mentioned this pull request Mar 8, 2022
Open
@tsteur
Copy link
Member Author

tsteur commented Mar 8, 2022

created #18900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review Stability For issues that make Matomo more stable and reliable to run for sys admins.
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsteur @sgiehl @justinvelluppillai