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

Ignore negative performance timings values #16516

Merged
merged 8 commits into from Oct 6, 2020
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Oct 3, 2020

Tracking currently fails when a negative value is sent. As a negative value shoud not occur at all, I think it should be fine to simply ignore them.

refs https://forum.matomo.org/t/error-query-mysqli-stat-ement-execute-error-out-of-range-value-for-column-time-transfer/38826

@sgiehl sgiehl 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 Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Oct 3, 2020
@sgiehl sgiehl added this to the 4.0.0-RC milestone Oct 3, 2020
@tsteur
Copy link
Member

tsteur commented Oct 3, 2020

@sgiehl is the negative value provided by the browser? what does a negative value mean in this case when the browser does that? If this negative value is set by the user then we would maybe rather want to throw an exception.

@MichaIng
Copy link
Contributor

MichaIng commented Oct 4, 2020

At least this successfully works around the issue. But I agree that it might be worth having a look at why these values even can be negative. I compared the visit IDs of the error log and visitors log and found:

2x iOS Safari 14
5x MacOS Safari 14
ahh one time Firefox mobile on iOS 14
... and so on...

okay the same anonymized user IDs re-appear across multiple visit IDs, I guess it is clear that it is an Apple issue. Not sure if that Firefox was somehow tracked wrong, or if this only shows that Apple users mostly use Safari but it is an Apple issue, not a Safari issue.

@tsteur
Copy link
Member

tsteur commented Oct 4, 2020

In the API let's throw an exception in general and then indeed lets also find out why the browser was sending a negative value and fix that bug and/or not send negative values in the JS

@sgiehl
Copy link
Member Author

sgiehl commented Oct 5, 2020

It seems some (older) browser versions have bugs around the performance timings causing some incorrect numbers 🤷
I'll update the PR, so the javascript won't send any performance timings if one of the timings seems to hold an obviously incorrect value.

@MichaIng
Copy link
Contributor

MichaIng commented Oct 5, 2020

MacOS version were mostly 10.15, a few 10.14, so quite current. However, if you need to to get some more details about the failed visits, let me know, I can also revert the applied changes to get fresh errors.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 5, 2020

I actually don't have a Mac. So I can't try to reproduce those problems 🤷
Maybe someone with MacOS/Safari can check for possible reasons...

@MichaIng
Copy link
Contributor

MichaIng commented Oct 5, 2020

My wife has one, but is working with it nearly the whole day. Will see if I can get her to do a break an browse our website a bid. Also the browser console output might be interesting 🤔.

request += '&pf_net=' + (performanceAlias.timing.connectEnd - performanceAlias.timing.fetchStart);

if (performanceAlias.timing.connectEnd < performanceAlias.timing.fetchStart) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

@sgiehl in this case wouldn't we maybe still add other timings? Or we don't send any info as soon as one value would be negative?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought about that for a while and decided to discard all values if one appears incorrect. Guess the others might not be trustworthy in that case as well.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it depends on when it actually happens. If it's because of caching or so then it might still make sense to report the others if something else is the reason for that then maybe not. I have no big preference though for now but I think ideally we should understand why it happens

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

👍 added a comment

@tsteur
Copy link
Member

tsteur commented Oct 5, 2020

btw @sgiehl we also have browserstack to test on any kind of operating system and browser. It can also resolve your local URLs so it's easy to debug.

@tsteur
Copy link
Member

tsteur commented Oct 6, 2020

build js

@tsteur
Copy link
Member

tsteur commented Oct 6, 2020

build js

@tsteur tsteur merged commit c65e4d4 into 4.x-dev Oct 6, 2020
@tsteur tsteur deleted the performancetimings branch October 6, 2020 19:14
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. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants