[LookerStudio] add new metricTypes report metadata (4.x) #20425
Conversation
core/Plugin/Report.php
Outdated
| * @return string[] maps metric name => semantic type | ||
| * @api | ||
| */ | ||
| public function getMetricSemanticTypes() |
There was a problem hiding this comment.
Would be awesome to add (return) type hints to this and all other new methods were possible.
| public function getMetricSemanticTypes() | |
| public function getMetricSemanticTypes(): array |
There was a problem hiding this comment.
@sgiehl parameter type hints as well or just return type hints?
There was a problem hiding this comment.
parameter type hints as well, as long as they are supported by PHP 7.2 🙂
plugins/Actions/Actions.php
Outdated
| 'nb_uniq_outlinks' => Metric::SEMANTIC_TYPE_NUMBER, | ||
| 'nb_searches' => Metric::SEMANTIC_TYPE_NUMBER, | ||
| 'nb_keywords' => Metric::SEMANTIC_TYPE_NUMBER, | ||
| 'avg_time_generation' => Metric::SEMANTIC_TYPE_DURATION, // TODO: should all have the same unit |
There was a problem hiding this comment.
Any reason for adding this here and not directly in the metric class? Wouldn't it be useful to use the metric classes to define the semantic type where possible?
At some point in the future I would actually love to have metric classes for all available metrics and that reports need to use a metric class for each metric they contain. But that might be a bit bigger refactoring, so not sure when we will get there. But already moving everything to the Metric class that can be defined there might already be helpful....
There was a problem hiding this comment.
Most reports reference metrics by their string names, not by a Metric instance, so it would be difficult in Report.php to get that information. There is a getSemanticType() method in Metric though, but right now it's primarily used for ProcessedMetrics, which are currently used in Report::$processedMetrics.
There was a problem hiding this comment.
Sure. But isn't avg_time_generation a processed metric as well? But might be not that import for now. Just wanted to mention that we should try to move as much as possible to the metrics, so we have less work when we start refactoring that at some point...
There was a problem hiding this comment.
I just put the same metrics from the 'Metrics.getDefaultMetricTranslations' hook into the new semantic types hook. I don't expect that would hinder the move from string metric names to metric classes in Report classes in any way. The work should take hours at most regardless.
…Metric classes instead of addMetricSemanticTypes() where possible
|
@sgiehl updated |
99e1e88 to
ac9498e
Compare
|
@sgiehl fyi, I think there are some changes in the bandwidth pr in the branch https://github.com/matomo-org/plugin-Bandwidth/tree/20425-fix-build. I recreated this pr but forgot that one. Apologies. |
a85b53e to
0df64ca
Compare
Description:
This PR adds new report metadata for the upcoming official looker studio integration. Looker studio requires connectors to provide structural metadata about the data the connectors query. This metadata is used to determine how the data is displayed and aggregated (within looker studio).
Matomo reports do not currently provide this information, so it needs to be added for the upcoming connector to use. Changes include:
Metrics:getDefaultMetricSemanticTypes()that behaves likegetDefaultMetricTranslations()and other methods.Metrics.getDefaultMetricSemanticTypesthat allows plugins to set semantic types.Note on terminology: the term "semantic type" is used by looker studio and I thought it was worth using here as well, since we already a column type (the database column types). Looker studio's docs on this are here: https://developers.google.com/looker-studio/connector/reference#semantictype
Originally I tried providing some simpler default logic based on the metric/record naming conventions matomo uses, but these conventions are not applied universally, and the number of exceptions made it too complicated. (For pre-5.0 Matomo versions, the connector will try to deduce this information from metric name, but this will be error prone compared to using explicit metadata.)
Review