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 event that allows plugins to archive 3rd party data #8631

Closed
diosmosis opened this Issue Aug 24, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@diosmosis
Member

diosmosis commented Aug 24, 2015

Currently, it is not possible to store 3rd party reports in archive tables (at least w/o some large hacks) due to this one optimization: https://github.com/piwik/piwik/blob/master/core/ArchiveProcessor/Loader.php#L121

If there are no visits for a period, the Archiver code for a plugin will not be launched. For plugins that only provide 3rd party data, there will never be visits in the log_visit table, so the archiver will never be launched.

To fix this for our LTS version, we can add an event that specifies idSites for whom this optimization should be skipped. This will allow many more plugins to be created for 2.15, while we work on 3.0.

Potential names for the new event include:

  • Archiver.getIdSitesNotUsingLogTables
  • Archiver.getIdSitesDisplayingThirdPartyData
  • Archver.getIdSitesDisplayingNonPiwikData
@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 26, 2015

Member

I thought quite for a while for a good name that actually describes what we're looking for but couldn't find any. For example one could display third party data but still reuse log_* tables etc eg when importing data from a 3rd party analytics system. After a while I thought how would someone import 3rd party data anyway? Why should they have to use the archiver? The archiver is pretty much there just for the log_* tables. At least currently it's not really built for something else and triggering such an event can be only a workaround (and I'm ok with that if it is needed for another plugin). As it's rather a workaround I don't mind which name we'll pick as none is really good. I'd probably prefer Archiver.getIdSitesNotUsingLogTables though as this is exactly the problem why they have to listen to this event. It's a bit technical but kinda accurate.

The ideal solution would be to either to refactor the archiver (which is planned for 3.0) and/or to offer a kinda new API for them, which could be even just a different method in the archiver for simplicity. For example next to aggregateDayReport() there would be maybe an additional method importDayReport(). We would always call this method even if there are no visits. Beside aggregateMultipleReports() there could be also an importMultipleReports(). It might be too confusing though and having another Import base class might be better. For example an Import base class would not need a method getLogAggregator() etc. The Import class name could be misleading and there would be potentially a better name for it.

I'm not sure how much work it is to introduce such new API but imagine it rather trivial, not sure though. When we detect a plugin defining an importer one would also maybe not have to listen to the CronArchiver.getIdSitesNotUsingTracker anymore since we would kinda know it is not using the tracker but importing data. The MultiSites plugin would still listen to that event since it's using the archiver.

Member

tsteur commented Aug 26, 2015

I thought quite for a while for a good name that actually describes what we're looking for but couldn't find any. For example one could display third party data but still reuse log_* tables etc eg when importing data from a 3rd party analytics system. After a while I thought how would someone import 3rd party data anyway? Why should they have to use the archiver? The archiver is pretty much there just for the log_* tables. At least currently it's not really built for something else and triggering such an event can be only a workaround (and I'm ok with that if it is needed for another plugin). As it's rather a workaround I don't mind which name we'll pick as none is really good. I'd probably prefer Archiver.getIdSitesNotUsingLogTables though as this is exactly the problem why they have to listen to this event. It's a bit technical but kinda accurate.

The ideal solution would be to either to refactor the archiver (which is planned for 3.0) and/or to offer a kinda new API for them, which could be even just a different method in the archiver for simplicity. For example next to aggregateDayReport() there would be maybe an additional method importDayReport(). We would always call this method even if there are no visits. Beside aggregateMultipleReports() there could be also an importMultipleReports(). It might be too confusing though and having another Import base class might be better. For example an Import base class would not need a method getLogAggregator() etc. The Import class name could be misleading and there would be potentially a better name for it.

I'm not sure how much work it is to introduce such new API but imagine it rather trivial, not sure though. When we detect a plugin defining an importer one would also maybe not have to listen to the CronArchiver.getIdSitesNotUsingTracker anymore since we would kinda know it is not using the tracker but importing data. The MultiSites plugin would still listen to that event since it's using the archiver.

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Aug 26, 2015

Member

This issue is just for enabling developers to display 3rd party data w/ as small a change as possible. 3.0 won't be out for 8 months or so, and I don't think plugin developers (ie, pro devs & their clients) should have to wait for that time to use this functionality. A new 'component' class might be the proper solution, but it doesn't make sense to write all that code when it may be removed for 3.0 and when a single pre-deprecated event will solve it too.

Regarding the name, since it will be removed eventually, I don't really have a preference. If you prefer technically accurate naming, maybe Archiver.getIdSitesWithNoVisitsToForceArchivingFor or Archiver.getIdSitesToForceArchivingForEvenIfNoVisits would do?

Member

diosmosis commented Aug 26, 2015

This issue is just for enabling developers to display 3rd party data w/ as small a change as possible. 3.0 won't be out for 8 months or so, and I don't think plugin developers (ie, pro devs & their clients) should have to wait for that time to use this functionality. A new 'component' class might be the proper solution, but it doesn't make sense to write all that code when it may be removed for 3.0 and when a single pre-deprecated event will solve it too.

Regarding the name, since it will be removed eventually, I don't really have a preference. If you prefer technically accurate naming, maybe Archiver.getIdSitesWithNoVisitsToForceArchivingFor or Archiver.getIdSitesToForceArchivingForEvenIfNoVisits would do?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 27, 2015

Member

I'm fine with any event name as long as it doesn't contain force since it leads to wrong expectations IMO. Eg because there can be still reasons why it would not be archived...

I'm fine with using the event as a workaround and to develop maybe a new base class in Piwik 3.0 for importing data.

Member

tsteur commented Aug 27, 2015

I'm fine with any event name as long as it doesn't contain force since it leads to wrong expectations IMO. Eg because there can be still reasons why it would not be archived...

I'm fine with using the event as a workaround and to develop maybe a new base class in Piwik 3.0 for importing data.

@diosmosis diosmosis self-assigned this Sep 18, 2015

@mattab mattab closed this in #8820 Sep 22, 2015

@mattab mattab changed the title from Create new event for LTS version that allows plugins to archive 3rd party data to New event that allows plugins to archive 3rd party data Oct 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment