Hi Ginnie,
On 03/24/10 09:55 AM, Virginia Wray wrote:
> Hi Keith -
>
> Thanks for the review. See my responses below.
> ginnie
>
>
>
> On 03/23/10 12:41, Keith Mitchell wrote:
>> Hi Ginnie,
>>
>> I think the document is looking fantastic. The detail has helped me
>> understand the logging module.
>>
>> My remaining concern is with the API for init_logger and add_handler.
>>
>> I had to read ahead all the way down to use cases to fully understand
>> the "handler" argument. It appears that a string or constant will be
>> mapped to a handler type by the init_logger call, and instantiated
>> using args also passed into init_logger. This explains Sarah's desire
>> to have arbitrary arguments passed in, but even with that, it sounds
>> overly restrictive. Why not instantiate the handler, and then pass in
>> the handler reference to init_logger?
>>
>> e.g.
>> handler = logging.FileHandler(...)
>> logging.init_logger(app_name, handler, logging.INFO)
>> In this way, any logging handler, even handlers which are not
>> directly defined in the logging module (e.g., an application specific
>> subclass) can be passed in, and the init_logger function doesn't have
>> to worry about understanding those things.
> The application won't be calling anything with "logging.". The logging
> interaction is going to be managed directly by the logging module.
I think I may have cornered myself in the language issue. When I stated
"logging.init_logger()", I should have said
"InstallLogger.init_logger()". I was a bit ambiguous with my usage of
the term "logging module" in that response, sometimes referring to
InstallLogger and sometimes referring to Python's logging module - sorry
about that. I'll be clearer this time around.
> If a handler isn't defined in the logging module, it couldn't be used
> for logging.
What is the benefit of imposing this restriction? Part of the
flexibility of Python and duck-typing is that one can create subclasses
or even duck-typed pseudo-classes - the benefits of such flexibility
including making it easier to debug or test odd scenarios by creating
one-off modifications, for example.
> The init_logger function gives the application an entrance to the
> logging module. It doesn't have direct access to it. From my
> understanding
> of how this would be implemented, the application would make a request
> to the engine for logging, which the engine passes on to the logging
> module.
This may be part of my confusion. The document states that init_logger
is a function of InstallLogger, but the implication here is that
init_logger is actually a function of the execution engine. Can you
clarify? I don't see an init_logger call in the UML diagram either.
> I don't have a problem with the application calling for a handler
> first, before initializing the logger, but there is overhead either
> way. The application would do the following:
> handler = add_handler( file)
> init_logger(app_name, handler, logging.INFO)
I agree there's overhead either way. The difference is, if you create a
handler first and then pass it in, there's a *much* higher level of
flexibility for what the application can pass in as a handler.
>
> However, my plan was to allow an application to initialze w/o
> requiring a handler object, and have a default set up if nothing is
> provided (this would be to a file handler with a agreed upon location
> for the file).
>
> init_logger(app_name)
>
> That would be about as simple as it could get.
Excellent. I completely agree with that sort of default.
>>
>> With intelligent defaults for keyword arguments for handlers, this
>> can even be reduced further to something as simple as:
>>
>> logging.init_logger(app_name, logging.Filehandler()).
>>
>> Similarly, with init_handler - why not call it "add_handler"?
> I'm fine with changing the name. That's not a problem.
>> Let the consumer pass in a constructed handler, and have the
>> add_handler code handle any additional registration, such as setting
>> up the proper formatting, logging level, and the like. Done properly,
>> this would be far more flexible, yet maintain simplicity for the
>> default cases. If defaults for kwargs is not sufficient, provide a
>> 'generator' or 'factory' function for those specific classes. For
>> example, if the HTTP defaults require logging module context, then a
>> function such as:
>> logging.generate_http_handler(host, url, ...)
>>
>> If the argument is complexity - I argue that it's more complex to ask
>> the consumers to construct the **kwargs object, instead of the more
>> natural method of simply instantiating the desired handler instance.
> The problem with this approach, if I'm understanding you correctly,
> is that the consumers aren't directly calling logging module
> interfaces. The idea is to use the "wrapper functions", which pass the
> info to the engine, and then, have the engine interact directly with
> the logging module. I don't think that what you are proposing really
> shifts the complexity; it just moves the responsibility from the
> logger to the application. The generate_http_handler function would
> accomplish the same thing as init_handler(http, hostname, url) or
> add_handler(.....), if I change the name. One way or another the
> information has to be propagated to the logger.
There is a slight difference between the concept of "init_handler" and
"add_handler", regardless of what exactly each of those functions might
be named, at least as I understand it.
In either case, the caller has to understand ALL *required* parameters
of the function, as well as any optional parameters that it wishes to
modify. That complexity is their either way - as you say, one way or
another, the application has to propagate that information to the
logger. And the parameters are the same either way, as well:
instantiation of a FileHandler requires the application to know it wants
a FileHandler, and to optionally pass in a filename (getting a default
of "/tmp/install_log" if no filename is passed in, for example). Whether
that FileHandler is created via FileHandler("/tmp/install_log") or
init_handler(FileHandler, "/tmp/install_log"), the same level of
complexity and flexibility is available *for the handlers that
init_handler is designed to understand*. If the required parameters to
FileHandler need to change, then the application has to be updated
regardless of method; if an optional parameter is added, and the
application doesn't want it, it doesn't have to be updated in either
case, and if it wants to make use of (the non-default), the app has to
be updated.
With the add_handler interface of creating a handler and passing it in,
the application has the option, but is not required, to modify the
handler however it chooses prior to registering it with the execution
engine or InstallLogger (the engine/InstallLogger can still ensure that
the passed in handler is reasonable, much like it would have to ensure
that the arguments passed in via init_handler would be reasonable). This
includes passing in a sub-class, modifying rarely used fields of the
handler, etc. Additionally, the application, since it is creating the
handler, will more readily be able to understand the parameters passing
in. What I'm saying is, with "init_handler(handler_type, **kwargs)", the
app has to dig through an additional layer to understand how to pass in
kwargs; with "add_handler(handler_instance)", whatever mechanism is
used to create the handler_instance is directly invoked. Even if that
mechanism is a wrapper provided by InstallLogger of the execution
engine, there's *immense* flexibility added by breaking it into two
steps (creation of the handler, either via handler.__init__() or a
'factory' method, and registering the handler with the
engine/InstallLogger).
>
> If I'm not understanding you, please let me know.
>>
>> Use cases:
>>
>> Pseudo code for log_messages: I know it's just sample code, but, if
>> log levels mapped to integers in the same way Python's logging module
>> does, then it's a much simpler task to do something like:
>>
>> def log_message(...):
>> for handler in self.handlers:
>> handler.log(self.logging_level, message, ...)
> The logging levels that exist in the Python logging module will be
> used. If we need to sub-class additional ones (which we will do for
> progress reporting), then we'll add other log levels.
> The way the Python logging module works is when you set up a handle
> with a default logging level, then anytime a message with that logging
> level is generated, it will be published to that
> handler. So, when the application generates log_message( level,
> message), it will go to the correct handlers that have been set up to
> receive messages at that level. I haven't looked specifically
> at how the Python logging module does this, but I wouldn't be
> surprised if it is implemented similarly to what you've done.
It is; my point here was just a minor nit, in that the long
"if/elif/else" block can and should be trivialized by mimicking the
Python logging module's concept of log levels mapping to integers. Being
blind to the "Level" parameter of "log_message" (and other functions
accepting a level parameter), like the Python logging.log(level, ...)
function, is the goal.
>
> Let me know if you have any other questions.
>
> Thanks again for your comments.
>
> ginnie
>>
>> Thanks,
>> Keith
>>
>> On 03/12/10 04:39 PM, Virginia Wray wrote:
>>> Hi -
>>>
>>> I've posted the second draft of the Logging design doc at:
>>>
>>> http://hub.opensolaris.org/bin/view/Project+caiman/Logging
>>>
>>> Select Loggingv1.2.pdf
>>>
>>> Please provide feedback by March 24, 2010.
>>>
>>> thanks,
>>>
>
>
