[Kimchi-devel] [PATCH] [Wok 4/7] Log AsyncTask success or failure

[Aline Manera]

On 08/31/2016 06:06 PM, Lucio Correia wrote:
> * Add plugin and log_id attributes to AsyncTask
> * Create _log() function to log AsyncTask exit status
> * Create auxiliary method to save log entry ID for a task
> * Handle WokException separately in order to add it in
> translatable form to user log
>
> Signed-off-by: Lucio Correia <luciojhc at linux.vnet.ibm.com>
> ---
>   src/wok/asynctask.py | 46 ++++++++++++++++++++++++++++++++++++++++------
>   src/wok/i18n.py      |  2 ++
>   2 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/src/wok/asynctask.py b/src/wok/asynctask.py
> index 021b76c..633c96f 100644
> --- a/src/wok/asynctask.py
> +++ b/src/wok/asynctask.py
> @@ -25,10 +25,13 @@ import time
>   import traceback
>   import uuid
>
> -
> -from wok.exception import InvalidOperation, OperationFailed
> +from wok.exception import InvalidOperation, OperationFailed, WokException
> +from wok.reqlogger import RequestRecord, ASYNCTASK_REQUEST_METHOD
> +from wok.utils import get_plugin_from_uri
>
>
> +MSG_FAILED = 'WOKASYNC0002L'
> +MSG_SUCCESS = 'WOKASYNC0001L'
>   tasks_queue = {}
>
>
> @@ -43,27 +46,54 @@ def clean_old_tasks():
>                   task.remove()
>

> +def save_request_log_id(log_id, task_id):
> +    tasks_queue[task_id].log_id = log_id
> +
> +

You can use "tasks_queue[task_id].log_id = log_id " directly in the code.
You are replacing one line code by one function. I don't see much gain 
on it specially while reviewing the code where you can grab the function 
to simply to understand it.

>   class AsyncTask(object):
>       def __init__(self, target_uri, fn, opaque=None, kill_cb=None):
> +        # task info
>           self.id = str(uuid.uuid1())
>           self.target_uri = target_uri
>           self.fn = fn
>           self.kill_cb = kill_cb
> +        self.log_id = None

> +        self.plugin = get_plugin_from_uri(target_uri)

The target_uri may be anything!
Usually on Kimchi, we have a pattern to be the the plugin base URI + 
resource URI but it may be not true to every plugin.
I suggest to get that info from cherrypy

> +        self.timestamp = time.time()
> +
> +        # task context
>           self.status = 'running'
>           self.message = 'The request is being processing.'
> -        self.timestamp = time.time()
>           self._cp_request = cherrypy.serving.request
>           self.thread = threading.Thread(target=self._run_helper,
>                                          args=(opaque, self._status_cb))
>           self.thread.setDaemon(True)
>           self.thread.start()
> +
>           # let's prevent memory leak in tasks_queue
>           clean_old_tasks()
>           tasks_queue[self.id] = self
>
> -    def _status_cb(self, message, success=None):

> +    def _log(self, code, status, exception=None):
> +        RequestRecord(
> +            {'code': code, 'params': {'target_uri': self.target_uri}},
> +            exception,
> +            self.log_id,
> +            app=self.plugin,
> +            req=ASYNCTASK_REQUEST_METHOD,
> +            status=status,
> +            user='',
> +            ip='',
> +        ).log()
> +

Isn't this function the same you created on Patch 1? Can't you reuse the 
other one?

> +    def _status_cb(self, message, success=None, exception=None):
>           if success is not None:
> -            self.status = 'finished' if success else 'failed'
> +            if success:
> +                self._log(MSG_SUCCESS, 200)
> +                self.status = 'finished'
> +            else:
> +                self._log(MSG_FAILED, 400, exception)
> +                self.status = 'failed'
>
>           if message.strip():
>               self.message = message
> @@ -72,10 +102,14 @@ class AsyncTask(object):
>           cherrypy.serving.request = self._cp_request
>           try:
>               self.fn(cb, opaque)
> +        except WokException, e:
> +            cherrypy.log.error_log.error("Error in async_task %s " % self.id)
> +            cherrypy.log.error_log.error(traceback.format_exc())
> +            cb(e.message, success=False, exception=e)
>           except Exception, e:
>               cherrypy.log.error_log.error("Error in async_task %s " % self.id)
>               cherrypy.log.error_log.error(traceback.format_exc())
> -            cb(e.message, False)
> +            cb(e.message, success=False)
>
>       def remove(self):
>           try:
> diff --git a/src/wok/i18n.py b/src/wok/i18n.py
> index 7ba7c24..4893be7 100644
> --- a/src/wok/i18n.py
> +++ b/src/wok/i18n.py
> @@ -59,6 +59,8 @@ messages = {
>       "WOKPROXY0001E": _("Unable to (re)start system's nginx.service. Details: '%(error)s'"),
>
>       # These messages (ending with L) are for user log purposes
> +    "WOKASYNC0001L": _("Successfully completed task '%(target_uri)s'"),
> +    "WOKASYNC0002L": _("Failed to complete task '%(target_uri)s'"),
>       "WOKCOL0001L": _("Request made on collection"),
>       "WOKRES0001L": _("Request made on resource"),
>       "WOKROOT0001L": _("User '%(username)s' login"),