@mattab opened this Pull Request on November 4th 2014 Owner
@mattab commented on November 10th 2014 Owner

Here is my feedback

  • There are some UI tests failing showing some bugs (Overlay, Contents, etc) : http://builds-artifacts.piwik.org/ui-tests.master/5655.1/screenshot-diffs/diffviewer.html
  • in AveragePageGenerationTime I see: getQuotientSafe($sumGenerationTime, $hitsWithTimeGeneration, $precision = 3) but in AverageTimeOnPage there is $precision = 0 - is it normal they have different precision?
  • move const EXTRA_PROCESSED_METRICS_METADATA_NAME from DataTable to another class
  • typo in getDependenctMetrics
  • maybe Report class should hold in processedMetrics an array of ProcessedMetrics instead of a mix of strings and objects?
  • could Metric::getMetric helper be moved to another class to keep the base class clean?
  • Can remove this TODO as it is not needed IMO TODO should create ratio & percent processed metric base types
  • TODO: this may cause regression w/ #2531, check?
    Does not seem the right ticket number - can you tell me more about what regression risk there is?

  • Move the new ProcessedMetrics classes from their 'Metrics/' folder into the Columns/ folder to be consistent with existing columns and dimensions put there. Eg Actions plugins columns.

This is a good effort! Hopefully you can finish in next day or so so we can release beta ASAP in time for 2.9.0 release on Thursday :+1:

@diosmosis commented on November 10th 2014 Member

in AveragePageGenerationTime I see: getQuotientSafe($sumGenerationTime, $hitsWithTimeGeneration, $precision = 3) but in AverageTimeOnPage there is $precision = 0 - is it normal they have different precision?

They are different metrics. In the existing code, they have different precisions.

move const EXTRA_PROCESSED_METRICS_METADATA_NAME from DataTable to another class

Where would I move it to and why?

maybe Report class should hold in processedMetrics an array of ProcessedMetrics instead of a mix of strings and objects?

Need to remain backwards compatible.

could Metric::getMetric helper be moved to another class to keep the base class clean?

Yes, but if all other metrics are converted, the indexes & names become implementation details for the core system, and this method can be removed completely. No reason to move it now.

Can remove this TODO as it is not needed IMO TODO should create ratio & percent processed metric base types

Ok, I don't think it's super necessary either.

TODO: this may cause regression w/ #2531, check? Does not seem the right ticket number - can you tell me more about what regression risk there is?

It's an old ticket, I can't remember what, but the TODO should be removed. If MultiSites.getAll works, there's no regression, and MultiSites.getAll works.

Move the new ProcessedMetrics classes from their 'Metrics/' folder into the Columns/ folder to be consistent with existing columns and dimensions put there. Eg Actions plugins columns.

Columns are not metrics. Columns refer to dimensions for tracked entities. They are Database columns, not DataTable columns. This would be inappropriate and confusing.

@mattab commented on November 10th 2014 Owner

Where would I move it to and why?

because this const is not actually used in DataTable and un-related to DataTable, it's an implementation detail of how we use metadata. Same actually applies to EMPTY_COLUMNS_METADATA_NAME and TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME etc. so maybe you can leave it there and we fix all those at once later...

Yes, but if all other metrics are converted

Ah ok, didn't catch this! could you note to create an issue with the scope of "converting metrics" to the new system? I'm curious the effort there and if it's worth we work on this?

@mattab commented on November 10th 2014 Owner

Move the new ProcessedMetrics classes from their 'Metrics/' folder into the Columns/ folder to be consistent with existing columns and dimensions put there. Eg Actions plugins columns.

Columns are not metrics. Columns refer to dimensions for tracked entities. They are Database columns, not DataTable columns. This would be inappropriate and confusing.

you make a good point, I'll think about it cc @tsteur

They are different metrics. In the existing code, they have different precisions.

ok

Need to remain backwards compatible.

that's a good reason :+1:

@diosmosis commented on November 10th 2014 Member

Ah ok, didn't catch this! could you note to create an issue with the scope of "converting metrics" to the new system? I'm curious the effort there and if it's worth we work on this?

The issue is here: https://github.com/piwik/piwik/issues/6262 . It would involve breaking open the 'archiving' system and making the point of extension metadata classes instead of Archiver classes. It would likely be a lot of work, but not as much as #5363 turned out to be, I think. If you want Piwik to be a solid and lean framework/platform like rails then it would be a good idea. If you'd like Piwik to remain an application with hooks and events then you could spend your time on other things.

@tsteur commented on November 10th 2014 Owner

Move the new ProcessedMetrics classes from their 'Metrics/' folder into the Columns/ folder to be consistent with existing columns and dimensions put there. Eg Actions plugins columns.

