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

Move logs to custom tables #3841

Closed
pippinsplugins opened this Issue Sep 28, 2015 · 31 comments

Comments

@pippinsplugins
Member

pippinsplugins commented Sep 28, 2015

API requests are logged by default with the EDD_Logging class. Each site that uses the API has potentially thousands (millions?) of API requests that are going to get logged.

Currently they get stored in wp_posts/wp_postmeta.

For the most part, they just sit there taking up space and potentially (most likely) slowing down other queries on the site that pull from wp_posts/wp_postmeta.

I propose we move the request logs to a new custom table. This will provide the following advantages:

  • It will remove a TON of data from posts/postmeta
  • It will give us a second (customers was first) test process for the eventual move of payments to a custom table. Requests logs are largely unused so will be far more forgiving of any errors or mistakes made in the migration
  • We'll use one DB row per log entry instead of 5-10 across two tables
  • It will speed up API requests since we're creating one DB entry instead of 5-10
  • It will speed up display of the request logs
  • It will make it easier to introduce future enhancements (such as log pruning)

Pull Request: #6264

@pippinsplugins pippinsplugins added this to the 2.6 milestone Sep 28, 2015

@pippinsplugins pippinsplugins added the API label Sep 28, 2015

@cklosowski cklosowski self-assigned this Jan 15, 2016

@pippinsplugins pippinsplugins modified the milestones: 2.8, 2.6 May 4, 2016

@brashrebel

This comment has been minimized.

Show comment
Hide comment
@brashrebel

brashrebel Aug 11, 2016

Contributor

Just curious, why limit this to only the API requests? Why not the whole EDD_Logging class? I've got a use case right now where I want to use the EDD_Logging class because it makes sense and is pretty easy to extend...except it stores all logs in wp_posts so...I have to be really careful about saving tons and tons of log entries there. A separate table for all logs would be uber much neato.

Contributor

brashrebel commented Aug 11, 2016

Just curious, why limit this to only the API requests? Why not the whole EDD_Logging class? I've got a use case right now where I want to use the EDD_Logging class because it makes sense and is pretty easy to extend...except it stores all logs in wp_posts so...I have to be really careful about saving tons and tons of log entries there. A separate table for all logs would be uber much neato.

@pippinsplugins

This comment has been minimized.

Show comment
Hide comment
@pippinsplugins

pippinsplugins Aug 12, 2016

Member

For this particular case, we want API logs separate from other logs. I can easily see us moving the main logging class too.

Member

pippinsplugins commented Aug 12, 2016

For this particular case, we want API logs separate from other logs. I can easily see us moving the main logging class too.

@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal Dec 23, 2017

Member

Proposed database schema

There are two new tables being introduced:

  • wp_edd_logs
  • wp_edd_logmeta

Schema for wp_edd_logs:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
parent_id bigint(20) unsigned NO NULL
type varchar(30) NO   NULL  
title varchar(200) NO   NULL  
message longtext NO   NULL  
date_created datetime NO    

Schema for wp_edd_logmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
edd_log_id bigint(20) NO MUL NULL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  
Member

sunnyratilal commented Dec 23, 2017

Proposed database schema

There are two new tables being introduced:

  • wp_edd_logs
  • wp_edd_logmeta

Schema for wp_edd_logs:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
parent_id bigint(20) unsigned NO NULL
type varchar(30) NO   NULL  
title varchar(200) NO   NULL  
message longtext NO   NULL  
date_created datetime NO    

Schema for wp_edd_logmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
edd_log_id bigint(20) NO MUL NULL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  
@pippinsplugins

This comment has been minimized.

Show comment
Hide comment
@pippinsplugins

pippinsplugins Dec 25, 2017

Member

I think the schema is good. To help confirm, here's a list of the log types I'm currently aware of that will be stored:

  • API requests
  • License activation / deactivation (or are these separate in EDD SL @cklosowski?)
  • Payment gateway errors
  • File downloads

We do have an opportunity here to merge logs and some notes together. We've talked about a dedicated notes table, but what if we instead made notes part of the logs table? It would make sense for at least some of the data that's currently stored as notes. For example, payments store "comments" whenever the status of a payment changes. Subscriptions from EDD Recurring do this as well. It makes logical sense for those to be logs as they're really a historical event much more so than a note.

Member

pippinsplugins commented Dec 25, 2017

I think the schema is good. To help confirm, here's a list of the log types I'm currently aware of that will be stored:

  • API requests
  • License activation / deactivation (or are these separate in EDD SL @cklosowski?)
  • Payment gateway errors
  • File downloads

We do have an opportunity here to merge logs and some notes together. We've talked about a dedicated notes table, but what if we instead made notes part of the logs table? It would make sense for at least some of the data that's currently stored as notes. For example, payments store "comments" whenever the status of a payment changes. Subscriptions from EDD Recurring do this as well. It makes logical sense for those to be logs as they're really a historical event much more so than a note.

