Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#761 closed enhancement (complete)

Conversion of xfrin to use the new logging interface

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: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is dependent on ticket #756, and involves converting the files in src/bin/xfrin to use the new logging interface.

Subtickets

Change History (8)

comment:1 Changed 9 years ago by stephen

  • Defect Severity set to N/A
  • Milestone changed from Year 3 Task Backlog to Sprint-20110628
  • Sub-Project set to DNS

comment:2 Changed 9 years ago by jelte

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

comment:3 follow-up: Changed 9 years ago by stephen

Overall, the logging looks as I expect. Just a few comments:

xfrindef.mes
Depending on discussions this afternoon, we might want to get rid of the $PREFIX.

Message text should start with a lower-case character.

I think that some message descriptions need to be expanded. For example, the description for BAD_ZONE_CLASS paraphrases the message text and does not explain why/where the message originated.

I know I started the convention of calling message files xxxdef.<whatever>. However, I'm now thinking that a convention of xxx_messages.<whatever> is more descriptive.

xfrin.py.in
The root logger name is initialized as "b10-xfrin" as is the name of the logger used to log messages in the xfrin.py module. Although this is perfectly valid, it does mean that any logger settings aimed at this module also affect the settings of the loggers used by the C++ modules (unless they have their own settings). Using a different name for xfrin.py logger (e.g. "xfrin") gives finer control: "b10-xfrin" settings affect everything that doesn't have its own settings, "b10-xfrin.xfrin" affects just this Python module.

I take it that the DONOTCALLMEEVER is just a place holder?

Jeremy has commented about multiple errors being attached to one messages. The code raises exceptions which are caught and the reason attached to them output (although I notice that occasionally an error is logged just prior to raising the exception). It might be neater to combine the raising of the exception and the logging of the message by raising the exception, passing the message code (and parameters) as constructor arguments, and having the constructor log the message. If the exceptions raised here were derived from a common base class, the "except" clause in main() could distinguish between exceptions where a message has already been logged, and one where output needs to be produced.

comment:4 in reply to: ↑ 3 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

Overall, the logging looks as I expect. Just a few comments:

xfrindef.mes
Depending on discussions this afternoon, we might want to get rid of the $PREFIX.

Message text should start with a lower-case character.

I know I started the convention of calling message files xxxdef.<whatever>. However, I'm now thinking that a convention of xxx_messages.<whatever> is more descriptive.

xfrin.py.in
The root logger name is initialized as "b10-xfrin" as is the name of the logger used to log messages in the xfrin.py module. Although this is perfectly valid, it does mean that any logger settings aimed at this module also affect the settings of the loggers used by the C++ modules (unless they have their own settings). Using a different name for xfrin.py logger (e.g. "xfrin") gives finer control: "b10-xfrin" settings affect everything that doesn't have its own settings, "b10-xfrin.xfrin" affects just this Python module.

Handled all these points. Also took example from 1040, and put the messages in alphabetical order, and removed some redundant whitespace. And expanded the one abbreviation.

I take it that the DONOTCALLMEEVER is just a place holder?

Actually it should already have been removed, did so now.

I think that some message descriptions need to be expanded. For example, the description for BAD_ZONE_CLASS paraphrases the message text and does not explain why/where the message originated.

Hmm, I did a few, though haven't changed them that much. This may also be related to my comment below;

Jeremy has commented about multiple errors being attached to one messages. The code raises exceptions which are caught and the reason attached to them output (although I notice that occasionally an error is logged just prior to raising the exception). It might be neater to combine the raising of the exception and the logging of the message by raising the exception, passing the message code (and parameters) as constructor arguments, and having the constructor log the message. If the exceptions raised here were derived from a common base class, the "except" clause in main() could distinguish between exceptions where a message has already been logged, and one where output needs to be produced.

Since we decided that we'd first do the conversion, and then improve the result, I was going to defer this to the followup improvement ticket. But I do have some thoughts about this; I do not think that is the right approach. For some of the current messages, it is hard to tell the specific problem, because it is logged too deep; say you have a method 'parseFoo(str): <dostuff> return FooObject?() or raise BadFOO(str)' which whould parse a string containing a Foo value, and it finds the string in error, that function is not the one that should log the error, since it does not know the context in which Foo is needed, or where it came from, and neither would the exception that is raised. So the exception should not log it either, the right place to log is where the exception is caught (in general, that is. I'm sure there are specific counterexamples).

comment:5 Changed 9 years ago by stephen

  • Status changed from assigned to reviewing

comment:6 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

All OK, please merge.

Concerning exceptions and messages, that is a fair point. I was thinking more of the high-level "catch-all" clauses. Perhaps if it really is a problem, we could do something such as including a message ID in the exception and listing that ID in the message when the exception is caught. At least each exception instance would be uniquely identified in a way that might be easier for the end-user to interpret.

comment:7 Changed 9 years ago by jelte

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

Thanks, merged, closing ticket.

Regarding the message ID, might be good, but again not always applicable (some message id depend on that context, e.g. read_domain_name(str) might raise a CANNOTPARSE exception but there may be several error id's associated with that 'LOADZONE_CANNOT_PARSE_DNAME unable to parse domain name on line %1: %2' 'XFROUT_CONFIG_CANNOT_PARSE_DNAME xfrout config contains an unparseable domain name: %1'. (not that these exist, but purely as an example).

comment:8 Changed 9 years ago by stephen

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