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

ENH: add logging proprety machine #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tacaswell
Copy link
Contributor

Add a sub-class of PropertyMachine which takes an optional logging in __init__ which is used to log all state changes at the INFO level.

If this of interest to you to include in main line I will add tests + docs + make level configurable.

tacaswell added 3 commits July 2, 2015 00:28
Add a sub-class of PropertyMachine which takes an optional logging
in `__init__` which is used to log all state changes at the INFO level.
@beregond
Copy link
Owner

beregond commented Jul 6, 2015

Logging is nice feature, so 👍 for the idea, but why would somebody create such property explicitly in business logic layer if he/she would like to have logging?

My suggestion is to forget about LoggingPropertyMachine and add standard logging to core PropertyMachine according to guideline here: https://docs.python.org/2/howto/logging.html#configuring-logging-for-a-library

This way anybody could enable/disable logging if needed without thinking in advance just by configuring logging handlers and without any change in business logic objects.

@tacaswell
Copy link
Contributor Author

The logic for having a sub-class that implements the logging framework is to avoid having to pay the (minor) performance cost if you do not want the logging.

@beregond
Copy link
Owner

Well @tacaswell I was thinking about this a bit, performance reason is not enough for me. Logging is important but I don't want to change implementation in order to enable/disable logs from my app (which is using this library) especially that this is not 'the way' suggested in the docs.

@beregond
Copy link
Owner

I'm open for discussion, but this solution is too dirty.

@tacaswell
Copy link
Contributor Author

https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library (docs for current python)

That mostly talks about not registering default handlers on to your library and how to avoid having your library annoy users if they are not using logging. I don't see anything that is for or against this usage.

Part of my thinking here is that users are going to integrate ssm into another library which is using logging and may have multiple state machines. As implemented it allows the client library to inject into ssm where to log messages to on a pre-machine basis. If ssm had it's own logging namespace the messages from all the user machines would go through that routing. It is possible to re-direct / filter log messages by attaching many handlers to the ssm logger and then rejecting all of the messages except for the instance you are interested in, but just routing the log messages to the right place to begin with seem cleaner.

@beregond
Copy link
Owner

https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library (docs for current python)

It's the same anyway ;)

That mostly talks about not registering default handlers on to your library and how to avoid having your library annoy users if they are not using logging. I don't see anything that is for or against this usage.

Yes, but the approach is different. I could stay with 'this is how its done', but more arguments for this at the end.

Part of my thinking here is that users are going to integrate ssm into another library which is using logging and may have multiple state machines.

Assuming we will do logging always as suggested in docs (logger = logging.getLogger(__name__)) if you have different libs it's enough to disable handlers for super_state_machine (since all other loggers will be children of this logger, as described in docs). The same with other libs, you can simply disable other_lib handlers, as long as logging there is done well.

If ssm had it's own logging namespace the messages from all the user machines would go through that routing.

Isn't that the point of logging?

As implemented it allows the client library to inject into ssm where to log messages to on a pre-machine basis. (...) It is possible to re-direct / filter log messages by attaching many handlers to the ssm logger and then rejecting all of the messages except for the instance you are interested in, but just routing the log messages to the right place to begin with seem cleaner.

Why this isn't more clear more below. About logging per-instance: I've never seen anything like that. Furthermore I find it totally useless:

  • First, when you have any state machine - lets say we have class named Car - you don't decide upfront 'I want log this instance, but don't want this one' because logs are usually used for post-mortem analysis 'what went wrong' - so you just log everything.
  • Second, if you want to filter out certain classes - you should make lib to log class name as well. With this you can find interesting logs, by using papertrail or standard file log, or whatever you need. You could also maybe overwrite loggers to put class name from traceback somehow, or anything, don't know for sure what would be best approach.

At last, few words why this solution isn't clear: you are repeating yourself. Simple state machines are... simple! It's easy to make two implementations without repeating logic here, because you can use inheritance like in your PR. Now think how would you make two versions of each object - one with logging, and second without. What if ssm will grow (and I intend it to ;) )? What if each function will as big as this init -> https://github.com/kivy/kivy/blob/master/kivy/input/providers/linuxwacom.py#L138 ? This would be extremely hard to maintain such code. KISS and DRY rules apply here. :)

That's why I think we should stick with standard option where logging is always given, but in case of lib disabled by default. And with this some help would be definitely appreciated.

P.S. You could take a look on kivy project and the way they are using logging - it's definitely worth seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants