Report tracking into wrong Site ID and missing token auth #13493
Conversation
tsteur
added
not-in-changelog
|
@tsteur just thought of another use case which occurs from time to time, if Matomo is in an unstable state, there may be columns missing, this happened to me regularly, such errors: DEBUG: Exception: Error query: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'last_idlink_va' in 'field list' In query: SELECT visit_last_action_time, visit_first_action_time, idvisitor, idvisit, user_id, visit_exit_idaction_url, visit_exit_idaction_name, visitor_returning, visitor_days_since_first, visitor_days_since_order, visitor_count_visits, visit_goal_buyer, location_country, location_region, location_city, location_latitude, location_longitude, referer_name, referer_keyword, referer_type, idsite, visit_entry_idaction_url, visit_total_actions, visit_total_interactions, visit_total_searches, config_device_brand, config_device_model, config_device_type, visit_total_events, visit_total_time, location_ip, location_browser_lang, campaign_content, campaign_id, campaign_keyword, campaign_medium, campaign_name, campaign_source, last_idlink_va, custom_var_k1, custom_var_v1, custom_var_k2, custom_var_v2, custom_var_k3, custom_var_v3, custom_var_k4, custom_var_v4, custom_var_k5, custom_var_v5 FROM log_visit WHERE visit_last_action_time >= ? AND visit_last_action_time <= ? AND idsite = ? AND config_id = ?
DEBUG: ORDER BY visit_last_action_time DESC
DEBUG: LIMIT 1 Parameters: array (
DEBUG: 0 => '2018-10-03 05:21:30',
DEBUG: 1 => '2018-10-03 06:21:30',
DEBUG: 2 => 1,
DEBUG: 3 => '/ƫK����',
DEBUG: )So it would be really great if we could detect these problems and also report them in the page 👍 |
|
Not really so much possible. Cause it was said it is not needed to support anything else and therefore it is not built to include metadata etc. If something like this is possible, it would be only possible to say a column was missing, but not which column or table etc. |
core/Tracker/Request.php
Outdated
| throw new UnexpectedWebsiteFoundException('Invalid idSite: \'' . $idSite . '\''); | ||
| // we allow 0... the request will fail anyway as the site won't exist... allowing 0 will help us | ||
| // reporting this tracking problem as it is a common issue. Otherwise we would not be able to report | ||
| // this problem in tracking failures |
There was a problem hiding this comment.
Does this mean the exception cannot be caught later? If so, could log the failure there.
There was a problem hiding this comment.
Yes it would otherwise not be caught later. and logging wouldn't work as people wouldn't see the logged error. Like nobody would really look there and eg on cloud people wouldn't even be able to see the logs. We have heaps of logs like idsite=0 already and want to make users aware of this problem with this PR. It's really the most common tracking error.
There was a problem hiding this comment.
I meant log through Failures::logFailure(), like catch in Tracker::main() and log that failure. But if it can't be caught then that's not an option I guess.
There was a problem hiding this comment.
Catching it there wouldn't 100% work eg when using bulk request or queued tracking and eg one request contains idsite=0...
The tracker handler starts processing each tracking request within $handler->process in the Tracker.:track method... and basically the only way be to handle it otherwise in Tracker::trackRequest() but by then this error might have already occurred.
plugins/CoreAdminHome/angularjs/trackingfailures/trackingfailures.controller.js
Show resolved
Hide resolved
|
Left some comments & need to resolve some conflicts. Haven't tested locally yet, will do so next review. |
|
@diosmosis applied the review feedback and resolved the conflicts. I might need to update some UI tests... |
|
Left one further comment, but will probably merge this today. if my comment is important can be done in a new PR |
|
@diosmosis just realising a feature to log not auth request regressed due to another PR... because |
|
@diosmosis just pushing a fix, and also fixing the failed system and ui tests with this commit |
|
Looks like there are some other test failures now? the novisit tests are failing (and possibly a unit test). Maybe some others too. |
|
hoping to have fixed them @diosmosis will check the results tomorrow. |
|
this was tricky... hopefully this time it fixes most or all of the tests :) |
|
Will be very useful 👍
Would it be possible to catch these sql error cases, and report them as failures in this new tool's UI? (currently they may go un-detected for a few hours or days until someone realises tracking does not anymore) |
|
No, at least not as part of 3.8 release. |
|
Almost working, looks like BulkTracking & QueuedTracking have some failing integration tests |
|
@diosmosis fixed the bulk and queued tracking tests. Looks like |
fix #12922
I was also thinking about showing a box in the reporting toolbar, eg on the right... but then thought it may not be needed for now and might be rather annoying. May be good to first see how many false reports maybe come in etc.
I reckon it is a good working solution v1 for this tool and see from there how/where it goes. Heaps could be done there.
Todo: We should write FAQ articles for those 2 particular errors and adjust the links in the code