@tsteur opened this Pull Request on December 23rd 2018 Member

fix https://github.com/matomo-org/matomo/issues/3528

@diosmosis made it work to conversion rate and made it less fragile by always adding all goal specific metrics when Goals.get is called without any idGoal. This can slow down the call to Goals.get quite a bit though I suppose maybe? Especially when the site has hundreds or thousands of goals. FYI @mattab @diosmosis .

If needed I suppose I could potentially add checks like https://github.com/matomo-org/matomo/commit/ca0f70ce0f20f1714f0a1bf793db214e13ab3ae5#diff-7da8217e8976f8d4fd07381ebc996c1dR485 (!Request::isRootRequestApiRequest() && Piwik::getAction() === 'getEvolutionGraph') but it makes things more likely to break.

There is one remaining issue I couldn't fix. The goal specific conversion rate is correctly shown when I include the general metric "Conversion rate"
image

but it is zero when I don't include the overview/general "Conversion rate" metric.

image

I've added some code for dependent metrics in the Goals.get API method but this doesn't seem to be enough. I believe the problem is that nb_visits in ColumnDelete filter might be deleted too early? I'm guessing it works when conversion_rate is added because it is defined in the report class for Goals.get here: https://github.com/matomo-org/matomo/blob/12770/plugins/Goals/Reports/Get.php#L37 and somewhere it figures out to keep nb_visits where it is otherwise deleted. Just haven't found yet where.

I might be able to continue on this end of the week or next week but otherwise feel free to have a look @diosmosis if you remember where the metric might get lost.

@diosmosis commented on December 25th 2018 Member

This can slow down the call to Goals.get quite a bit though I suppose maybe? Especially when the site has hundreds or thousands of goals.

Is it possible for a site to have hundreds/thousands of goals in practice? If so, this would effectively break the UX for the metric picker, there would be far too many metrics. CC @mattab

@diosmosis commented on December 25th 2018 Member

If needed I suppose I could potentially add checks like ca0f70c#diff-7da8217e8976f8d4fd07381ebc996c1dR485 (!Request::isRootRequestApiRequest() && Piwik::getAction() === 'getEvolutionGraph') but it makes things more likely to break.

Could also add a request parameter to the ViewDataTable config like 'showAllGoalSpecificMetrics' = 1 and handling this case w/ that parameter. Since Goals.get is used in API.get which is used in archiving, seems like it would be a good idea? (assuming of course the possible broken UX isn't an issue)

@mattab commented on December 31st 2018 Member

Is it possible for a site to have hundreds/thousands of goals in practice? If so, this would effectively break the UX for the metric picker, there would be far too many metrics. CC @mattab

thousands would happen very rarely (edge case), however 30 or 50 goals would occur more frequently. Is the UI slow or unresponsive when 100 goals or is it still fast? it's OK if the feature is not usable with thousands of goals, as long as it doesn't break the UI or make things worse. please confirm and feel free to merge then @diosmosis

@tsteur commented on January 2nd 2019 Member

I'll move this into 3.9.0 as the feature got even more complicated. I'll need to generate heaps of goals and do couple tests etc.

@tsteur commented on January 2nd 2019 Member

fyi: I will test probably with 1000 goals max... tested 50.000 goals and UI is not usable... it would never load not even after 10 minutes... did a test with a smaller max input time
image

even when emptying the getMetric method it would be still crazy slow and not load ... anyway. I don't think anyone who is taking goals seriously can manage to have an eye on more than 1000 goals :)

@mattab commented on January 2nd 2019 Member

should be good enough to test with 100 goals :)

@tsteur commented on January 3rd 2019 Member

I actually did test it only with 100 in the end as otherwise all the sparklines on that page etc take forever to load. I reckon 1000 goals in the picker would work fine and more likely other parts would be a problem (it starts with eg archiving 1000 goals)... I think I've fixed the problem I mentioned in the description re conversion rate for a goal and next tests would need to be adjusted if they work.

also added the flag you suggested @diosmosis

This Pull Request was closed on January 22nd 2019
Powered by GitHub Issue Mirror