@tsteur opened this Pull Request on February 25th 2022 Member

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 commented on March 1st 2022 Member

@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 commented on March 2nd 2022 Member

@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 commented on March 2nd 2022 Member

👍 That should be fixed now @sgiehl

@sgiehl commented on March 3rd 2022 Member

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:
https://github.com/matomo-org/matomo/blob/285bbfd022e2e23728daa453b6876d51061af018/plugins/Live/Controller.php#L169

@tsteur commented on March 3rd 2022 Member

@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 commented on March 4th 2022 Member

@tsteur but VisitorProfile::handleAdjacentVisitorIds() calls Live/Model::queryAdjacentVisitorId, which has been changed:
https://github.com/matomo-org/matomo/blob/115527353a9e75e01aa4d263408956ae45403bea/plugins/Live/VisitorProfile.php#L94-L97

@tsteur commented on March 7th 2022 Member
@sgiehl commented on March 7th 2022 Member

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

https://github.com/matomo-org/matomo/blob/8770ed05a81c9f47c28231bdbc63693cfe2a09c0/plugins/Live/API.php#L220

image

@tsteur commented on March 7th 2022 Member

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

@sgiehl commented on March 8th 2022 Member

@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 commented on March 8th 2022 Member
This Pull Request was closed on March 8th 2022
Powered by GitHub Issue Mirror