Fix to show multiple line graphs instead of 1 if multiple metrics present #18870

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

Adding the fix changes to

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

• [ ] Functional review done
• [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• [ ] Security review done
• [ ] Wording review done
• [ ] 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

@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?

@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.

@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

May be allowing a max of 3-4 metrics should be done

@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.

@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.

@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.

@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.

@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.

@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.

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

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

@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.

@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.

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