Opened 9 years ago

Closed 9 years ago

#899 closed task (complete)

Implement logging using Log4cplus

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

Description

This is dependent on the success of ticket #898, and involves re-implementing logging using Log4cplus.

(See minutes of the 2011-05-03 sprint planning meeting for more context. Also note that the files for the implementation of logging using log4cxx are still present in the src/lib/log directory: these may be useful as both Log4cplus and log4cxx are similar.)

Subtickets

Change History (12)

comment:1 Changed 9 years ago by stephen

Additional information can be found in this thread in the bind10-dev mailing list archive.

comment:2 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:3 Changed 9 years ago by jreed

I created a branch of #898 that provides configure.ac addition for testing log4cplus. As of now it is not in master. Use that code in trac898 for this 899.

comment:4 Changed 9 years ago by stephen

  • Component changed from Unclassified to logging
  • Milestone changed from Year 3 Task Backlog to Sprint-20110531

The files src/lib/log/xdebuglevel.* may be useful as examples; these are the implementation files used to link the logging code into log4cxx. They can be removed once the logging is successfully linked into log4cplus.

comment:5 Changed 9 years ago by stephen

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

comment:6 Changed 9 years ago by stephen

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

comment:7 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

The patch is rather long one. But it is true it's quite simple, so I think it is mostly OK, with some details:

  • Why is the destructor of DerivedLogger explicitly mentioned and virtual? As this class has no virtual functions (except for the inherited ones), there's no need for one, the default will be virtual, because the destructor of parent class is already virtual. Not that it would be wrong, it just surprised me.
  • I don't like including the linker flags everywhere. Is it really needed? Because I think the whole purpose of our liblog is to hide the internal implementation.
  • This bit looks strange:
    +
    +/// Minimum/maximum debug levels.  These are defined wi
    +
    
    (in logger_level.h)
  • Isn't a class with only static methods a little bit misleading? Such class is more like namespace, isn't it? (Not that it would produce different code, in the end)
  • specify exact directory of log4cplus library and headers ‒ this message looks strange to me. Is it possible to specify inexact directory? And it looks like they should live in one directory, but one is in lib, one in include, I really wondered which one of them you mean for a short while. Wouldn't „installation prefix of log4cplus“ be better?
  • This FIXME in Formatter::arg was wrong, because the underlying replace did replace all occurrences. However, what you describe isn't the biggest problem (that is, actually, quite expected output). If we use the original example, you would now get "42 42" (the first %1 gets replaced to %2 and then both %2 get replaced), which is by no means expected output (it is understandable once you get it, but it's surprising).

Also, as this brings a new dependency, I believe we need a changelog and a heads-up email for developers to install it if they didn't already.

Thanks

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

  • Owner changed from stephen to vorner

Why is the destructor of DerivedLogger explicitly mentioned and virtual? As this class has no virtual functions (except for the inherited ones), there's no need for one, the default will be virtual, because the destructor of parent class is already virtual. Not that it would be wrong, it just surprised me.

No good reason (other than DerivedLogger was copied from elsewhere), it's been removed.

I don't like including the linker flags everywhere. Is it really needed? Because I think the whole purpose of our liblog is to hide the internal implementation.

Well caught. It should have been in the Makefile.am for liblog, not everywhere else! Corrected.

This bit looks strange...

Incomplete last sentence removed - it now reads just "Minimum/maximum debug levels".

Isn't a class with only static methods a little bit misleading? Such class is more like namespace, isn't it? (Not that it would produce different code, in the end)

Possibly, but I wanted to group the methods together (the init() call relates to two of those methods) and indicate that they are implementation-specific functions. Putting them in the general isc::log namespace loses that grouping, but I didn't want to create a new namespace under it. A class name ending in Impl provides the grouping and conveys the information that they are implementation-specific (so are related to other xxxImpl classes).

specify exact directory of log4cplus library and headers ‒ this message looks strange to me. Is it possible to specify inexact directory? And it looks like they should live in one directory, but one is in lib, one in include, I really wondered which one of them you mean for a short while. Wouldn't „installation prefix of log4cplus“ be better?

configure.ac also includes the messages "specify exact directory of Botan library" and "specify exact directory for Boost headers", so I guess this one is consistent.

This FIXME in Formatter::arg was wrong, because the underlying replace did replace all occurrences. However, what you describe isn't the biggest problem (that is, actually, quite expected output). If we use the original example, you would now get "42 42" (the first %1 gets replaced to %2 and then both %2 get replaced), which is by no means expected output (it is understandable once you get it, but it's surprising).

I've updated the comment.

comment:10 in reply to: ↑ 9 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

Isn't a class with only static methods a little bit misleading? Such class is more like namespace, isn't it? (Not that it would produce different code, in the end)

Possibly, but I wanted to group the methods together (the init() call relates to two of those methods) and indicate that they are implementation-specific functions. Putting them in the general isc::log namespace loses that grouping, but I didn't want to create a new namespace under it. A class name ending in Impl provides the grouping and conveys the information that they are implementation-specific (so are related to other xxxImpl classes).

OK. Just a matter of preference I guess.

specify exact directory of log4cplus library and headers ‒ this message looks strange to me. Is it possible to specify inexact directory? And it looks like they should live in one directory, but one is in lib, one in include, I really wondered which one of them you mean for a short while. Wouldn't „installation prefix of log4cplus“ be better?

configure.ac also includes the messages "specify exact directory of Botan library" and "specify exact directory for Boost headers", so I guess this one is consistent.

Ah, OK. I've never seen them, because I have these libraries from portage. Log4cplus wasn't there, so it's installed in different location.

So, I guess this can be merged. At last, it does build here, so hopefully not many breakages will happen.

comment:11 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5

comment:12 Changed 9 years ago by stephen

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

Committed as change db21e8ab55f132828542f1a34642d98bbaf6d8ee

Note: See TracTickets for help on using tickets.