@geekdenz opened this Pull Request on August 18th 2021 Contributor

fixes #16859

Description:

Still need to add documentation.

Is it correct to sendRequest immediately?

Are there other parameters we need to take care of?

Review

@tsteur commented on August 18th 2021 Member

Is it correct to sendRequest immediately?

@geekdenz it wouldn't be a separate request immediately but the information would be sent along the next tracking request. It's mentioned in the issue.

@sgiehl commented on August 18th 2021 Member

@geekdenz I guess this might need to be implemented in another way. The setPagePerformanceTiming method might only need to set some internal variables for the performance metrics. And in sendRequest we check if custom values are provided and append them in that case and if not fall back to the auto detection. See
https://github.com/matomo-org/matomo/blob/da4df21e808fa983e32c6402c8f3de90145e0910/js/piwik.js#L3773-L3777

When appending custom values we also need to set performanceTracked = true to ensure the values aren't tracked twice.
Maybe we should also set performanceTracked = false in setPagePerformanceTiming to ensure the values are tracked again when some updates them.

@geekdenz commented on August 23rd 2021 Contributor

build js

@geekdenz commented on August 23rd 2021 Contributor

build js

@geekdenz commented on August 24th 2021 Contributor

build js

@geekdenz commented on August 25th 2021 Contributor

Only failure is now this:

https://app.travis-ci.com/github/matomo-org/matomo/jobs/533572041#L1120-L1122

Looks like it's not caused by my. So OK to merge?

@geekdenz commented on August 25th 2021 Contributor

build js

@geekdenz commented on August 30th 2021 Contributor
@geekdenz commented on August 30th 2021 Contributor

build js

@geekdenz commented on August 31st 2021 Contributor

build js

@geekdenz commented on August 31st 2021 Contributor

Please see the updated branch at https://github.com/matomo-org/test-examples/tree/trackingpageperformance to ease testing for this.

@geekdenz commented on September 7th 2021 Contributor

build js

@geekdenz commented on September 7th 2021 Contributor

Before reviewing, maybe give me another chance to check some details tomorrow (2021-09-08). I'll comment once I've rechecked some old manual checks I have not done yet. Might save us all some time.

@geekdenz commented on September 7th 2021 Contributor

build js

@geekdenz commented on September 7th 2021 Contributor

build js

@geekdenz commented on September 9th 2021 Contributor

Not 100% sure but I think this test failure is probably not related to this issue:
https://app.travis-ci.com/github/matomo-org/matomo/jobs/536091064#L1112-L1128
Maybe something a reviewer would want to consider before approving.

@geekdenz commented on September 9th 2021 Contributor

build js

@geekdenz commented on September 10th 2021 Contributor

build js

@geekdenz commented on September 10th 2021 Contributor

Considerations that went into this:

  • could have an object parameter where only desired parameters are set - but this would add complexity that has minimal positive impact on users
  • could throw an error for each undefined - but this would make code and usage more complicated, need to assume users only pass undefined if intended
@geekdenz commented on September 11th 2021 Contributor

I think there are still some improvements that should be made here before merging.

@geekdenz commented on September 12th 2021 Contributor
@geekdenz commented on September 12th 2021 Contributor

build js

@geekdenz commented on September 13th 2021 Contributor

Saw that these seem fine to download:

https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/48972/

Not sure which change made these fail, but it seems not related to this PR.

@sgiehl commented on September 15th 2021 Member

Not sure which change made these fail, but it seems not related to this PR.

@geekdenz Those look like some random failures. Might be good to merge in the latest changes from 4.x-dev into your branch as on 4.x-dev all tests are currently passing.

@tsteur commented on September 15th 2021 Member

Tested it locally and IE8 etc and seems to work 👍

@sgiehl commented on September 16th 2021 Member

@tsteur Your last commit actually would have required another build of the javascript files. Will fix that directly on 4.x-dev

This Pull Request was closed on September 15th 2021
Powered by GitHub Issue Mirror