All logging should be our own file

michaeltorbert commented 8 years ago

Our log function should start writing to a log file instead of the database or error log as appropriate. We should figure out ways of tagging certain information... For example using JSON (even better is ndjson):

[{date:'2012-01-01 02:00:01', severity:"ERROR", msg:"Foo failed"}, {date:'2012-01-01 02:04:02', severity:"INFO", msg:"Bar was successful"}, {date:'2012-01-01 02:10:12', severity:"DEBUG", msg:"Baz was notified"},

http://ndjson.org/

michaeltorbert commented 8 years ago

` $date = date("Y-m-d h:m:s"); $file = FILE; $level = "warning";

$message = "[{$date}] [{$file}] [{$level}] Put your message here".PHP_EOL; // log to our default location error_log($message);

// error.log [2015-08-14 09:08:23] [/var/www/html/index.php] [warning] My error message here!

function logger($message, array $data, $logFile = "error.log"){
foreach($data as $key => $val){
$message = str_replace("%{$key}%", $val, $message);
}
$message .= PHP_EOL;

return file_put_contents($logFile, $message, FILE_APPEND); }

logger("%file% %level% %message%", ["level" => "warning", "message" =>"This is a message", "file" =>FILE]); `

michaeltorbert commented 7 years ago

This is a good UI for logging. See how they do it... file or db.

contactashish13 commented 6 years ago

@michaeltorbert I would suggest logging in the database as a transient field with a limited timespan of X minutes (X ~ 10 minutes). This would also be triggered only when debug / similar flag is enabled.

contactashish13 commented 5 years ago

@michaeltorbert I've worked on an implementation of this PR https://github.com/semperfiwebdesign/all-in-one-seo-pack/pull/2204 and this is how it looks (just an example)

The salient points are:

it logs in the database because I don't think using the filesystem is reliable when it comes to different hosting providers. As a backup and for testing on local/dev instances, it also logs to error log
it logs into one transient variable. It logs the last 100 requests (configurable through a filter) and caches them upto 5 minutes (configurable through a filter)
it logs only when the Log events is checked otherwise it does not log
the implementation provided is both as an action hook (preferable) and as a function so that it can be extended further if required. do_action( 'aiosp_log', 'some error message', 'warn', __FILE__, __LINE__ );
the radio buttons can be used to toggle what kind of logs to see. This filter is through CSS/JS not through ajax
it captures the location (file and line number) of the log statement
a download functionality can also be added that downloads the logs as a CSV file for users to send the logs to us
if this looks fine, future code should contain do_action hooks invoking this wherever needed so that debugging becomes easier if someone sends us the log file.

comments welcome @arnaudbroes @wpsmort @EkoJR

michaeltorbert commented 5 years ago

Personally my preference would be to log to a file, with the understanding that it may not work on some crappy hosts. This way we can use the WP Filesystem API. I'm certainly open to discussion on it though.

Also, I'd change the radios to checkboxes so that multiple ones can be selected (obviously "all" would deselect the reset).

arnaudbroes commented 5 years ago

I'm fine with both ideas although a text file would have my preference as well since that's easily accessible via SFTP.

michaeltorbert commented 5 years ago

@arnaudbroes That was my thinking. log files are easier to work with, send, etc. They can also be easily deleted instead of filling up the db. I worry about runaway log entries killing a db. On that topic, our logging should be off by default.

arnaudbroes commented 5 years ago

I wasn't even thinking about log entries running wild. Yes, we should definitely log to a file instead of the database. I only see this causing unwanted issues otherwise.

wpsmort commented 5 years ago

I also agree that this should be logging to a file rather than the database and that the radios be changed to checkboxes and remove the All option.

What file and file location will this log to? In our help for the Log events option it says the all-in-one-seo-pack.log file in its plugin directory. Should we change the location to /wp-content/ so it's in the same location as the WordPress debug log?

michaeltorbert commented 5 years ago

I'd definitely change it to wp-content. We may also want to have a constant AIOSEOP_DEBUG_LOG which can be set to a custom path in wp-config.

contactashish13 commented 5 years ago

I think we take files to be easy to work with because of our familiarity and not any technical reasons. Writing to files is costly as compared to databases. Clearing files is costly. Any file operation is costly. Using g transients you reduce all that effort so I'm not cconvinced. Is there a strong technical reason I'm missing?

On Feb 15, 2019, 23:00, at 23:00, Michael Torbert notifications@github.com wrote:

I'd definitely change it to wp-content. We may also want to have a constant AIOSEOP_DEBUG_LOG which can be set to a custom path in wp-config.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/semperfiwebdesign/all-in-one-seo-pack/issues/660#issuecomment-464132861

michaeltorbert commented 5 years ago

I mentioned technical reasons above. They're also easier for users to use, send, etc. There's definitely a resource overhead cost of writing to files vs database, but presumably logging wouldn't be running all the time on a production site, only for development, staging, etc.

contactashish13 commented 5 years ago

Not sure easier to work with qualifies as a technical reason :) That's more of user friendliness than a technical reason for preference or efficiency.

