Skip to content
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

New API parameter: hideColumns= to specify a list of columns not to return #3300

Closed
mattab opened this issue Aug 3, 2012 · 20 comments
Closed
Assignees
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Aug 3, 2012

In:
http://demo.piwik.org/?module=API&method=API.getReportMetadata&idSites=&period=day&date=today&format=xml&token_auth=anonymous&hideColumns=metricsDocumentation

or http://demo.piwik.org/?module=API&method=API.getProcessedReport&idSite=7&period=day&date=today&apiModule=UserCountry&apiAction=getCountry&showTimer=1&format=xml&token_auth=anonymous

It would be very useful to be able to NOT return the <metricsDocumentation> field since it takes much space. For Piwik Mobile transfert speed is key and extra data such as this makes the app much slower than necessary, esp. since Mobile phone internet trasnafer speed is often slow.

The new parameter would at least work for these API above, but it would be nice to have a generic parameter hideColumns that would work in all APIs automatically.

@diosmosis
Copy link
Member

What about a parameter that lists columns to show (ie, filter_show_columns)? This way new columns won't result in performance degradations.

For metric documentation, since it's always the same, perhaps it could be moved to another API function 'getMetricDocumentation'? Doesn't make sense to return it w/ every report.

What do you think of these ideas?

@mattab
Copy link
Member Author

mattab commented Aug 4, 2012

That could be a simpler solution indeed, to return metrics documentation in a different API...

listing columns to show might be more tricky. OK to move the <metricsDocumentation> to a separate API call :)

I'll ask Thomas if that'd work

@diosmosis
Copy link
Member

I've been thinking about this some more, and I've revised my solution. Using one getMetricsDocumentation function won't work since the docs can differ based on plugin (for example, the Actions plugin defines a different documentation for nb_visits than the default docs). Instead I think we should do the following:

  • Expose getDefaultMetricsDocumentation as an API function.
  • Modify getProcessedReport to return no metrics documentation.
  • Modify getMetadata & getReportMetadata to return only custom metrics documentation (so they won't return the default docs).

Do you think this will work?

@diosmosis
Copy link
Member

Actually, just came up w/ a better idea, I'll upload a patch in a bit.

@mattab
Copy link
Member Author

mattab commented Aug 6, 2012

I think it's simpler to simply have a hideMetricsDoc=1 parameter?
the goal is really to make Piwik Mobile app faster and this is the onyl piece of data unused, so it's better / OK to do the quick fix here :)

@diosmosis
Copy link
Member

Yes that was my idea :)

@diosmosis
Copy link
Member

(In [6682]) Refs #3300, added hideMetricsDoc parameter to metadata API actions that can return metrics documentation (such as getProcessedReport, getReportMetadata & getMetadata).

@diosmosis
Copy link
Member

(In [6684]) Refs #3300, test hideMetricsDoc w/ getMetadata instead of getReportMetadata.

@diosmosis
Copy link
Member

Attachment: Patch for this issue (implements filter_hide_columns/filter_show_columns).
3300.diff.tar.gz

@diosmosis
Copy link
Member

I've put up a patch that implements the other part of this issue. It will allow you to use two new filter parameters: filter_hide_columns/filter_show_columns that will remove columns from the API result.

One thing to note: This patch allows filter_hide_columns or filter_show_columns to be specified. The way I've implemented this in the patch will result in the RemoveColumns filter running even if the parameters aren't supplied. It shouldn't result in any added overhead though, since nothing will happen when there are no parameters. But if it should be changed, I can fix it by creating two small sub-classes (RemoveColumnsWhitelist/RemoveColumnsBlacklist) in RemoveColumns.php.

Let me know what you think.

@mattab
Copy link
Member Author

mattab commented Aug 7, 2012

Cool feature.

  • DataTable->deleteColumn(s) should be used to delete columns
  • at some point we should think of renaming filters, I dislike the name filter_xx for existing ones... for example this could be exposed via hideColumns showColumns
  • Tests: can you add a test of filtering out columns in processed reports: should it remove first level "entities" or simply remove columsn from the reportData ? It should remove the columns from the report itself (so expected that hideColumns=metricsDocumentation does not work)
  • Tests: add a test of expanded=1 and showing column is removed recursively (done via deleteColumn())

@diosmosis
Copy link
Member

Attachment: New patch for hide/show columns.
3300.diff.tar.2.gz

@diosmosis
Copy link
Member

I uploaded a new patch.

Replying to matt:

Cool feature.

  • DataTable->deleteColumn(s) should be used to delete columns

This didn't work too well w/ showColumns so I took a different approach to make sure subtables got filtered.

  • at some point we should think of renaming filters, I dislike the name filter_xx for existing ones... for example this could be exposed via hideColumns showColumns
  • Tests: can you add a test of filtering out columns in processed reports: should it remove first level "entities" or simply remove columsn from the reportData ? It should remove the columns from the report itself (so expected that hideColumns=metricsDocumentation does not work)

I got this to work, but I had to add a bit of a hack to API/API.php.

  • Tests: add a test of expanded=1 and showing column is removed recursively (done via deleteColumn())

Added :). Let me know what you think of this patch.

One other thing to note: Right now, this filter won't work w/ some columns, since they are added via DataTable::queueFilter. At the moment, queued filters are run after generic filters, so it won't affect them. Should this change?

@mattab
Copy link
Member Author

mattab commented Aug 11, 2012

Thanks for the new patch, the code is much better indeed :-)

Review:

  • What happens if all columns removed? or if column specified is not showing? throw exception?
  • hack in API.php OK :) we don't have much choice indeed!
  • Tests look good!
  • The parameters must work with processed metrics indeed. Maybe the filter should not be generic but be applied manually here
Index: core/API/ResponseBuilder.php
===================================================================
--- core/API/ResponseBuilder.php    (revision 6707)
+++ core/API/ResponseBuilder.php    (working copy)
@@ -301,6 +301,8 @@
            $datatable->applyQueuedFilters();
        }

+       // HERE call RemoveColumns filter manually?
+       
        // if requested, flatten nested tables
        if (Piwik_Common::getRequestVar('flat', '0', 'string', $this->request) == '1')
        {
  • in tests can you please add more excluded columns, so the output is more simple even? (it's better that tests fail less often but would still fail whenever needed)
  • Can you add a test when all columns excluded or column showColumns= does not exist?
  • Also add processed metrics in the tests :)

@diosmosis
Copy link
Member

Attachment: Another patch.
3300.diff.tar.3.gz

@diosmosis
Copy link
Member

Replying to matt:

Thanks for the new patch, the code is much better indeed :-)

Review:

  • What happens if all columns removed? or if column specified is not showing? throw exception?

If hideColumns specifies every column that is to be returned, an empty result is returned. If showColumns is empty, nothing happens. Do you think these are appropriate behaviors?

  • hack in API.php OK :) we don't have much choice indeed!
  • Tests look good!
  • The parameters must work with processed metrics indeed. Maybe the filter should not be generic but be applied manually here
Index: core/API/ResponseBuilder.php
===================================================================
--- core/API/ResponseBuilder.php  (revision 6707)
+++ core/API/ResponseBuilder.php  (working copy)
@@ -301,6 +301,8 @@
          $datatable->applyQueuedFilters();
      }

+     // HERE call RemoveColumns filter manually?
+     
      // if requested, flatten nested tables
      if (Piwik_Common::getRequestVar('flat', '0', 'string', $this->request) == '1')
      {

This is what I ended up doing. It won't work for ViewDataTable, but I'm not sure that's an issue.

  • in tests can you please add more excluded columns, so the output is more simple even? (it's better that tests fail less often but would still fail whenever needed)
  • Can you add a test when all columns excluded or column showColumns= does not exist?
  • Also add processed metrics in the tests :)

I've added a new patch w/ these tests. Let me know if it's ok to commit.

@mattab
Copy link
Member Author

mattab commented Aug 19, 2012

Feedback:

  • in handleTableReport(), refactor the code dealing with columns in a new private method to keep things tidy
  • in one of the tests, can you add for both hide/show a column name that does not exist, to check the code doesn't trigger errors/notices?

Looks good to commit!

@diosmosis
Copy link
Member

(In [6836]) Fixes #3300, added hideColumns/showColumns query parameters to customize API output & modified ColumnDelete filter to be more robust.

@diosmosis
Copy link
Member

Replying to matt:

Feedback:

  • in handleTableReport(), refactor the code dealing with columns in a new private method to keep things tidy
  • in one of the tests, can you add for both hide/show a column name that does not exist, to check the code doesn't trigger errors/notices?

I already do this (see 'nb_hits' in hide/showColumns in the VisitsSummary.get tests).

Looks good to commit!

I noticed that there was already a ColumnDelete filter, so I removed my RemoveColumns filter and augmented that one before I committed.

@diosmosis
Copy link
Member

(In [6837]) Refs #3300, tweak to code comment

@mattab mattab added this to the 1.12.x - Piwik 1.12.x milestone Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

2 participants