Columns are not metrics. Columns refer to dimensions for tracked entities. They are Database columns, not DataTable columns. This would be inappropriate and confusing.

you make a good point, I'll think about it cc @tsteur

The dimensions were initially in a Dimensions folder but we moved them to a Columns folder as we also wanted to put the Metrics there. I've never seen it as "Database column" but always as "DataTable column". That it is stored in the database is just a detail and there are even dimension classes that are actually not stored in the database (I think they only define a name or so).

I agree it is certainly more confusing for us core developers but shouldn't be so important as we are building a platform for other developers (they are our users). For plugin developers, who don't know much about Piwik core, it could be - maybe - even more confusing having them separated as they maybe only think of columns of reports and are not so familiar with the different terms etc. Maybe it is not so important in the end as the generator will place it in the correct folder anyway. Maybe the name Columns is not ideal and it should be placed somewhere else? eg Reports/Columns.

If we leave it in Metrics folder we should definitely move Columns folder to Dimensions as it would otherwise be really confusing.

@diosmosis commented on November 10th 2014 Member

I've never seen it as "Database column" but always as "DataTable column".

Are dimensions ever stored in DataTables the same way as metrics? Dimensions just describe what the 'label' column represents, no?

'Columns' is an incorrect name to me. nb_visits is calculated as the COUNT(*) in SQL queries. The fact that it is calculated is what makes it a metric. A dimension, like IdGoal, is not calculated, it is just stored as part of a tracked entity.

@mattab commented on November 24th 2014 Owner

e1098939 looks really risky could we revert this? I have no idea of the implications besides that such change worries me much

Edit: OK it was reverted already!

@mattab commented on November 24th 2014 Owner

Feedback:

  • getSiteIdFromMetadata should not be in the DataTable class but somewhere else eg. close to code where 'site' metadata is written in datatable
  • GetSiteSearchNoResultKeywords and GetSiteSearchKeywords and also Pages Following a Site Search reports should not have the following processed metrics:
    new AverageTimeOnPage(), new BounceRate(), new ExitRate(), new AveragePageGenerationTime()
    • actually these metrics are not relevant to the user in the context of viewing site search reports. they make scheduled reports more complicated so let's remove those extra columns.
  • similarly in Entry pages report processed should have metrics Entry Page URL Entrances Bounces Bounce Rate but now it has also exit_rate, avg_time_generation, avg_time_on_page -> let's remove these to KISS for Scheduled reports Entry/Exit pages
  • Similary for Exit pages the metrics displayed in processed report should be only those ones Exit Page URL Exits Unique Pageviews Exit rate.
    • why? Entry/Exit reports are kinda like "views" showing a subset of columns, this is on purpose to make a point and make it easy to spot good and bad entry and exit pages.
    • <avg_order_revenue>Average Order Value</avg_order_revenue> should be added to Ecommerce goals only. Got: this processed metric was added to each goal.

Looking forward to seeing it merge, please do merge it once you're done with your TODO and feedback :+1:

@diosmosis commented on November 24th 2014 Member

similarly in Entry pages report processed should have metrics Entry Page URL Entrances Bounces Bounce Rate but now it has also exit_rate, avg_time_generation, avg_time_on_page -> let's remove these to KISS for Scheduled reports Entry/Exit pages

These should be removed from API.getProcessedReport but present for normal report (for BC) as requested in comments above. Where do you see them in API.getProcessedReport?

Similary for Exit pages the metrics displayed in processed report should be only those ones Exit Page URL Exits Unique Pageviews Exit rate.

Same as above.

@mattab commented on November 25th 2014 Owner

These should be removed from API.getProcessedReport but present for normal report (for BC) as requested in comments above. Where do you see them in API.getProcessedReport?

It took me a while to find it again in this huuuuge diff but here it is:

@diosmosis commented on November 25th 2014 Member

Entry pages has and and in metadata definition:

Entry pages has metrics documentation for exit_rate, is that what you mean? It is not elsewhere in the report.

@diosmosis commented on November 25th 2014 Member

GetSiteSearchNoResultKeywords and GetSiteSearchKeywords and also Pages Following a Site Search reports should not have the following processed metrics: new AverageTimeOnPage(), new BounceRate(), new ExitRate(), new AveragePageGenerationTime()

Should these metrics be removed from raw reports or just from API.getProcessedReport?

@mattab commented on November 25th 2014 Owner

Entry pages has metrics documentation for exit_rate, is that what you
mean? It is not elsewhere in the report.

Yes that's what I mean, it would be more clear/consisten if metrics not
in the list of metrics would be removed from the metric documentation +1

@mattab commented on November 25th 2014 Owner

Just remove from Processed Report

This Pull Request was closed on November 27th 2014
Powered by GitHub Issue Mirror