On Feb 16, 2019, 01:05, at 01:05, Michael Torbert notifications@github.com wrote:

I mentioned technical reasons above. They're also easier for users to use, send, etc. There's definitely a resource overhead cost of writing to files vs database, but presumably logging wouldn't be running all the time on a production site, only for development, staging, etc.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/semperfiwebdesign/all-in-one-seo-pack/issues/660#issuecomment-464172626

michaeltorbert commented 5 years ago

I worry about runaway log entries killing a db.

I'd rather have excessive log entries make a 10GB file than a 10GB db.

contactashish13 commented 5 years ago

Not gonna happen with the constraints I mentioned. Log length and log expiry. Maybe you can check the code to see what's happening and how it works to reassure yourself it's not just random forget and insert into the database.

On Feb 16, 2019, 01:10, at 01:10, Michael Torbert notifications@github.com wrote:

I worry about runaway log entries killing a db.

I'd rather have excessive log entries make a 10GB file than a 10GB db.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/semperfiwebdesign/all-in-one-seo-pack/issues/660#issuecomment-464174182

michaeltorbert commented 5 years ago

Out of curiosity, why did you choose to store the logs as transients rather than something proprietary, and why do you recommend they be stored for only 10 minutes?

Interestingly, WooCommerce does both file and database: https://github.com/woocommerce/woocommerce/blob/master/includes/log-handlers/class-wc-log-handler-db.php https://github.com/woocommerce/woocommerce/blob/master/includes/log-handlers/class-wc-log-handler-file.php

contactashish13 commented 5 years ago

I chose transients because the cleanup pf those is handled by WP automatically. Also because I don't want them cached as regular options are auto cached.

Caching for 5 minutes and using only the last 100 logs are just a random numbers and can be increased or decreased using the filters.

On Feb 16, 2019, 08:58, at 08:58, Michael Torbert notifications@github.com wrote:

Out of curiosity, why did you choose to store the logs as transients rather than something proprietary, and why do you recommend they be stored for only 10 minutes?

Interestingly, WooCommerce does both file and database: https://github.com/woocommerce/woocommerce/blob/master/includes/log-handlers/class-wc-log-handler-db.php

https://github.com/woocommerce/woocommerce/blob/master/includes/log-handlers/class-wc-log-handler-file.php

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/semperfiwebdesign/all-in-one-seo-pack/issues/660#issuecomment-464279506

EkoJR commented 5 years ago

Well, 5 minutes doesn't leave much time, and a lot of errors are caught after the fact.

I also think it would be best to make it an OOP class. Which seems to be what WooCommerce did.

I'm also concerned if this creates multiple transients instead of 1. Each transient save and load is an SQL query, which results in a higher time and memory. With the Sitemap Optimization to attachments, I kept it as a static variable to load 1 time, and a hook on shutdown to save the data.

michaeltorbert commented 5 years ago

@EkoJR I think he just meant that he randomly put down a number to be figured out later, not that he specifically wanted that amount of time. ;)

wpsmort commented 4 years ago

Replaced by #3003

awesomemotive / all-in-one-seo-pack

All logging should be our own file #660

Out of curiosity, why did you choose to store the logs as transients rather than something proprietary, and why do you recommend they be stored for only 10 minutes?