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

Jump to bottom

add setting to disable dimensions server side, skip those dimensions … #16609

Closed
wants to merge 23 commits into from
Closed

add setting to disable dimensions server side, skip those dimensions … #16609

wants to merge 23 commits into from

Conversation

diosmosis
Copy link
Member

Fixes #15435

@diosmosis diosmosis added 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. labels Oct 26, 2020
@diosmosis diosmosis added this to the 4.0.0-RC milestone Oct 26, 2020
@tsteur
Copy link
Member

tsteur commented Oct 28, 2020

@diosmosis haven't had a detailed look yet or tried it yet but do you think it is possible to remove a report when a dimension is disabled? Say there is a report "City" with a "City" dimension but this dimension is disabled. I reckon this might be possible to do in Piwik\Plugins\Report::isEnabled which would then check if a dimension is set and if so check if that dimension was maybe disabled.

Then ideally the widget and report would disappear from the UI. Or maybe better we would render a simple standard view saying "This report was disabled by a super user. To enable it and start tracking this data, please ask a super user to enable this report in ..."

@diosmosis
Copy link
Member Author

@tsteur added report disabling + some dimension names + only showing tracked dimensions

tsteur
tsteur requested changes Oct 29, 2020
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment •
edited

Choose a reason for hiding this comment

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

