Ensures that metrics in scheduled reports are fully formatted #23976
Conversation
| 'language' => $language, | ||
| 'serialize' => 0, | ||
| 'format' => 'original', | ||
| 'format_metrics' => 'all', |
There was a problem hiding this comment.
@sgiehl
This will format all the metrics that are in micro seconds to be formatted in its default value.
Unfortunately, the scope of the issue is just for the performance based metrics to be formatted in sentence mode.
It should only affect these metrics:
- avg_time_network
- avg_time_server
- avg_time_transfer
- avg_time_dom_processing
- avg_time_dom_completion
- avg_time_on_load
- avg_page_load_time
Which are only shown in Performance Overview and Custom Reports where these metrics are added.
There was a problem hiding this comment.
That makes things just fully inconsistent. We really shouldn't start overwriting the formatting just for specific metrics. Maybe the issue just missed those metrics?
There was a problem hiding this comment.
I understand that it would be inconsistent if the metrics are formatted differently for just the metrics stated above, but that was what was asked for when 'demo-ing' the feature before
I think the reason to just format the metrics above was because they are usually in actual micro seconds and would be easier to read in 'sentence mode' as compared to 'HH:mm:ss' mode.
It also makes the other duration metrics (not shown above) become 'wider' since most of them would show 'x mins y s' and the formatting does not really make sense.
There was a problem hiding this comment.
Are you making assumptions or does the issue clearly mention that the other duration metrics should not be touched?
There was a problem hiding this comment.
It was clearly mentioned. I was told that ALL other duration metrics should remain the same, and only those 7 mentioned above should be formatted.
@chip Pison
Only show the NEW format 1.45s for these metrics:
Page Performance
- avg_time_network
- avg_time_server
- avg_time_transfer
- avg_time_dom_processing
- avg_time_dom_completion
- avg_time_on_load
- avg_page_load_time
There was a problem hiding this comment.
I can't see any reason why they should have different formats. Either we want all duration based metrics to be more human readable or not. The pretty format for those metrics used in the UI already is that "new" format.
It is more like a bug, that the formatting isn't applied correctly for scheduled reports.
I'd suggest to iterate back to product team to discuss that.
There was a problem hiding this comment.
@sgiehl
Hi Steffan,
I discussed this with PO yesterday. I created two reports to give him, one was with all duration metrics formatted to be human readable and one report that only had performance based metrics formatted to be human readable and the rest was just as before. I also put in there that your solution was more simple and is more consistent.
The PO decided to stick with the solution where its only the performance based metrics are reformatted to be human readable.
I can arrange a small catchup between the 3 of us at your time if you want to discuss this further
Cheers
There was a problem hiding this comment.
Yes please. I'd like to avoid introducing more inconsistencies into our already inconsistent formatting behavior.
|
Based on our discussion yesterday, I will close this off now and will be replaced by this PR: #24050 |
2 participants
Description
Sending
format_metrics=allshould ensure that the metrics are formatted as expected.There is some strange behavior that a
0for revenue is sometimes not formatted. The reason for that is, that the original reports misses the revenue column when there were no goal conversions at all for a record.This is caused by the record builders using https://github.com/matomo-org/matomo/blob/9a3ef94df61682e7d543f3f8b438c368f6eb71a6/core/DataTable/Filter/EnrichRecordWithGoalMetricSums.php, which skips adding revenue if there are no goals converted. This imho doesn't make much sense, but isn't anything we should change here.
replaces #23931
Checklist
Review