Improvement of logging for masakari-controller

ntt-sic / masakari

[UNMAINTAINED]

https://wiki.openstack.org/wiki/Masakari

Apache License 2.0

41 stars 25 forks source link

Improvement of logging for masakari-controller #43

Closed rkmrHonjo closed 8 years ago

rkmrHonjo commented 8 years ago

This pull-request contains following changes.

Use oslo.log for logging.
Add configuration values for logging.
Improve log messages.

And, remove unnecessary line from masakari-controller/db/api.py

muroi commented 8 years ago

Thanks the patch! I'm really surprised at all function has following lines and the method name is hard coded. If you'd like to output log in begging and ending of methods, we should implement decorator for outputting log and use the decorator.

msg = BEGIN function-name: with arguments
LOG.debug(msg)

msg = END function-name
LOG.debug(msg)

rkmrHonjo commented 8 years ago

Hi Muroi,

Thank you for your comment. I fix my code.

rkmrHonjo commented 8 years ago

Hi @muroi ,

I fixed my code in accordance with your comments. (If I should squash two commits, please point out.)

P.S. I didn't remove logs from _get_vm_param(). Your comment is basically right. But _get_vm_param() will retry do_instance_show() if it is failed. It doesn't just call rc_util_api.do_instance_show().

muroi commented 8 years ago

Thanks for updating. Overall the codes looks good. Great job!

I still have some questions. please check the inline comments.

rkmrHonjo commented 8 years ago

Thank you for your re-comments. I check those & fix my code.

muroi commented 8 years ago

Oops, I forgot commenting an important stuff. The decorator method needs wraps[1] decorator.

http://docs.python.jp/3.5/library/functools.html#functools.wraps

rkmrHonjo commented 8 years ago

Oh...Thank you for pointing out.

rkmrHonjo commented 8 years ago

Hi @muroi ,

Sorry for taking long to fix my code. Please check new commit.

rkmrHonjo commented 8 years ago

Hi @muroi ,

I rethought about my code for logging thread name. Then I understood that my most recent patch is not good.

My fix plan is following:

Sorry, I noticed that thread name is already contained in log record object!
If we specify "%(threadName)s" in logging format string, we'll be able to output thread name to log.
So I'll remove the class that is inherited RequextContext.
It is unnecessary.
And I'll add "%(threadName)s" to the logging format string configuration.

(Of cource, I haven't forgotten to name thread uniquely.)

Regards.

rkmrHonjo commented 8 years ago

Hi @muroi ,

I pushed a new patch. Fix points are written in my previous comment.

Regards.

rkmrHonjo commented 8 years ago

Oops, some useless/bad codes might remain... I'm terribly sorry, please wait until the end of my re-checking.

rkmrHonjo commented 8 years ago

Dear @muroi ,

Sorry to keep you wainting. Re-checking by myself was done. Please review my codes.

muroi commented 8 years ago

thank updating, @rkmrHonjo . LGTM, waiting others comments.

sampathP commented 8 years ago

@rkmrHonjo Thanks for fix. @muroi Thanks for review. LGTM, good for merge.