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

Add connector for Errors #11

Closed
4 tasks
frankiejarrett opened this issue Nov 27, 2013 · 8 comments
Closed
4 tasks

Add connector for Errors #11

frankiejarrett opened this issue Nov 27, 2013 · 8 comments

Comments

@frankiejarrett
Copy link
Contributor

Actions that result in errors should be added as a core part of the Stream plugin.

@shadyvb Should we save errors to the wp_stream table with the type column as error instead of stream? Then the action record that caused the error would become the parent and the error type could be saved as record meta.

Error types:

  • Notice
  • Warning
  • Fatal
  • WP Error
@ghost ghost assigned shadyvb Dec 5, 2013
@shadyvb
Copy link
Contributor

shadyvb commented Dec 5, 2013

@fjarrett What actions will be tracked ? all wp actions, in which we hook to everything, which would not be performance friendly at all, or those that we're already tracking, which is a not reliable list of actions, since we sometimes have to workaround WP-lack of hooks in specific flows.

My opinion on this, that we should register our own error handler so we handle every single error that happens on the site. That's is something that LOTS would definitly be interested in, actually this might be a major selling point!

See: http://php.net/manual/en/function.set-error-handler.php , http://php.net/manual/en/function.set-exception-handler.php

@frankiejarrett
Copy link
Contributor Author

@westonruter would love to get your thoughts on this.

@westonruter
Copy link
Contributor

The most important thing about capturing errors is to have some context for when the error occurred. Even if there is not a preceding action which was captured by Stream which could be linked to as the parent, what if we logged all actions that got fired, for example via add_action( 'all' ) and keep track of the most recently-invoked action/filter, and then attach this action/filter to the metadata of the stream entry?

One big concern I have about having Stream capture errors is that there could be multiple errors that happen with each page load, especially with notices, and this will balloon the DB table faster than the speed of light. So if such errors were tracked, then it seems there must be some way to aggregate entries:

The first time that an error/exception is caught, it should compute a hash to fingerprint the error. Then the errexception should be looked up in the WP_Object_Cache to see if it has been previously seen. If it is not present in the cache, then it should insert a new Stream entry for the errexception and store a object-cache entry for the future. The next time the error happens, we can check to see if there is an entry in the cache for it, and then increment a counter on the cache entry without touching the DB. Then maybe once an hour the counters could be saved to the DB. Just thinking about performance here. I think maybe an external persistent Object Cache might need to be a requirement.

I do no think there is a way to capture WP_Error. See: http://danielbachhuber.com/2013/05/08/status-156/

@frankiejarrett
Copy link
Contributor Author

@westonruter Excellent thoughts. Thanks.

Regarding hashing errexceptions, is there any optimized way we could leverage the Transients API for this? It doesn't necessarily mean accessing the database every time there is an error if there is a memcached plugin available.

I don't think we should be logging any front-end errors at all. Since the core purpose of Stream is strictly to monitor changes happening to the site, error tracking needs to be in line with that and only care about errors occurring in the WP Admin area. And really, I think we only care about when an error occurs as a result of a user action.

So if performance becomes a big concern, it's likely that it's a very large site with a lot of admin activity resulting in potentially a lot of errors. In which case, that type of large site would probably be using memcached, so the Transients API would seamlessly optimize for it.

@shadyvb
Copy link
Contributor

shadyvb commented Dec 7, 2013

@westonruter re: checking relative action, there is the global wp_current_filter variable and the 'wp_actions' array used by did_action, instead of hooking into all.

I believe we're now talking about two different cases, WP_Error instances, and the plain old PHP errors / exceptions., which were my main concern in my first comment.

I do not know of a method to override WP_Error to attach our required functionality, either at construct/destruct or even add. So that's a blank area for me unless we can push for an update to WP core that delivers our wanted behavior, and i'm not a core contributor yet so i don't know their flexibility on this.

So my proposal for handling plain ol' php errors still stands.

@fjarrett About handling only admin actions, i'm not sure all users would agree here, if i'm a site admin, i'd probably want to log all errors related to my code, which would be in admin back-end and on the front-end as well. So i hope we can make this a user choice, and not fix it up for them.

@frankiejarrett
Copy link
Contributor Author

@shadyvb I just want to be careful that a feature, like error logging, doesn't go beyond the core purpose of the actual plugin itself, which is to simply track logged-in user activity.

There are already plugins available that will help you monitor the PHP error log, and if want to be really hardcore about monitoring your server, there are tons of 3rd party services available.

As it relates to Stream (right now), it seems the most beneficial thing to know is whether or not a user action resulted in an error, not to build a comprehensive error tracking plugin. Does that make sense?

@shadyvb
Copy link
Contributor

shadyvb commented Dec 8, 2013

@fjarrett Thanks for clearing this out, i think we're on the same page now.

However, i don't think we have a strategy yet to tackle the issue. as @westonruter said there is currently no way of tracking WP_Error usage globally.

@frankiejarrett
Copy link
Contributor Author

I'm closing this for now.

frankiejarrett pushed a commit that referenced this issue Aug 22, 2014
frankiejarrett pushed a commit that referenced this issue Aug 22, 2014
frankiejarrett pushed a commit that referenced this issue Aug 22, 2014
jonathanbardo added a commit that referenced this issue Aug 22, 2014
Issue-11 Views This close #11
frankiejarrett pushed a commit that referenced this issue Aug 22, 2014
…view Param as soon as possible, to avoid problems. #12 #11
frankiejarrett added a commit that referenced this issue Aug 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants