Remove url parameter from metricsEvent
#9157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Base automatically changed from
change-background-metrics-event-category-name
to
develop
August 7, 2020 16:45
The `url` parameter was used to override the `currentPath`, but it never worked correctly. It was supposed to be used for setting the `url` query parameter that was sent to Matomo, but `currentPath` was always used even if it `url` was set and `currentPath` was empty. Instead, `currentPath` is now always used. There was never a need to provide an "override" for `currentPath` when it can be set directly. The metrics provider does set `currentPath` automatically by default, but this can be overwritten already by passing a second parameter to `metricsEvent`. There were two places this `url` parameter was being used: background events, and path changes. Background events were submitted with no `currentPath`, so because of the bug with the `url` parameter, the metrics utility would crash upon each event. So those were never actually sent. This commit will fix that crash. The `currentPath` parameter was supplied as an empty string for the path change events, so those never crashed. They just had the `url` query string parameter set incorrectly (to an empty string). It should now be correctly populated, which should mean we'll be capturing all path changes now. Previously we were only capturing path changes to pages that happened to include an event, because of this blank `url` problem.
Gudahtt
force-pushed
the
remove-metrics-url-parameter
branch
from
a81869f to
0979a30
Compare
rekmarks
reviewed
Aug 7, 2020
Gudahtt
force-pushed
the
remove-metrics-url-parameter
branch
from
864a057 to
bc8031f
Compare
The `pv_id` parameter currently isn't generated correctly on Firefox, as the generation assumes that the current URL starts with `chrome-extension://`. The `url` query parameter is still unique for each path, so it's probably good enough for generating an id for each page. This is just a temporary fix; it will be removed in a future PR, where Firefox will be properly supported.
Gudahtt
force-pushed
the
remove-metrics-url-parameter
branch
from
bc8031f to
a4a276b
Compare
rekmarks
approved these changes
Aug 7, 2020
Builds ready [a4a276b]
Page Load Metrics (571 ± 49 ms)
|
Gudahtt
added a commit
that referenced
this pull request
Aug 7, 2020
* Remove `url` parameter from `metricsEvent` The `url` parameter was used to override the `currentPath`, but it never worked correctly. It was supposed to be used for setting the `url` query parameter that was sent to Matomo, but `currentPath` was always used even if it `url` was set and `currentPath` was empty. Instead, `currentPath` is now always used. There was never a need to provide an "override" for `currentPath` when it can be set directly. The metrics provider does set `currentPath` automatically by default, but this can be overwritten already by passing a second parameter to `metricsEvent`. There were two places this `url` parameter was being used: background events, and path changes. Background events were submitted with no `currentPath`, so because of the bug with the `url` parameter, the metrics utility would crash upon each event. So those were never actually sent. This commit will fix that crash. The `currentPath` parameter was supplied as an empty string for the path change events, so those never crashed. They just had the `url` query string parameter set incorrectly (to an empty string). It should now be correctly populated, which should mean we'll be capturing all path changes now. Previously we were only capturing path changes to pages that happened to include an event, because of this blank `url` problem. * Use `url` query parameter as fallback for generating `pv_id` The `pv_id` parameter currently isn't generated correctly on Firefox, as the generation assumes that the current URL starts with `chrome-extension://`. The `url` query parameter is still unique for each path, so it's probably good enough for generating an id for each page. This is just a temporary fix; it will be removed in a future PR, where Firefox will be properly supported.
Gudahtt
added a commit
that referenced
this pull request
Aug 10, 2020
* origin/master: (44 commits) Add category in eventOpts (#9164) Update changelog for v8.0.7 (#9161) Version v8.0.7 Remove web3 e2e tests (#9159) Add web3 usage metrics, prepare for web3 removal (#9144) Use `pathname` instead of URL for `currentPath` metrics parameter (#9158) Remove `url` parameter from `metricsEvent` (#9157) Change MetaMetrics category for background events (#9155) remove .network-name height Use [email protected] (#9154) Update 'react-devtools' to ^4.8.0 (#9140) Fix connection removal bug (#9137) Add source map validator to CI (#9135) Update source map validator target files (#9133) Improve sourcemap validator console report (#9131) Add `validate-source-maps` npm script (#9134) Non-zero exit code upon failure to validate source maps (#9132) Update `brfs` from v1.6.1 to v2.0.2 (#9115) Factor out `getEnvironment` function in build script (#9114) Update `browserify` from v16.2.3 to v16.5.1 (#9113) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
urlparameter was used to override thecurrentPath, but it never worked correctly. It was supposed to be used for setting theurlquery parameter that was sent to Matomo, butcurrentPathwas always used even if iturlwas set andcurrentPathwas empty.Instead,
currentPathis now always used. There was never a need to provide an "override" forcurrentPathwhen it can be set directly. The metrics provider does setcurrentPathautomatically by default, but this can be overwritten already by passing a second parameter tometricsEvent.There were two places this
urlparameter was being used: background events, and path changes. Background events were submitted with nocurrentPath, so because of the bug with theurlparameter, the metrics utility would crash upon each event. So those were never actually sent. This commit will fix that crash.The
currentPathparameter was supplied as an empty string for the path change events, so those never crashed. They just had theurlquery string parameter set incorrectly (to an empty string). It should now be correctly populated, which should mean we'll be capturing all path changes now. Previously we were only capturing path changes to pages that happened to include an event, because of this blankurlproblem.