@diosmosis opened this Pull Request on November 23rd 2020 Member

Description:

Introduces a new core metric, nb_profilable calculated with VisitsSummary metrics as the sum of visits that are profilable. If less than 1% of visits for a period are not profilable features that depend on profilable data are hidden or disabled. This includes:

  • reports that rely on detecting returning visitors (everywhere in the UI)
  • the nb_uniq_visitors metric (in the UI and API)
  • segments for dimensions that rely on detecting returning visitors (hidden in the UI only)
  • visitor log features that rely on detecting returning visitors

Other changes:

  • nb_profilable is stored even if it's value is 0, this is to be able to differentiate between archives that are old and do not have the metric vs ones that are new and have 0 profilable visits. There is an alternative to check whether ts_archived is < a certain time, but getting this information w/ numeric data is a bit complicated. It might be do-able though.

Fixes #16363

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on December 21st 2020 Member

@tsteur / @sgiehl this is ready for an initial review

@diosmosis commented on January 14th 2021 Member

@tsteur @sgiehl this review is ready for review. could use a general review of the strategy used first.

@diosmosis commented on January 17th 2021 Member

wondering generally instead of removing some reports to maybe rather show a notice like "This report won't show any data because no profilable visitors" (would need to word it better).

:+1: this makes sense to me. It would be strange to just click a report and suddenly not be able to see a report. I'll make this change?

@tsteur commented on January 17th 2021 Member

I'll make this change?
sounds great

@diosmosis commented on January 18th 2021 Member

@mattab can you provide a UI review + look at the comments on the review and provide your thoughts?

@diosmosis commented on August 10th 2021 Member

@tsteur looked over this pr again. In it I'm adding nb_profilable as a core metric, but it is saved even if the value is 0. This is so we can tell when it was never computed. An alternative is to not save if 0 like every other metric, but in that case also save has_computed_profilable=1 as another metric. Do you think this is a better approach?

@tsteur commented on August 11th 2021 Member

@diosmosis thought about it for a while and I reckon we can simply always store the column and no other column needed. Meaning we treat the values before it was not tracked as not profilable. It be worse if it was the other way around and by default it would assume a visitor was profilable.

@tsteur commented on August 25th 2021 Member

@diosmosis is this ready for a review? I know it has the label but not sure when it was added.

@diosmosis commented on August 25th 2021 Member

@tsteur it's been waiting for answers from @mattab, I'll remove the Needs Review tag.

@mattab commented on August 25th 2021 Member

I'll review this later today

@mattab commented on August 27th 2021 Member

More feedback:

  • Nice work, this will be very valuable!
  • Can we return profilable element in the response of Live.getLastVisitsDetails - it would be helpful to troubleshoot which visit is profilable or not, for example to validate value of <nb_profilable>, or to notice why the API output may not include some elements such as visitorType, visitorTypeIcon, visitCount, daysSinceFirstVisit, daysSinceLastEcommerceOrder etc.
  • Getting error The following error just broke Matomo (v4.4.1): Call to a member function isRequiresProfilableData() on null on /home/matt/dev/matomo/core/Plugin/Report.php(1013) at URL index.php?forceView=1&viewDataTable=VisitorLog&module=Live&action=getLastVisitsDetails&small=1&idSite=1&period=day&date=2021-08-27&segment=&showtitle=1&random=6365
  • i wasn't able to trigger the display of the text in plugins/CoreHome/templates/_nonProfilableDataWarning.twig -> were you able to get this to show? (it would be great to have UI test for it if not done yet)
  • Currently when there is no profilable data at all (cookie-less enforced), it doesn't hide all of the reports in "Behavior > engagement". It only hides some of them (the Returning Visits Over Time and the Frequency Overview are correctly hidden, but the Visits by Visit Number and Visits by Days Since Last Visit are still displayed (expected them not to be displayed in this case?)

Screenshot from 2021-08-27 16-12-58

  • also when these reports show in the report footer: This report requires profilable data, but the current period of 2021-08-27 does not have any, so this report will not have useful information and should not be relied upon. (see screenshot above)
    • Could we change it to This report is inaccurate should not be relied upon. This report requires profilable data (tracking cookies enabled), but none of the visits did in the current period of 2021-08-27.
    • Also, could we show the notice much more visible, using a more visible "yellow notice" /warning style? just changing the CSS of another similar notice to warning it looks like this, which would be acceptable i think (at least visible and can't really miss it)
      Screenshot from 2021-08-27 15-04-51
@mattab commented on August 27th 2021 Member

Segments are removed in the UI but nowhere else. We can also disable them and provide a tooltip saying why it doesn't make sense to use them on the current period. But when creating the segment, the period is arbitrary (eg, maybe they want it to be applied to a period w/ profilable data). Seems odd to see it greyed out and realize you have to change the period to be able to use it.

Could we leave the segments available, and not change the Segment Editor UI, but whenever they apply / use a segment that doesn't make in cookie-less (so when they are viewing the reports), then we could display a Warning notification explaining that the segment relies on data that is inaccurate because visits are not profilable?

@diosmosis commented on August 29th 2021 Member

@tsteur / @mattab this is ready for another review

@mattab commented on August 30th 2021 Member

in the original issue only unique visitors is requested. I'm not sure if there are others we'd also want to remove. Fingerprints could be useful. Can user IDs be specified without a visitor ID?

@diosmosis Here is the full scope we know so far, it is found with some more details in this FAQ: https://matomo.org/faq/general/faq_156/

Here is the full list of reports we need to disable or warn about inaccurate data:

  • Unique Visitors and New VS Returning visitors metrics will be inaccurate
  • Goals and Ecommerce conversions will be attributed to the channel used in the visit that converts
  • Multi Attribution and Cohort reports won’t show data
  • A small limited number of other reports will be inaccurate, these are:
    • Days since last visit
    • Visits by visit count
    • Visits to Conversion
    • Days to Conversion
@diosmosis commented on August 31st 2021 Member

@mattab

Unique Visitors and New VS Returning visitors metrics will be inaccurate

The whole engagement sparklines widget is disabled so no need to specify _returning specifically.

Goals and Ecommerce conversions will be attributed to the channel used in the visit that converts

Added a warning to goals & ecommerce pages.

Multi Attribution and Cohort reports won’t show data

Created PRs for this.

Other reports are marked as requiring profile data.

@tsteur commented on September 21st 2021 Member

@mattab was this one reviewed and good to merge once tests pass?

@mattab commented on September 27th 2021 Member

Not yet, I will review it as soon as possible (hopefully this week)

@tsteur commented on October 14th 2021 Member

what's the latest here @mattab ?

Powered by GitHub Issue Mirror