Generally I reckon this feature will be quite useful 👍 It's nice to be able to disable things like longitude, latitude etc. I'm expecting though there will be eventually side effects and some hard coding needed later as when any of these 2 dimensions are disabled, then the real time map report won't work but it's likely still shown because we maybe cannot disable it automatically (haven't tested it though, maybe it does work).

  • There are some dimensions that should not be possible to be disabled like measurable, visitId, last action time, profilable, visitorId, channel type, visit type, fingerprint. Might also need to check if there are maybe some dimensions that could cause issues or side effects in unexpected reports when they are not tracked.

  • userId for example appears twice in the list. Probably Piwik\Plugins\UserId\Columns\UserId should not be used as it's not tracking any data?

  • To disable the related userId report we may need to change the dimension in the GetUsers report to Piwik\Plugins\CoreHome\Columns\UserId

Didn't check if any other dimensions appear multiple times.

  • Could we add a list of disabled dimensions (if any) to the system check as Informational output? This will help when troubleshooting issues in the future so we can see if some dimensions are disabled when we ask for the system report.

Btw... when selecting a dimension, it always closes directly after just selecting one dimension. Not sure if it can stay open? Not too important for now if it's not easy to do. Also when there are too many selected they aren't all visible but that's fine too for now as it's a general problem of the multi select. Do you think it would be easy to use the single expandable select ui control similar to in custom reports instead? It would be still a problem that after selecting one metric that it closes but at least it would show all selected dimensions plus supports search etc. see screen
image

Wonder if we should move this setting to Privacy -> Anonymise data page or alternatively at least under the Privacy Manager system settings maybe? It took me actually quite a while to find the setting and only after looking in the code found the setting.

Was thinking ideally it would also remove the dimension from the list of segments but I reckon for now we maybe don't want to do this as it could cause issues with existing segments plus maybe some other issues. I suppose we could always do this later.

In custom reports we could maybe remove any disabled dimension from the list of available dimensions? Would need a PR in the custom report and then it would probably also affect the list of selectable metrics. It can quickly get maybe complicated so we maybe wouldn't need to have this as part of the MVP.

Regarding the UI:
Been thinking about the reports that have disabled dimensions for quite a while. I'm still not 100% sure what to do there but thinking maybe we should show the report after all and instead only add a "footer or no data message" when no data exists something like : The dimension for this report is disabled. If you are wanting to track this data please ask a user with super user access to enable this dimension in the xxx settings.

This way users can maybe still view already tracked data and users won't be as confused when a certain report is not shown. On the other side it would have been nice to be able to disable reports this way but for now not needed as part of MVP I would say.

The only problem is that API consumers like the Matomo Mobile app wouldn't know that such a footer message needs to be shown but this is maybe also not needed as part of MVP. I wonder if eventually we maybe simply add another checkbox to decide whether to show related reports or not.

@mattab if you have any thoughts on this let us know ASAP

plugins/CoreAdminHome/SystemSettings.php Outdated Show resolved Hide resolved
plugins/CoreAdminHome/SystemSettings.php Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

Btw... when selecting a dimension, it always closes directly after just selecting one dimension. Not sure if it can stay open?

This is caused by the following code: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/CorePluginsAdmin/angularjs/form-field/form-field.directive.js#L102-L108

I'm not sure if it's really needed or even what it's intended purpose was.

@diosmosis
Copy link
Member Author

Wonder if we should move this setting to Privacy -> Anonymise data page or alternatively at least under the Privacy Manager system settings maybe?

I thought privacy might be a good place as well.

Don't really have an opinion on the other UX items, mostly those seem to be about the scope of the issue.

@mattab
Copy link
Member

mattab commented Nov 1, 2020

@mattab if you have any thoughts on this let us know ASAP

My main thought is about find any possible issue before people find it themselves (it would save us lots of time if we found any bug proactively). for example: could we create some test to disable all the dimensions using this feature, and track some various data (pageview, event, download, outlinks, with most other dimensions set) and test if tracking+archiving+some APIs still work as expected? (at least one manual test and also some automated tests).

@diosmosis
Copy link
Member Author

for example: could we create some test to disable all the dimensions using this feature, and track some various data (pageview, event, download, outlinks, with most other dimensions set) and test if tracking+archiving+some APIs still work as expected? (at least one manual test and also some automated tests).

I can add a system test like this.

@tsteur
Copy link
Member

tsteur commented Nov 2, 2020

Be good to move it into the privacy manager since it is mostly interesting for privacy reasons to track less data.

@diosmosis let me know when all the changes are done and I'll review again

@diosmosis
Copy link
Member Author

@tsteur still fixing the tests, but this should be ready for another review

@tsteur
Copy link
Member

tsteur commented Nov 8, 2020

@diosmosis is this ready for another review? or any questions open?

@diosmosis
Copy link
Member Author

@tsteur yes it's ready for a review

@diosmosis
Copy link
Member Author

@tsteur yes ready for another review

@tsteur
Copy link
Member

tsteur commented Nov 9, 2020

Thanks @diosmosis.

I started reviewing again and started having these points

  • Can we move this to the privacy manager -> anonymise data page?
  • When there is no data for a report, and it has a dimension that is disabled, can we show a message that it's showing maybe no data because the dimension is disabled?
  • Can we hide dimensions that store a numeric or enum value since they likely won't store any personal data from a privacy perspective? I reckon eg "number of interactions" etc would then remove from the list. This would I think solve two problems: Too many dimensions in the list make it hard to use and some dimensions could break something maybe like "Visit ecommerce status at the end of the visit".

But then noticed the feature is not all that useful as I was hoping if we don't also remove the reports from the UI that are disabled. However, removing these reports might cause issues that people don't understand why certain reports are not visible etc. And then these dimensions would still appear in segments etc and for some dimensions we would need to do more hard coding to disable the related reports (like removing real time map if longitude or latitude dimension is disabled). Disabling some dimensions could cause problems eg with fingerprint (when disabling plugins). Disabling referrer url or referrer type could break the correctness of quite a few reports as well and everything is overall not that predictable what would happen when disabling certain dimensions. I should have thought it more through from the beginning as it turns out quite complicated. I reckon overall the feature might not be used that much but could cause more issues (especially in support and troubleshooting bugs etc) that it is not worth proceeding with this for now. As a workaround on On-Premise people still have the possibility to disable certain dimensions by disabling plugins. We might need to roll this feature out for Cloud and On-Premise as well although Matomo isn't really tested much when certain plugins are disabled and it doesn't always work. I'm thinking though overall disabling entire plugins might be more of a solution then this maybe.

Thanks for your work on this @diosmosis. For now we will close this PR and also close the related issue as a wontfix.

@diosmosis in a new PR can we still keep this change to add a default value to visits count? https://github.com/matomo-org/matomo/pull/16609/files#diff-4de94fc1a73792e176363f13ba9a47f47f0d69bef9e180f6b11e11b828a14c2cR21

And maybe we could also keep the changes for adding the dimension names like for the plugin dimensions as eg in https://github.com/matomo-org/matomo/pull/16609/files#diff-c025d6a1afc9d0b6ed34a29a2fe6f050da06b16821d05cbb8445781a684ee8f5R23 ?
This would make them actually available in custom reports

@tsteur tsteur mentioned this pull request Nov 9, 2020
Closed
@diosmosis
Copy link
Member Author

@tsteur I guess we'd want to go through and list every dimension that someone might want to disable and see if it is already in its own plugin.

I can move the other changes into a new PR.

Also I guess I misread the privacymanager change, I moved it to the plugin's settings, not the anonymise data page.

@diosmosis diosmosis mentioned this pull request Nov 9, 2020
Merged
9 tasks
@tsteur
Copy link
Member

tsteur commented Nov 9, 2020

Thanks for creating the other PR @diosmosis 👍

@tsteur tsteur closed this Nov 9, 2020
@tsteur tsteur deleted the 15435-disabled-dimensions branch November 9, 2020 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur requested changes

Assignees
No one assigned
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
Milestone
4.0.0-RC
Development

Successfully merging this pull request may close these issues.

Have an option to track only needed data for better privacy
3 participants
@diosmosis @tsteur @mattab