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
Fix to show multiple line graphs instead of 1 if multiple metrics present #18870
Conversation
@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 |
@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. |
@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 @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 |
@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 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? |
@sgiehl I tried setting columns using As this is the ideal way to pass columns to ImageGraph Plugin. Adding the changes suggested in PR, we can then set columns in |
@AltamashShaikh It should be possible to manipulate the evolution charts urls in the processed report parameters. Like it's done here: matomo/plugins/PagePerformance/PagePerformance.php Lines 147 to 158 in be72457
Maybe that solution works. |
@sgiehl The solution suggested works no need to make any changes in core 👍 |
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
andavg_sum_events_totalevents_per_visits
we receive below output in Email reportAdding 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