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

Add all goal specific metrics to goal overview #13906

Merged
merged 9 commits into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@tsteur
Copy link
Member

tsteur commented Dec 23, 2018

fix #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 ca0f70c#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.

tsteur added some commits Nov 8, 2018

@tsteur tsteur added this to the 3.8.0 milestone Dec 23, 2018

@mattab mattab added the Needs Review label Dec 24, 2018

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Dec 25, 2018

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

This comment has been minimized.

Copy link
Member

diosmosis commented Dec 25, 2018

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

This comment has been minimized.

Copy link
Member

mattab commented Dec 31, 2018

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

This comment has been minimized.

Copy link
Member

tsteur commented Jan 2, 2019

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 tsteur modified the milestones: 3.8.0, 3.9.0 Jan 2, 2019

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Jan 2, 2019

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

This comment has been minimized.

Copy link
Member

mattab commented Jan 2, 2019

should be good enough to test with 100 goals :)

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Jan 3, 2019

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

@diosmosis diosmosis merged commit 224cacc into 3.x-dev Jan 22, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@diosmosis diosmosis deleted the 12770 branch Jan 22, 2019

@mattab mattab modified the milestones: 3.9.0, 3.8.1 Jan 22, 2019

flyapen added a commit to ICTO/matomo that referenced this pull request Jan 23, 2019

Add all goal specific metrics to goal overview (matomo-org#13906)
* add all goal  metrics to goals overview graph

* add conversion rates

* correctly calculate conversion rate

* more fixes

* performance tweak

* improve performance of getMetric when having heaps of goals

* fix conversion rate might not be shown properly

* add parameter to fetch all goal metrics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment