Skip to content

Commit

Permalink
Remove url parameter from metricsEvent (#9157)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
Gudahtt authored Aug 7, 2020
1 parent 1d84f33 commit 8713927
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 9 deletions.
2 changes: 1 addition & 1 deletion app/scripts/lib/backend-metametrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export default function backEndMetaMetricsEvent (metaMaskState, eventData) {
sendMetaMetricsEvent({
...stateEventData,
...eventData,
url: METAMETRICS_TRACKING_URL + '/background',
currentPath: METAMETRICS_TRACKING_URL + '/background',
})
}
}
8 changes: 3 additions & 5 deletions ui/app/helpers/utils/metametrics.util.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const METAMETRICS_BASE_URL = 'https://chromeextensionmm.innocraft.cloud/piwik.ph
const METAMETRICS_REQUIRED_PARAMS = `?idsite=${projectId}&rec=1&apiv=1`
const METAMETRICS_BASE_FULL = METAMETRICS_BASE_URL + METAMETRICS_REQUIRED_PARAMS

const METAMETRICS_TRACKING_URL = inDevelopment
export const METAMETRICS_TRACKING_URL = inDevelopment
? 'http://www.metamask.io/metametrics'
: 'http://www.metamask.io/metametrics-prod'

Expand Down Expand Up @@ -119,7 +119,6 @@ function composeParamAddition (paramValue, paramName) {
* @property {string} config.currentPath The location path the user is on at the time of the event
* @property {string} config.metaMetricsId A random id assigned to a user at the time of opting in to metametrics. A hexadecimal number
* @property {string} config.confirmTransactionOrigin The origin on a transaction
* @property {string} config.url The url to track an event at. Overrides `currentPath`
* @property {boolean} config.excludeMetaMetricsId Whether or not the tracked event data should be associated with a metametrics id
* @property {boolean} config.isNewVisit Whether or not the event should be tracked as a new visit/user sessions
* @returns {string} - Returns a url to be passed to fetch to make the appropriate request to matomo.
Expand All @@ -141,7 +140,6 @@ function composeUrl (config) {
currentPath,
metaMetricsId,
confirmTransactionOrigin,
url: configUrl,
excludeMetaMetricsId,
isNewVisit,
} = config
Expand All @@ -167,10 +165,10 @@ function composeUrl (config) {
numberOfTokens: (customVariables && customVariables.numberOfTokens) || numberOfTokens,
numberOfAccounts: (customVariables && customVariables.numberOfAccounts) || numberOfAccounts,
}) : ''
const url = configUrl || currentPath ? `&url=${encodeURIComponent(currentPath.replace(/chrome-extension:\/\/\w+/, METAMETRICS_TRACKING_URL))}` : ''
const url = currentPath ? `&url=${encodeURIComponent(currentPath.replace(/chrome-extension:\/\/\w+/, METAMETRICS_TRACKING_URL))}` : ''
const _id = metaMetricsId && !excludeMetaMetricsId ? `&_id=${metaMetricsId.slice(2, 18)}` : ''
const rand = `&rand=${String(Math.random()).slice(2)}`
const pv_id = ((url || currentPath) && `&pv_id=${ethUtil.bufferToHex(ethUtil.sha3(url || currentPath.match(/chrome-extension:\/\/\w+\/(.+)/)[0])).slice(2, 8)}`) || ''
const pv_id = currentPath ? `&pv_id=${ethUtil.bufferToHex(ethUtil.sha3(currentPath.match(/chrome-extension:\/\/\w+\/(.+)/)?.[0] || url)).slice(2, 8)}` : ''
const uid = metaMetricsId && !excludeMetaMetricsId
? `&uid=${metaMetricsId.slice(2, 18)}`
: excludeMetaMetricsId
Expand Down
6 changes: 3 additions & 3 deletions ui/app/pages/routes/routes.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ import {
UNLOCK_ROUTE,
} from '../../helpers/constants/routes'

import { METAMETRICS_TRACKING_URL } from '../../helpers/utils/metametrics.util'

import { ENVIRONMENT_TYPE_NOTIFICATION, ENVIRONMENT_TYPE_POPUP } from '../../../../app/scripts/lib/enums'
import { getEnvironmentType } from '../../../../app/scripts/lib/util'

Expand Down Expand Up @@ -98,11 +100,9 @@ export default class Routes extends Component {
this.props.history.listen((locationObj, action) => {
if (action === 'PUSH') {
pageChanged(locationObj.pathname)
const url = `&url=${encodeURIComponent('http://www.metamask.io/metametrics' + locationObj.pathname)}`
this.context.metricsEvent({}, {
currentPath: '',
currentPath: `${METAMETRICS_TRACKING_URL}${locationObj.pathname}`,
pathname: locationObj.pathname,
url,
pageOpts: {
hideDimensions: true,
},
Expand Down

0 comments on commit 8713927

Please sign in to comment.