@sunnyratilal sunnyratilal changed the title from Move API requests log to a custom table to Move logs to a custom table Dec 25, 2017

@sunnyratilal sunnyratilal added Priority: High and removed API labels Dec 25, 2017

@sunnyratilal sunnyratilal self-assigned this Dec 25, 2017

@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal Dec 25, 2017

Member

I like it. We could link the payment by adding in an object_id column to the wp_edd_logs table so a payment note would look like this:

id parent_id object_id type title message date_created
1 4 payment Status change Status changed from Pending to Complete 2017-12-25 01:54:00

One thing to note is that we currently a user ID can be attached to a payment note (because it's part of WP_Comment) - would we need to port that across if we decide to merge notes and logs?

Member

sunnyratilal commented Dec 25, 2017

I like it. We could link the payment by adding in an object_id column to the wp_edd_logs table so a payment note would look like this:

id parent_id object_id type title message date_created
1 4 payment Status change Status changed from Pending to Complete 2017-12-25 01:54:00

One thing to note is that we currently a user ID can be attached to a payment note (because it's part of WP_Comment) - would we need to port that across if we decide to merge notes and logs?

@pippinsplugins

This comment has been minimized.

Show comment
Hide comment
@pippinsplugins

pippinsplugins Dec 26, 2017

Member

We will need a creator_id, author_id, user_id, or similar to record who made a note.

What's a scenario where we need parent|child relationships for logs?

Member

pippinsplugins commented Dec 26, 2017

We will need a creator_id, author_id, user_id, or similar to record who made a note.

What's a scenario where we need parent|child relationships for logs?

@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal Dec 27, 2017

Member

Ah, I forgot to remove that, not quite so sure why I added parent_id 😄

Member

sunnyratilal commented Dec 27, 2017

Ah, I forgot to remove that, not quite so sure why I added parent_id 😄

@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal Dec 27, 2017

Member

Proposed database schema (version 2)

There are two new tables being introduced:

  • wp_edd_logs
  • wp_edd_logmeta

Schema for wp_edd_logs:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
object_id bigint(20) unsigned YES MUL NULL
type varchar(30) NO    
title varchar(200) NO    
message longtext NO    
date_created datetime NO    
user_id bigint(20) unsigned NO 0

Schema for wp_edd_logmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
edd_log_id bigint(20) NO MUL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  

Note: the default user_id is set to 0. If this is the case, we assume it's EDD Bot.

Member

sunnyratilal commented Dec 27, 2017

Proposed database schema (version 2)

There are two new tables being introduced:

  • wp_edd_logs
  • wp_edd_logmeta

Schema for wp_edd_logs:

Field Type Null Key Default Extra
id bigint(20) unsigned NO PRI NULL auto_increment
object_id bigint(20) unsigned YES MUL NULL
type varchar(30) NO    
title varchar(200) NO    
message longtext NO    
date_created datetime NO    
user_id bigint(20) unsigned NO 0

Schema for wp_edd_logmeta:

Field Type Null Key Default Extra
meta_id bigint(20) NO PRI NULL auto_increment
edd_log_id bigint(20) NO MUL  
meta_key varchar(255) YES MUL NULL  
meta_value longtext YES   NULL  

Note: the default user_id is set to 0. If this is the case, we assume it's EDD Bot.

@pippinsplugins

This comment has been minimized.

Show comment
Hide comment
@pippinsplugins

pippinsplugins Dec 27, 2017

Member

@sunnyratilal In your example above I think payment should be payment_note.

Member

pippinsplugins commented Dec 27, 2017

@sunnyratilal In your example above I think payment should be payment_note.

@mindctrl

This comment has been minimized.

Show comment
Hide comment
@mindctrl

mindctrl Dec 27, 2017

Member

Probably not a popular opinion, but I'm not convinced we should introduce a global log/note table. I agree with what Christoff said here, that payment notes should be in their own table.

Having a global logs table means we automatically have to query/filter based on the object type before we can use any of the data. That introduces overhead and the possibility of people making mistakes with the data. If we have to filter out unrelated data every time we fetch from the table, it's a good indicator the table structure isn't right.

Is there any use case where all the log data will be needed without filtering by type? If not, I think that's a good case for separate tables.

Also API logs, license logs, and download logs all have the potential to generate tons of entries. Along with payments, each of these logs types is slightly different and may require a different data structure down the road, if not from day one. Compromising on a more abstracted, global data structure will result in us relying on meta data for the case where there's not a column or where adding one doesn't make sense for all logs types, which will undo much of the performance benefits we get from going with custom tables.

Another thought. Conceptually it could be argued that logs and notes are different things, primarily that one is an automated activity log and the other is supplied by a human.

Member

mindctrl commented Dec 27, 2017

Probably not a popular opinion, but I'm not convinced we should introduce a global log/note table. I agree with what Christoff said here, that payment notes should be in their own table.

Having a global logs table means we automatically have to query/filter based on the object type before we can use any of the data. That introduces overhead and the possibility of people making mistakes with the data. If we have to filter out unrelated data every time we fetch from the table, it's a good indicator the table structure isn't right.

Is there any use case where all the log data will be needed without filtering by type? If not, I think that's a good case for separate tables.

Also API logs, license logs, and download logs all have the potential to generate tons of entries. Along with payments, each of these logs types is slightly different and may require a different data structure down the road, if not from day one. Compromising on a more abstracted, global data structure will result in us relying on meta data for the case where there's not a column or where adding one doesn't make sense for all logs types, which will undo much of the performance benefits we get from going with custom tables.

Another thought. Conceptually it could be argued that logs and notes are different things, primarily that one is an automated activity log and the other is supplied by a human.

@cklosowski

This comment has been minimized.

Show comment
Hide comment
@cklosowski

cklosowski Dec 27, 2017

Member
Member

cklosowski commented Dec 27, 2017

@chriscct7

This comment has been minimized.

Show comment
Hide comment
@chriscct7

chriscct7 Dec 27, 2017

Member
Member

chriscct7 commented Dec 27, 2017

@pippinsplugins

This comment has been minimized.

Show comment
Hide comment
@pippinsplugins

pippinsplugins Dec 27, 2017

Member

Notes schema proposed by @JJJ here is good.

If we separate logs and notes, where do status changes fall? Are they a log or a note? They be be triggered automatically or by a store owner.

Member

pippinsplugins commented Dec 27, 2017

Notes schema proposed by @JJJ here is good.

If we separate logs and notes, where do status changes fall? Are they a log or a note? They be be triggered automatically or by a store owner.

@mindctrl

This comment has been minimized.

Show comment
Hide comment
@mindctrl

mindctrl Dec 27, 2017

Member

Status change would be a log. A note is user supplied text, not a log of activity.

Member

mindctrl commented Dec 27, 2017

Status change would be a log. A note is user supplied text, not a log of activity.

@mindctrl

This comment has been minimized.

Show comment
Hide comment
@mindctrl

mindctrl Dec 27, 2017

Member

I don't want to get too caught up on note vs log. I'm more interested in getting the schema right.

Member

mindctrl commented Dec 27, 2017

I don't want to get too caught up on note vs log. I'm more interested in getting the schema right.

sunnyratilal added a commit that referenced this issue Dec 28, 2017

sunnyratilal added a commit that referenced this issue Mar 31, 2018

sunnyratilal added a commit that referenced this issue Mar 31, 2018

sunnyratilal added a commit that referenced this issue Mar 31, 2018

sunnyratilal added a commit that referenced this issue Mar 31, 2018

sunnyratilal added a commit that referenced this issue Mar 31, 2018

sunnyratilal added a commit that referenced this issue Mar 31, 2018

@sunnyratilal sunnyratilal moved this from Todo to Done in 3DD™: Custom Tables Apr 6, 2018

@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal Apr 9, 2018

Member

As per a discussion in Slack, we won't expose public API to allow editing of logs.

Member

sunnyratilal commented Apr 9, 2018

As per a discussion in Slack, we won't expose public API to allow editing of logs.

@sunnyratilal sunnyratilal moved this from Schema Changes to Done in Release 3.0 Sprint May 1, 2018

JJJ added a commit that referenced this issue May 23, 2018

Update Logs UI to use new logs APIs. See #3841.
* Tweak filter CSS to accomodate another admin screen
* Consolidate Logs views into like-methods & functions to reduce duplication
* Introduce base Logs List Table class to handle shared code across all tables
* Add date boundaries (required modifications to base query class)
@sunnyratilal

This comment has been minimized.

Show comment
Hide comment
@sunnyratilal

sunnyratilal May 29, 2018

Member

Closing out as this has been completed. Any bugs found during alpha/beta testing should be opened as new issues.

Member

sunnyratilal commented May 29, 2018

Closing out as this has been completed. Any bugs found during alpha/beta testing should be opened as new issues.

sunnyratilal added a commit that referenced this issue Jun 6, 2018

sunnyratilal added a commit that referenced this issue Jun 7, 2018

sunnyratilal added a commit that referenced this issue Jun 10, 2018

Core object & database table tweaks:
* Allow direct access to object variables using magic getter.
* Remove `get_*()` methods.
* Add @Property notation for all the classes for IDE type-hinting.
* Rename `payment_id` to `order_id` on file download logs table.
* Re-order columns to put `discount` before `tax`.
* Add $date_modified to Log classes.
* Remove meta methods from Core objects.

#6651 #3841

sunnyratilal added a commit that referenced this issue Jun 10, 2018

Core object & database table tweaks:
* Allow direct access to object variables using magic getter.
* Remove `get_*()` methods.
* Add @Property notation for all the classes for IDE type-hinting.
* Rename `payment_id` to `order_id` on file download logs table.
* Re-order columns to put `discount` before `tax`.
* Add $date_modified to Log classes.
* Remove meta methods from Core objects.

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