Log importer should check success from tracking api http status code

[mattab]

Goal of this issue is to make the Log importer check whether the tracking api were a success based on the http status code returned from tracking API. Currently it checks that the response is valid json: matomo-org/matomo@a85c83b but this does not work with QueuedTracking: matomo-org/plugin-QueuedTracking#5

Follows up matomo-org/matomo#6982

[tsteur]

It should only consider a 2XX response as success. Currently we usually return a 400 or 500 response code in case of any failure. Other plugins might return other errors in the future.

Checking for response format doesn't really work since there might be plugins in the future that return a XML response in tracker etc. It so not yet public API for 3rd party developers but we do this kinda stuff already in QueuedTracking plugin.

[diosmosis]

a4d1eea refs this

@mattab Can you check if you still get the error? If not, you can close this.

[mattab]

@diosmosis is it possible to check for HTTP status code rather than Gif OR Json?

[diosmosis]

This is done by urllib2. An exception is raised for non-success codes. Not checking response will result in silent failure if BulkTracking is not enabled.

[mattab]

Makes sense - it's working 👍

[tsteur]

In the future there might be as well XML response etc. Also the QueuedTracker plugin might return a 204 response code in case it was tracked via JavaScript (URL parameter send_image=0). I think this could again cause issues when using "--replay-tracking"?

If BulkTracking is used but not enabled, it should jump into this line here (did a quick test): https://github.com/piwik/piwik/blob/master/core/Tracker/Response.php#L78

Maybe we should return a response code "400 Bad Request" in this case there? This would mean such a case should be detect as well by urllib2

[diosmosis]

The 400 code is for bad requests sent by the client, so I think it's not fitting here. What about 501 (Not Implemented), since there is no tracker Handler implementation to use?

[tsteur]

I think a 400 code would be correct as it is a bad request sent by the client. It misses a tracking parameter. Not implemented is not 100% true as we always fallback to the default handler which uses $_GET+$_POST. If you prefer a 501 that could be ok as well though

[diosmosis]

What tracking parameter is missing? I am considering the case for the original change to check the tracker response, which was that BulkTracking was disabled causing the tracker to not track and the log importer to fail silently.

[tsteur]

Sorry I meant for this:

If BulkTracking is used but not enabled, it should jump into this line here (did a quick test): https://github.com/piwik/piwik/blob/master/core/Tracker/Response.php#L78 Maybe we should return a response code "400 Bad Request" in this case there? This would mean such a case should be detect as well by urllib2

When it jumps into this line, it kinda must be a bad request as no requests are given. A "Not implemented" can be under circumstances correct as well but it depends. Reading the spec a 501 should be returned if the HTTP method (GET, POST, PUT, DELETE, ...) is not supported (I might be wrong). If one sends an empty GET request to piwik.php the HTTP method is actually supported but it misses URL tracking parameters so it is rather a 400 I reckon.