Adds new event to define units for metrics #13640
Conversation
sgiehl
added
c: Platform
core/Metrics.php
Outdated
| * @param string $idSite id of the current site | ||
| * @param string $unit should hold the unit (e.g. %, €, s or empty string) | ||
| */ | ||
| Piwik::postEvent('Metrics.getUnit', [$column, $idSite, &$unit]); |
There was a problem hiding this comment.
I think usually we post the metric that can be changed and is passed by reference first. Then all following parameters that may change the metric which makes it also easier to add more params later if needed.
The method seems to be only used by evolution yet seeing the event name you could think it would be applied to all reports etc. Maybe the event name could be renamed?
There was a problem hiding this comment.
I've adjusted the code. Wondering if we really should mention it in the changelog. Imho it would be nice if wouldn't need it at all. But guess that would require some more refactoring, so we are able to determine the unit based on the column name. Maybe we even could get a "Metric" instance based on the column name and have a method getUnit in such metric classes or similar. Or we have a metric type and do the formatting based on the type (using the existing formatters)
There was a problem hiding this comment.
I don't know how much work it is, and whether it cause any trouble... but may be worth a try to check if the metric ID can be found by https://github.com/matomo-org/matomo/blob/3.x-dev/core/Columns/MetricsList.php and the formatting could be done eg through https://github.com/matomo-org/matomo/blob/3.x-dev/core/Plugin/ArchivedMetric.php#L130 but as you say this would require potentially some more work and not sure how much it is doable/not doable.
There was a problem hiding this comment.
We can't simply do the formatting in PHP as the yaxsis are grouped based on the defined units. Also the JS imho needs the unformatted numbers to work. So we need some kind of definition which column has which metric.
| if (!empty($unit)) { | ||
| return $unit; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could this method get called a lot? If so might make sense to cache the value in the transient cache to avoid posting the event too many times.
There was a problem hiding this comment.
It's called in JqplotDataGenerator. Not sure if it's worth to cache that, as cache would need to be based on columnname and idsite. So likely a lot entries would be created
|
LGTM, not sure if there's more to the |
Evolution charts and row evolution uses the method
Metrics::getUnitto determine the unit that should be shown with any metric.For new plugins having custom metrics there is currently no possibility to manipulate that behavior. This causes all metrics, that don't match the format handled by the core method, to be shown as numbers only, even if they should be percent or time values.
The new event will make that possible.