Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1010 closed task (complete)

Configuration of the Python logging API

Reported by: stephen Owned by: jelte
Priority: medium Milestone: Sprint-20110628
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Python programs need to extract their logging information from the configuration database and use it to configure the Python logging.

Subtickets

Change History (9)

comment:1 Changed 8 years ago by stephen

  • Component changed from Unclassified to logging

comment:2 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:3 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

Python modules now have the same option to let logging config be handled automatically as c++ modules do when they create a ModuleCCSession.

When handled automatically, the new module-level function isc.log.log_config_update is called, which in turn calls the same log config handler as the c++ version does (so apart from converting to c++, there is no python-specific parsing of the configuration data).

Note that you can configure it, but right now no module actually uses it yet :) (i'll take up a ticket next to handle that)

notable other changes:

  • renamed the c++ my_logconfig_handler to default_logconfig_handler and made it public
  • added (optional) config-update callbacks for remote modules in python (c++ already had this)
  • made the log/init.py a tad smarter (it found the wrong dir on my system)
  • added a function to convert from python basic types (which new_config and json-read data are in) to ElementPtr? (currently only in ccsession.cc, if we find use for this elsewhere we should put it in a wrapper-util somewhere)

comment:4 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:5 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

I noticed few things:

  • The distcheck fails for me, it can't find the module. For some reason it didn't fail before. Does it run for you?
  • With the default_logconfig_handler, the n parameter doesn't seem too descriptive. Wouldn't there be a better name?
  • The InternalError was kind of expected to only propagate up trough the stack when there already is a python exception registered. The way you first throw InternalError and after catching it, you make it inconsistent. Could it possibly be reversed?
  • Wouldn't the whole conversion be easier like converting to JSON string and passing the string to Element::fromJSON? I think the code would be shorter with better code reuse. But it might look less elegant on the other hand, so I'm not sure which one is better.

Thanks

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

I noticed few things:

  • The distcheck fails for me, it can't find the module. For some reason it didn't fail before. Does it run for you?

I've been thinking about this, and I have a new way to find the .libs dir; the pythonpath should already contain the correct base directory (both build and srcdir, in the case of tests), so it should work if we walk through the python path and see if isc/log/.libs is in there, and add it if it is. Code in commit a4f1f8de765810aecff1194c74a108682e3de28e

  • With the default_logconfig_handler, the n parameter doesn't seem too descriptive. Wouldn't there be a better name?

Yeah it should just be module_name

  • The InternalError was kind of expected to only propagate up trough the stack when there already is a python exception registered. The way you first throw InternalError and after catching it, you make it inconsistent. Could it possibly be reversed?

Ok, done. Added a comment to the definition of InternalError? to describe its intended use.

Moot point now, see below. (did leave in the added comment)

  • Wouldn't the whole conversion be easier like converting to JSON string and passing the string to Element::fromJSON? I think the code would be shorter with better code reuse. But it might look less elegant on the other hand, so I'm not sure which one is better.

I had thought about that, and I was not sure either. It would certainly save a bit of code, at the cost of two json conversions. Originally I chose this method because requiring JSON strings would make the interface slightly muddier. But now that I think about it, the JSON form is actually more flexible, and the actual conversion would be more or less hidden by the handler in the python code anyway.

So yeah, let's do it through json. Currently simple json.dumps() calls suffice, so it's not that much of a change from the callers side.

comment:7 in reply to: ↑ 6 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

The searching of path works for me now :-).

Shouldn't the commented thing be removed? Or you want to test that this is rejected?

-        isc.log.log_config_update({}, log_spec)
+        #isc.log.log_config_update("{", log_spec)
+        isc.log.log_config_update("{}", log_spec)

I had thought about that, and I was not sure either. It would certainly save a bit of code, at the cost of two json conversions. Originally I chose this method because requiring JSON strings would make the interface slightly muddier. But now that I think about it, the JSON form is actually more flexible, and the actual conversion would be more or less hidden by the handler in the python code anyway.

So yeah, let's do it through json. Currently simple json.dumps() calls suffice, so it's not that much of a change from the callers side.

Well, I originally thought that the json.dumps() would be called from within the wrappers (it is possible to call python functions from C++ somehow), so the user wouldn't need to. But as this is only in limited place, this is probably OK too.

So, if the above one is to be deleted, I think it can be merged right away.

comment:8 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Replying to vorner:

Hello

The searching of path works for me now :-).

yay

Shouldn't the commented thing be removed? Or you want to test that this is rejected?

oh, no that should have been removed already, done.

So, if the above one is to be deleted, I think it can be merged right away.

Cool, thanks.

Merged, closing ticket.

comment:9 Changed 8 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5
Note: See TracTickets for help on using tickets.