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
Conversation
@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. |
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 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. |
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 |
It seems some (older) browser versions have bugs around the performance timings causing some incorrect numbers 🤷 |
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. |
I actually don't have a Mac. So I can't try to reproduce those problems 🤷 |
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some notes on possible browser bugs here: https://nicj.net/navigationtiming-in-practice/
For Webkit and especially iOS there were bug reports on that: https://bugs.webkit.org/show_bug.cgi?id=188620 https://bugs.webkit.org/show_bug.cgi?id=168055 https://bugs.webkit.org/show_bug.cgi?id=186919
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added a comment
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. |
build js |
build js |
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