@AltamashShaikh opened this Pull Request on March 2nd 2022 Contributor

Fix to show multiple line graphs instead of 1 if multiple metrics present
Fixes: #DEV-2254

When multiple metrics are selected the line graph plots only for the first metric while sending a custom report over an Email.

Example: A custom report with evolution graph containing 3 metrics nb_visits,avg_sum_actions_per_visits and avg_sum_events_totalevents_per_visits we receive below output in Email report
Screenshot from 2022-03-02 13-01-22

Adding the fix changes to
Screenshot from 2022-03-02 12-56-08

Need to make changes in plugin too
Example: https://github.com/innocraft/plugin-CustomReports/pull/124

Description:

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

@sgiehl commented on March 8th 2022 Member

@AltamashShaikh your change actually still changes the default behavior. So by default those charts will now include all metrics instead of only the main metric. For any reports including a lot metrics this might actually be problematic, or did you test that with a report that has 10 metrics or more? Maybe we should limit that to the first X metrics.

@tsteur what do you think regarding changing the default behavior here?

@tsteur commented on March 8th 2022 Member

@AltamashShaikh @sgiehl by default be good to keep existing behaviour indeed as many reports have heaps of metrics and this could cause issues. Or if there are metrics of different units. Then things could get messy and it be better if the report itself specified which metrics.

@AltamashShaikh commented on March 9th 2022 Contributor

@sgiehl Yes allowing all the metrics breaks the image
Also throws an error Matomo encountered an error: Argument 2 passed to CpChart\Data::setPalette() must be of the type array, null given, called in /var/www/html/work/matomo/plugins/ImageGraph/StaticGraph/GridGraph.php on line 75 (which lead to: Zend_Session is currently marked as read-only.)
After fixing the error the graph looks like broken
Screenshot from 2022-03-09 13-54-40
May be allowing a max of 3-4 metrics should be done

@sgiehl commented on March 9th 2022 Member

@AltamashShaikh as @tsteur mentioned: Also using a couple of metrics might cause issue if they are using different unit. So we need to find another way to do that. For example each report could hold an additional array of metrics that would be displayed in the evolution charts. Or it could also be part of the scheduled reports configuration to choose which metrics to display.
But for the specific issue you are trying to solve I would only apply the needed changes in CustomReports plugin and don't change anything in core. Maybe this would be possible using some events like ScheduledReports.getRendererInstance.

@tsteur commented on March 15th 2022 Member

@sgiehl in above PR would this change anything in core? If not, it might be a better/easier solution than using an event? Or how would it look like using the event?

My understanding is with this PR each plugin / each report would define which columns to add? I haven't looked too much though.

@sgiehl commented on March 15th 2022 Member

@tsteur currently the PR would change the core behavior. It would try to display all available columns if the report has columns defined and no columns where set in the request.

I already mentioned my suggested solutions above. The solution using an event would only make sense if we e.g. need a speical handling for custom reports. If we want to define the metrics that should be displayed in evolution by default for all reports we could e.g. introduce a new property in Report class, which holds the metrcis for evolution charts.

@tsteur commented on March 15th 2022 Member

@sgiehl I might be looking wrong. I haven't seen any core report that has columns configured in the report metadata aka it might not change anything unless defined by a report?

I don't think a report property be needed for now and could go with a more pragmatic solution. This could be always done later I suppose if needed.

I had a very quick look for ScheduledReports.getRendererInstance but couldn't directly figure out how to make it work just for custom reports there.

@AltamashShaikh otherwise another idea would be what we talked about to change the reportId generation. So we allow a report to define it's own reportId, and then we could set the columns in the parameters after all.

@AltamashShaikh commented on March 21st 2022 Contributor

@sgiehl I might be looking wrong. I haven't seen any core report that has columns configured in the report metadata aka it might not change anything unless defined by a report?

I don't think a report property be needed for now and could go with a more pragmatic solution. This could be always done later I suppose if needed.

I had a very quick look for ScheduledReports.getRendererInstance but couldn't directly figure out how to make it work just for custom reports there.

@AltamashShaikh otherwise another idea would be what we talked about to change the reportId generation. So we allow a report to define it's own reportId, and then we could set the columns in the parameters after all.

@tsteur The uniqueID solution doesn't work, @sgiehl we are planning to limit the max no of allowed metrics to 3 which will always make sure the graphs looks more visible.

@sgiehl commented on March 24th 2022 Member

@AltamashShaikh whatever solution you may plan: There shouldn't be any changes needed in ImageGraph plugin. The method already supports providing the columns you want to display. If you want to show additional columns in scheduled reports you may need to change it there. But please keep in mind, simply using the first three columns might cause other unwanted issues.
If you eg show visits and actions in the same graph and the actions are a lot higher, the line for visits might always be near to 0, which might make it impossible to see the values at all. Also if those columns have different metric types - like one might holds a normal number, another a time or a currency. I'm not sure if that might cause problems in ImageGprah and if those columns would then be displayed correctly.

@github-actions[bot] commented on April 16th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@sgiehl commented on April 26th 2022 Member

@AltamashShaikh What's the current state of this PR?

@AltamashShaikh commented on April 27th 2022 Contributor

@AltamashShaikh What's the current state of this PR?

@sgiehl I tried setting columns using $this->parameters['columns'] here similar to idCustomReport

As this is the ideal way to pass columns to ImageGraph Plugin.
However this breaks due to uniqueId logic generation considers everything inside parameters
https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/ScheduledReports/API.php#L415

Adding the changes suggested in PR, we can then set columns in $reportMetaData and we can show multiple line graphs.
We will limit it to 3 metrics max.
There will be variation in 'y-axis' due to multiple metrics but that's how line graphs are when we try to plot multiple metrics with 1 side axis and also this will be set by user, how would generally be aware of this variance or can change later.

@sgiehl commented on April 27th 2022 Member

@AltamashShaikh It should be possible to manipulate the evolution charts urls in the processed report parameters. Like it's done here: https://github.com/matomo-org/matomo/blob/be724571d2f477b4f553411d41df3fb22408c0a6/plugins/PagePerformance/PagePerformance.php#L147-L158

Maybe that solution works.

@AltamashShaikh commented on April 28th 2022 Contributor

@sgiehl The solution suggested works no need to make any changes in core :+1:

This Pull Request was closed on April 28th 2022
Powered by GitHub Issue Mirror