Navigation Menu

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

Jump to bottom

Site content detection should be done in one place #20018

Closed
justinvelluppillai opened this issue Nov 15, 2022 · 3 comments
Closed

Site content detection should be done in one place #20018

justinvelluppillai opened this issue Nov 15, 2022 · 3 comments
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone
4.14.0

Comments

@justinvelluppillai
Copy link
Contributor

The new Consent Manager, GA3, GA4, GTM detection mechanisms and existing GtmSiteTypeGuesser class don't check the enable_internet_features config setting before calling out to the internet. It should check this config and do nothing if it is set to 0.

As part of this task the GtmSiteTypeGuesser and new SiteContentDetector could be improved to share code or at least logic - eg the SiteContentDetector caches for 7 days, and GtmSiteTypeGuesser caches for only 1 day, and GtmSiteTypeGuesser avoids requests to certain hosts (see f839cb9#diff-47ed7c5049ee406ef66904e382872672fc2bd9864140609e7918b871f989ec6aR185)
@justinvelluppillai justinvelluppillai added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. To Triage An issue awaiting triage by a Matomo core team member labels Nov 15, 2022
@justinvelluppillai justinvelluppillai mentioned this issue Nov 15, 2022
Merged
11 tasks
@bx80
Copy link
Contributor

bx80 commented Nov 15, 2022

I think it should be fairly straightforward to move the GtmSiteTypeGuesser functionality into SiteContentDetector which would then unify the caching and host blocking settings. SiteContentDetector was intended to support detection of multiple site properties. This would also reduce the request count by half. It might also make sense to move SiteContentDectector to the SitesManager plugin at the same time.

@justinvelluppillai
Copy link
Contributor Author

I doubt there is much depending on the GtmSiteTypeGuesser class yet but this might be something we remove in Matomo 5.0.0 to avoid breaking anything for anyone.

@justinvelluppillai justinvelluppillai removed the To Triage An issue awaiting triage by a Matomo core team member label Nov 15, 2022
@justinvelluppillai justinvelluppillai added this to the For Prioritization milestone Nov 15, 2022
@justinvelluppillai justinvelluppillai changed the title Site content detection should respect enable_internet_features config Site content detection should be done in one place Nov 15, 2022
@mattab mattab modified the milestones: For Prioritization, 4.14.x Mar 27, 2023
@tsteur
Copy link
Member

tsteur commented Mar 28, 2023

I believe this was done in #20381 and could be closed now. In cloud we will be using this now as well.

@mattab mattab closed this as completed Jun 1, 2023
@sgiehl sgiehl closed this as completed Jun 15, 2023
@sgiehl sgiehl modified the milestones: 4.15.0, 4.14.0 Jul 5, 2023
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.14.0
Development

No branches or pull requests
6 participants
@tsteur @mattab @justinvelluppillai @sgiehl @bx80 @innocraft-automation