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

Ensure a valid timestamp is set when recording a conversion #15587

Merged
merged 2 commits into from Feb 20, 2020
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Feb 18, 2020

refs matomo-org/matomo-for-wordpress#215

see matomo-org/matomo-for-wordpress#215 (comment)

There should be no harm in this logic. Basically the last visit time is the current timestamp anyway. It be actually probably even more accurate to use the current timestamp there in general.

Eg this could happen if an exception was triggered in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L192-L194 where the visitor was not found for some reason when updating, or when no value changed I suppose because it was updated within a few ms.

Another possibility that may be better be to set the visit_last_action_time https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L249 at the beginning of that function or so. Not sure if there is maybe a reason it's set at the end. Or maybe it could be at least moved up one line before updateExistingVisit https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 so if it triggers an exception then we still have surely a value set. Not sure if it would fix that particular error though. Can't really think of a different way though how this would happen so far.

I was able to trigger the VisitorNotFoundInDb exception when no value changed
image

in
image

To be safe could apply both patches. The one already done in GoalManager and moving up one line setting the last visit time.

refs matomo-org/matomo-for-wordpress#215

see matomo-org/matomo-for-wordpress#215 (comment)

There should be no harm in this logic. Basically the last visit time is the current timestamp anyway. It be actually probably even more accurate to use the current timestamp there in general.

Eg this could happen if an exception was triggered in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L192-L194 where the visitor was not found for some reason when updating, or when no value changed I suppose because it was updated within a few ms. 

Another possibility that may be better be to set the `visit_last_action_time ` https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L249 at the beginning of that function or so. Not sure if there is maybe a reason it's set at the end. Or maybe it could be at least moved up one line before `updateExistingVisit` https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 so if it triggers an exception then we still have surely a value set. Not sure if it would fix that particular error though. Can't really think of a different way though how this would happen so far.

I was able to trigger the VisitorNotFoundInDb exception when no value changed
@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Feb 18, 2020
@tsteur tsteur added this to the 4.0.0 milestone Feb 18, 2020
@sgiehl
Copy link
Member

sgiehl commented Feb 18, 2020

Wondering if we shouldn't change the behavior, so no error is triggered when there actually were no values to update. Maybe we should check if the value actually is different and unset it if not here:

$valuesToUpdate = $this->getExistingVisitFieldsToUpdate($visitIsConverted);
// update visitorInfo
foreach ($valuesToUpdate as $name => $value) {
$this->visitProperties->setProperty($name, $value);
}

@tsteur
Copy link
Member Author

tsteur commented Feb 18, 2020

@sgiehl technically there were values to update but if a request is sent twice within a short time the values might not actually change causing 0 to be returned.

I wonder if this is maybe not an issue in Matomo core because MYSQLI_CLIENT_FOUND_ROWS is set at least for the Mysqli tracker. PDO also seems to set PDO::MYSQL_ATTR_FOUND_ROWS. So this seems like a WordPress only issue since I can't find that flag there and I can't really add that flag.

Will see if I can add that flag to WordPress since there is a MYSQL_CLIENT_FLAGS constant config but this might not always work since a user might define custom flags, and I would need to figure out if WordPress is using mysqli or mysql to set correct constant etc.

I kind of reckon too though we could simply not return an error when updateExistingVisit is not changing any value in model::updateVisit since we can kind of assume it should find that visit. Otherwise it would think it is a new visit anyway. Not sure why it's there.

@tsteur
Copy link
Member Author

tsteur commented Feb 18, 2020

I've had a look and there is no reliable way for me to set the flag without possibly breaking a WordPress installation. I reckon we could either ignore wasInserted and always return true or if $wasInserted = false (which should be rarely the case) we could do a select from where idsite=? and idvisit =? and if the row exists only then return true. Then we could safely assume there was no change and technically the update was successful. This query should be pretty much never executed in On-Premise so it wouldn't be an issue to have it there. Any thoughts?

@sgiehl
Copy link
Member

sgiehl commented Feb 19, 2020

Haven't looked deeper in the code, but I would have tried something like this

 $valuesToUpdate = $this->getExistingVisitFieldsToUpdate($visitIsConverted); 
  
// update visitorInfo 
foreach ($valuesToUpdate as $name => $value) { 
    if ($this->visitProperties->getProperty($name) === $value) {
        unset($valuesToUpdate[$name]);
        continue;
    }
    $this->visitProperties->setProperty($name, $value); 
} 

That might prevent doing an update at all if no values changed. But not sure if that would work.

Your suggestion with trying to fetch the visit if update returns false, sounds fine as well.

@tsteur
Copy link
Member Author

tsteur commented Feb 19, 2020

Cheers @sgiehl I'll have a think in a bit. I reckon it might not work 100% re checking the values like this technically since maybe the value in the DB has changed again between fetching visitor information and updating it. Although this could be ignored probably. I will check that out.

@tsteur
Copy link
Member Author

tsteur commented Feb 20, 2020

@sgiehl couldn't really go with the suggested approach. I could add some of the logic between those lines https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L236-L239 but then in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L244 there would be more values added so it would have values again in the end. And I couldn't really do the check here https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 because the values are always the same because visitProperties are set here: https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L240 I could try creating a copy of visitProperties or compare some values before and after but it's not really reliable to assume what the value in the DB is. Therefore added the hasVisit check and also leaving the goal manager check in there just to be safe.

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.

Looks good now

@tsteur tsteur merged commit 2b64abf into 4.x-dev Feb 20, 2020
@tsteur tsteur deleted the wp_215 branch February 20, 2020 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants