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

Add piwik.js public api method setPagePerformanceTiming. #17901

Merged
merged 48 commits into from Sep 15, 2021
Merged

Conversation

geekdenz
Copy link
Contributor

@geekdenz geekdenz commented Aug 18, 2021

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
Copy link
Member

tsteur commented Aug 18, 2021

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.

@geekdenz geekdenz marked this pull request as ready for review August 18, 2021 05:58
@geekdenz geekdenz added the Needs Review PRs that need a code review label Aug 18, 2021
@geekdenz geekdenz added this to the 4.5.0 milestone Aug 18, 2021
@sgiehl
Copy link
Member

sgiehl commented Aug 18, 2021

@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

matomo/js/piwik.js

Lines 3773 to 3777 in da4df21

// performance tracking
if (configPerformanceTrackingEnabled && performanceAvailable && !performanceTracked) {
request = appendAvailablePerformanceMetrics(request);
performanceTracked = true;
}

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 geekdenz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 20, 2021
@geekdenz geekdenz changed the title add piwik.js public api method setPagePerformanceTiming Add piwik.js public api method setPagePerformanceTiming. Aug 23, 2021
@geekdenz
Copy link
Contributor Author

build js

@geekdenz
Copy link
Contributor Author

build js

@geekdenz
Copy link
Contributor Author

build js

@geekdenz
Copy link
Contributor Author

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?

js/piwik.js Outdated Show resolved Hide resolved
to work as expected added comments from @sgiehl
because this is not obvious
@geekdenz
Copy link
Contributor Author

build js

@geekdenz geekdenz marked this pull request as draft August 25, 2021 22:41
@geekdenz geekdenz removed the Needs Review PRs that need a code review label Aug 26, 2021
@geekdenz
Copy link
Contributor Author

Added some more tests here:

matomo-org/test-examples@33c6d3c

@geekdenz geekdenz added the Needs Review PRs that need a code review label Sep 8, 2021
@geekdenz geekdenz marked this pull request as ready for review September 8, 2021 01:09
@geekdenz
Copy link
Contributor Author

geekdenz commented Sep 9, 2021

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.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Getting there 🎉 Left a comment re different behaviour of values.

js/piwik.js Outdated Show resolved Hide resolved
js/piwik.js Outdated Show resolved Hide resolved
js/piwik.js Outdated Show resolved Hide resolved
@geekdenz
Copy link
Contributor Author

geekdenz commented Sep 9, 2021

build js

@geekdenz
Copy link
Contributor Author

build js

@geekdenz
Copy link
Contributor Author

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 geekdenz marked this pull request as draft September 11, 2021 08:25
@geekdenz
Copy link
Contributor Author

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

we could write more functions like this in
the future, e.g. mapIn, filter or map, but
currently this is not needed because there
is just this one use-case
@geekdenz
Copy link
Contributor Author

Documented here:
matomo-org/developer-documentation#549

@geekdenz
Copy link
Contributor Author

build js

@geekdenz geekdenz marked this pull request as ready for review September 13, 2021 01:14
@geekdenz
Copy link
Contributor Author

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
Copy link
Member

sgiehl commented Sep 15, 2021

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
Copy link
Member

tsteur commented Sep 15, 2021

Tested it locally and IE8 etc and seems to work 👍

@tsteur tsteur merged commit 25c9512 into 4.x-dev Sep 15, 2021
@tsteur tsteur deleted the m-16859 branch September 15, 2021 23:17
@sgiehl
Copy link
Member

sgiehl commented Sep 16, 2021

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

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
3 participants