Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Track actual window location origin and hash #1853

Merged
merged 9 commits into from
Apr 26, 2018

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented Apr 25, 2018

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Do we actually want to be reporting origins to piwik? I thought we hid these intentionally as they could fairly identifying.

Also, is origin what you want? This will miss out the path, right? So we'll go from https://riot.im/app/ to https://riot.im/.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Apr 25, 2018
@lukebarnard1
Copy link
Contributor Author

lukebarnard1 commented Apr 25, 2018

I think the decision was indeed to report origins even though they might not be ours. It's true that we ship with piwik.riot.im being pinged by default, which is surprising.

But you're absolutely right that this neglects the path, I shall fix that.

@dbkr
Copy link
Member

dbkr commented Apr 25, 2018

Ftr, if we do report origins, we should add it to https://github.com/matrix-org/matrix-react-sdk/blob/master/src/Analytics.js#L200

@lukebarnard1
Copy link
Contributor Author

OK, having spoken with @lampholder on this, we're going to go for:

  • Opt-in analytics, i.e. not reporting to piwik.riot.im by default
  • Reporting the true origin

This way we can configure the deploys we care about (like riot.im) to send analytics whilst not upsetting deployers about leaking their domains.

If other riot deploys want to share their analytics with us, they can but we will also filter on the piwik side using a segment restricted to the domain we care about.

If a Riot is upgraded to include this commit, it will stop reporting
analytics unless specified in config.json. The sample config already
contains the configuration required to continue reporting.
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Apr 25, 2018
@dbkr
Copy link
Member

dbkr commented Apr 25, 2018

OK, but you'll need to update the list of info we send as per above.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Apr 25, 2018
@lukebarnard1
Copy link
Contributor Author

oh right, yep

Also, redesign analytics modal to be one big table, instead of table +
paragraph.
for new anayltics modal redesign
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Apr 26, 2018
@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Apr 26, 2018
@dbkr
Copy link
Member

dbkr commented Apr 26, 2018

Giving back to you in case you need to coordinate merging with piwik things

@lukebarnard1
Copy link
Contributor Author

Turns out we do care about /user/ vs. /user/<redacted> enough for us to want to change it. It would be confusing otherwise. The fix will be to special case paths that have variables in them (i.e. /user/..., /group/... and /room/...).

To preserve the analytics for these pages we did previously
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Apr 26, 2018
@dbkr
Copy link
Member

dbkr commented Apr 26, 2018

re-lgtm, although also I've just noticed: did you mean PII rather than PPI? Or is there a meaning of PPI other than Payment Protection Insurance I'm unaware of?

@lukebarnard1
Copy link
Contributor Author

sigh...

definitely meant to type PII

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants