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

limit the size of product reports #17823

Merged
merged 4 commits into from Aug 4, 2021
Merged

limit the size of product reports #17823

merged 4 commits into from Aug 4, 2021

Conversation

diosmosis
Copy link
Member

Description:

Applying row limits to product reports as we do to most other reports. This PR does not include the use of ranking query, just two new INI config options to limit the report sizes.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty straightforward. Do we need to mention these new settings in any documentation (eg here https://matomo.org/faq/how-to/faq_54/)?

@diosmosis
Copy link
Member Author

👍 makes sense to

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Do we need to mention that in any changelog? Before the report was actually unlimited and now it's truncated to 500 by default. The change might be relevant for those tracking more than 500 different products...

@tsteur
Copy link
Member

tsteur commented Jul 28, 2021

@sgiehl it'll be mentioned in the regular changelog later as part of the release

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jul 29, 2021
@mattab
Copy link
Member

mattab commented Jul 30, 2021

Ideally we would set it to a higher value by default, as Ecommerce store owners have an expectation that the product report is accurate, and 500 products out of say 10,000 unique products would be quite inaccurate. For User ID we set a limit of 50,000. Maybe we could set 10,000 for products?

@diosmosis
Copy link
Member Author

Updated. Also removed the subtable setting since the products reports don't have subtab.es.

@diosmosis diosmosis merged commit 3efc65b into 4.x-dev Aug 4, 2021
@diosmosis diosmosis deleted the 17816-product-limits branch August 4, 2021 20:40
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiting ecommerce product reports to 10,000 to avoid memory issues with new INI config setting to customise
5 participants