https://github.com/matomo-org/matomo/pull/17427 - Mouve

Site d'origine

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

Performance timing API as default #17427

Merged
merged 10 commits into from Apr 14, 2021
Merged

Conversation

@flamisz
Copy link
Contributor

@flamisz flamisz commented Apr 7, 2021

Description:

fixes #17259

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed
@flamisz flamisz added this to the 4.3.0 milestone Apr 7, 2021
@flamisz flamisz self-assigned this Apr 7, 2021
@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 7, 2021

@flamisz I think for that issue, based on a slack discussion, we were going to use the old API by default and switch to the new one only if the old one wasn't available to support older browsers that may not support the new API (eg, on mobile devices).

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 7, 2021

@flamisz as suggested in #17259 (comment) wondering if it makes sense to prefer reading data from the older API and only fallback to the newer API if the older doesn't exist anymore for reasons mentioned in the comment and also in case eg some browsers now have not responseEnd but domLoading etc. It seems like a safer way.

Additionally would need to check if performanceAlias.timing API has responseEnd or if a different property needs to be used there (domLoading).

Also to fully fix the issue need to add the rounding to integer values (see eg #17259 (comment) and #17259 (comment) as the newer API returns floats.).

And we'd also need to check it eg with iPad running iOS9.3 with Safari 9 if any possible to see if the newer API behaves faulty there maybe as we say in our analytics some funny numbers there (we have browserstack account for this)

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 7, 2021

@tsteur and @diosmosis thanks for the review.

The performance.timing API contains responseEnd, we already use it for another data.

I can make the old one default and round them integer (interestingly Firefox uses integer, but Chrome does not).

I thought now we know what is the main issue, we'd like to use the new API because we talked about using the old one by default before @sgiehl found the problem, that's why I didn't change that part, but I'll do it now.

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 7, 2021

The rounding was already solved in this PR #17372

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 7, 2021

build js

1 similar comment
@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 7, 2021

build js

@flamisz flamisz changed the title Use domLoading instead of responseEnd in JS tracking client Performance timing API as default Apr 7, 2021
js/piwik.js Outdated Show resolved Hide resolved
@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 7, 2021

build js

@sgiehl
sgiehl approved these changes Apr 9, 2021
Copy link
Member

@sgiehl sgiehl left a comment

Seems to work as expected now. Tested it in Chrome, Firefox and IE

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 12, 2021

@flamisz looks like the JS tests aren't passing (JSLint failing), can you take a look? Would be ready to merge afterwards.

@flamisz flamisz force-pushed the 17259-performance-metric-changes branch from b075af4 to 56ac8be Apr 12, 2021
@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 12, 2021

jslint error:

if ('domLoading' in performanceData)
Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead.
@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 12, 2021

@flamisz is it possible to disable jslint for that line?

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 12, 2021

@flamisz is it possible to disable jslint for that line?

@diosmosis I can use undefined (newest commit) or try to disable for that line, but this is from the jslint doc:

JSLint was designed to reject code that some would consider to be perfectly fine...

...JSLint will hurt your feelings. Side effects may include headache, irritability, ...

😄

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 12, 2021

build js

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 13, 2021

@flamisz just had a thought, I'm not sure what valid values are acceptable for this code, but are null values accepted here or should we also check for them? (I know the previous code would have let them pass, just thought about it now.)

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 13, 2021

btw could maybe just use hasOwnProperty or so instead of ignoring jslint there maybe?

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 13, 2021

@flamisz just had a thought, I'm not sure what valid values are acceptable for this code, but are null values accepted here or should we also check for them? (I know the previous code would have let them pass, just thought about it now.)

@diosmosis it returns a unsigned long long, it can be 0 but not null.

btw could maybe just use hasOwnProperty or so instead of ignoring jslint there maybe?

@tsteur I used the undefined check instead, because the hasOwnProperty tricky sometimes, if it's an inherited property. The undefined should work the same way as the in, because the value itself can't be undefined, only unsigned long long.

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 13, 2021

@tsteur can you look at this comment quickly: #17427 (comment) ?

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 13, 2021

@diosmosis Might be good to test this in IE10 or so (seems pretty much the oldest browser that supports this API see https://caniuse.com/?search=performance ). Not sure if it could potentially result in an error if it wasn't defined etc. I think usually we'd maybe do typeof aaa.bbb !== 'undefined' (the result of typeof would need to be assigned to a variable first). Seems in past we usually used isDefined(aaa.bbb)? No big preference though. For consistency might be better to use isDefined().

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 13, 2021

Hi @flamisz, when you're able to can you take a look at #17427 (comment)? (making this comment since you weren't pinged in thomas' comment)

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 14, 2021

build js

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Apr 14, 2021

@diosmosis and @tsteur I'm using isDefined() now. I haven't seen before we have this function. Make sense to use it.

@diosmosis diosmosis merged commit 086874b into 4.x-dev Apr 14, 2021
0 of 2 checks passed
0 of 2 checks passed
@travis-ci
Travis CI - Branch Build Failed
Details
@travis-ci
Travis CI - Pull Request Build Failed
Details
@diosmosis diosmosis deleted the 17259-performance-metric-changes branch Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants

level de log mis à 1

Récupération du document https://github.com/matomo-org/matomo/pull/17427.

Récupération de l'URL https://github.com/matomo-org/matomo/pull/17427...

Raccourcis

Commandes

Fermer