Opened 9 years ago

Closed 9 years ago

#1003 closed task (complete)

root logger names

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

Description

Root logger names are currently based on the name of the binary they're from (e.g. b10-resolver). This, however, is not how they appear internally (in for instance bindctl, where a module name is based on what is specified in the .spec file (e.g. Resolver)).

We should make those the same, and strip 'b10-' for configuration purposes. However, in the logger output, we do want b10- included. The suggestion is to prefix the name read in the configuration with 'b10-' and leave the module code as it is (and make it a convention that the name from the specfile and the actual binary name should match). That way, you configure resolver logging with the name 'resolver', and in the printed output it would become 'b10-resolver'.

Subtickets

Change History (11)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from New Tasks to Sprint-20110628

comment:2 Changed 9 years ago by jreed

  • Type changed from defect to task

comment:3 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 2

comment:4 Changed 9 years ago by stephen

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

comment:5 Changed 9 years ago by stephen

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

Note: src/bin/zonemgr/zonemgr.py.in has been updated to initialize logging as without it the unit tests fail. (Changes to cfgmgr mean that the logging library is now referenced, and the logging code throws an exception unless it has been initialized. Ticket #1154 has been raised to complete the conversion of zonemgr to the new logging interface.)

comment:6 Changed 9 years ago by jinmei

I made a few trivial changes (basically editorial/style related)
and pushed them directly to the branch.

ccsession.cc

  • This comment seems to be a verbatim copy of the ticket description, and doesn't 100% match what the function specifically does:
    +// This function prefixes the name read in the configuration with 'b10-" and
    +// leaves the module code as it is. [...]
    
    that is, "this function...leaves the module code as it is" doesn't make sense.
  • This comment doesn't make sense to me:
    +    // since we'll only be updating one first-level element,
    +    // and we return as const again, a shallow map copy is
    +    // enough
    
    What does 'return as const' mean? Perhaps you intended to make the return value of this function ConstElementPtr??
  • This doesn't parse:
    +// Copies the map for a logger, changing the of the logger.
    
    "changing the <something> of the logger"?
  • Is it safe to call LOG_xxx in getRelatedLoggers(). I don't follow all possible control flows, but this seems to be producing a log message while configuring logging, which looks fragile.

config_messages.mes

  • CONFIG_LOG_EXPLICIT: s/will updated/will be updated/?
    +configuration for the program will updated with the information.
    
    same for CONFIG_LOG_WILD_MATCH.

ccsession_unittests.cc

  • why adding iostream? The other changes to this file don't seem to require it (at least not directly)
  • CCSessionTest() constructor: not a big deal, but I'd make root_name const and initialize it in the initialization list.
  • I'd test the case where the input name is "b10-test".
        doRelatedLoggersTest("[ { \"name\": \"b10-test\" }]",
                             "[]");
    

comment:7 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

b10Prefix(): I don't see the need for cast. According to http://www.cplusplus.com/reference/clibrary/cctype/tolower/ "The value is returned as an int value that can be implicitly casted to char."

OK, removed. (I've never really understood why toupper()/tolower() take ints as arguments and return int - I think I've only ever used them in the context of passing in a char and converting the result to a char.)

This comment seems to be a verbatim copy of the ticket description, and doesn't 100% match what the function specifically does

I've rewritten the header comment.

What does 'return as const' mean? Perhaps you intended to make the return value of this function ConstElementPtr??

Good catch, I did. I've also updated the associated comment.

This doesn't parse...

Fixed.

Is it safe to call LOG_xxx in getRelatedLoggers(). I don't follow all possible control flows, but this seems to be producing a log message while configuring logging, which looks fragile.

In this case it is OK. The messages are generated during the parsing of the configuration and construction of the data structure used to modify the logging. During this process, messages can be safely logged using whatever is the current logging configuration.

CONFIG_LOG_EXPLICIT: s/will updated/will be updated/?
same for CONFIG_LOG_WILD_MATCH.

Well spotted.

why adding iostream? The other changes to this file don't seem to require it (at least not directly)

Left over from debugging a problem. Removed.

CCSessionTest() constructor: not a big deal, but I'd make root_name const and initialize it in the initialization list.

Good point. Done.

I'd test the case where the input name is "b10-test".

Good thought. Added.

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

Replying to stephen:

The revised version basically looks okay. Please feel free to merge.

One follow up comment:

Is it safe to call LOG_xxx in getRelatedLoggers(). I don't follow all possible control flows, but this seems to be producing a log message while configuring logging, which looks fragile.

In this case it is OK. The messages are generated during the parsing of the configuration and construction of the data structure used to modify the logging. During this process, messages can be safely logged using whatever is the current logging configuration.

This code path seems to be called in itialization, too (e.g., from
ModuleCCSession::addRemoteConfig() via the ModuleCCSession
constructor), not only at the update time. I'm not sure if this
really cause a problem or some kind of counter intuitive behavior, but
if so, I'd suggest creating a separate ticket for that (on closer look
this doesn't seem to be specific to getRelatedLoggers(). The
ModuleCCSession itself has LOG_xxx in it, for example).

comment:10 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:11 Changed 9 years ago by stephen

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

Committed in change d2f96b7e1e3e4a5917ea73a56429fa645d8ede7c

Note: See TracTickets for help on using tickets.