Opened 9 years ago

Closed 8 years ago

#736 closed enhancement (complete)

Implement logging configuration

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

Description

Set up the system so that it is possible to configure the logging. This will involve editing the .spec file to define the relevant logging parameters. It is assumed that the C++/Python logging will access this information via the normal methods.

Some thoughts on the subject can be found at LoggingCppApiDesign.

Subtickets

Change History (13)

comment:1 Changed 9 years ago by shane

  • Estimated Difficulty changed from 0.0 to 7
  • Milestone changed from Year 3 Task Backlog to Sprint-20110419

comment:2 Changed 9 years ago by shane

  • Priority changed from major to critical

comment:3 Changed 9 years ago by stephen

  • Milestone changed from Sprint-20110503 to Year 3 Task Backlog

comment:4 Changed 8 years ago by stephen

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

comment:5 Changed 8 years ago by stephen

  • Component changed from Unclassified to logging

comment:6 Changed 8 years ago by jelte

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

comment:7 Changed 8 years ago by jelte

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

ready for review, trac736.

some notes:

I've made ModuleCCSession so that it automatically tracks logging
configuration (so modules don't have to do this themselves, they do have
to tell ModuleCCSession to do so with a boolean in its constructor,
b10-resolver now does this).

We seem to be missing a bit of functionality in the C++ part of
configuration data classes; for one i've created a ticket (993, direct
list element addressing), and another i want to think a bit about (i would
really like to have access to a specific part of the specification from a
single configuration value; now we have to traverse the specification tree
from the base). But even without this is all possible, as implemented in
the added code to ccsession.cc, just a bit awkward.

I did change the callback argument footprint to addRemoteConfig; it also
needs the ConfigData? element (so that it can get to the defaults), updated
in the other places where this was called.

I also updated a bit of code in config_data; I ran into a few bugs when
implementing this (it actually got some wrong data in some cases), and
refactored the function that had gotten a bit long.

Example configuration session (assumes Boss is already set up so that it is running the resolver, not auth):

> ./src/bin/bindctl/run_bindctl.sh 
["login success "] login as root
> config show Logging
Logging/loggers	[]	list	(default)
> config add Logging/loggers
> config show all Logging
Logging/loggers[0]/name	""	string	(default)
Logging/loggers[0]/severity	"INFO"	string	(default)
Logging/loggers[0]/debuglevel	0	integer	(default)
Logging/loggers[0]/additive	false	boolean	(default)
Logging/loggers[0]/output_options	[]	list	(default)
> config set Logging/loggers[0]/name b10-resolver
> config set Logging/loggers[0]/debuglevel DEBUG
Error: DEBUG is not an integer
> config set Logging/loggers[0]/severity DEBUG
> config set Logging/loggers[0]/debuglevel 999
> config add Logging/loggers[0]/output_options
> config show Logging/loggers[0]/output_options
Logging/loggers[0]/output_options[0]/destination	"console"	string	(default)
Logging/loggers[0]/output_options[0]/stream	"stdout"	string	(default)
Logging/loggers[0]/output_options[0]/flush	false	boolean	(default)
Logging/loggers[0]/output_options[0]/facility	""	string	(default)
Logging/loggers[0]/output_options[0]/filename	""	string	(default)
Logging/loggers[0]/output_options[0]/maxsize	0	integer	(default)
Logging/loggers[0]/output_options[0]/maxver	0	integer	(default)
> config set Logging/loggers[0]/output_options[0]/destination file
> config set Logging/loggers[0]/output_options[0]/filename /tmp/bind10.log
> config commit
> 

Note that due to a bug in the current bindctl, you can only use 'add' to add a clean item to an existing list if the last item you added has been changed in any way (i don't remember if i had already created a ticket for that but i will if not)

comment:8 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:9 Changed 8 years ago by jelte

oh if you have already pulled, i just pushed one more commit (I had forgotten to add a log message to configdef.mes, and i had forgotten to remove a few debug print statements). Commit id is 542a15d604018ea73a5a28f26b30b92fb668e399

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

  • Owner changed from stephen to jelte

Reviewed commit 542a15d604018ea73a5a28f26b30b92fb668e399

src/bin/cfgmgr/plugins/b10logging.py
As each logger should have a name, perhaps it would be useful to include its name in any error message?

The check for "filename" occurs whether or not the destination is a file (contrary to what the error message says).

There could be a check that if "facility" is present, the output is to syslog.

Question: Although the logging code makes the distinction between "file", "console" and "syslog", is there a need for the user-visible configuration to do so? Could we not remove the "console" destination and have it that file output to "stdout" or "stderr" routes output to the console? (In fact, how about replacing "filename", "stream" and "facility" with "output", and interpret the value of "output" in a destination-specific way?)

src/lib/config/ccsession.h
ModuleCCSession constructor: the documentation for the arguments "command_handler" and "start_immediately" is not in the correct place in the header.

Is the "handle_logging" argument to the ModuleCCSession constructor a temporary construct? (Under what circumstances will this module not take care of logging configuration?)

src/lib/config/config_data.cc
findListOrMapSubSpec: Is, strictly speaking, the header correct? It finds the inner-most _spec if the specification is nested.

findItemInSpecList: (throwing an exception when nothing is found.) Presumably not finding the element is an unexpected occurrence? Does this make the handling of optional elements more awkward? Would returning a null pointer be better?

find_spec_part: not in the changes made here, but would use of tokens() (in util/strutil.{h,cc}) simplify the code?

Comment: again not in the changes here, but some of the functions in the file are either in the anonymous namespace or are declared static. This means they do not have unit tests.

comment:11 in reply to: ↑ 10 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

two commits, one for the minor issues, one for the change to 'output' for facility/file/stream, for the other points see below

Replying to stephen:

Reviewed commit 542a15d604018ea73a5a28f26b30b92fb668e399

src/bin/cfgmgr/plugins/b10logging.py
As each logger should have a name, perhaps it would be useful to include its name in any error message?

ack, done

The check for "filename" occurs whether or not the destination is a file (contrary to what the error message says).

There could be a check that if "facility" is present, the output is to syslog.

since there is now just the 'output', these issues are either changed or irrelevant now.

Question: Although the logging code makes the distinction between "file", "console" and "syslog", is there a need for the user-visible configuration to do so? Could we not remove the "console" destination and have it that file output to "stdout" or "stderr" routes output to the console? (In fact, how about replacing "filename", "stream" and "facility" with "output", and interpret the value of "output" in a destination-specific way?)

Ok. This is what I initally meant with compacting the options, but I had chosen to make it a one-to-one mapping first. This prompted me to indeed change it. stream, facility and filename are now gone, and replaced by the one setting 'output'. Since destination defaults to console, output defaults to stdout.

note that you must delete your b10-config.db or remove the loggers part if you have one, this is an incompatible change and we don't have module-specific versioning (yet, but even if we did i probably wouldn't up it for a non-merged change).

src/lib/config/ccsession.h
ModuleCCSession constructor: the documentation for the arguments "command_handler" and "start_immediately" is not in the correct place in the header.

moved

Is the "handle_logging" argument to the ModuleCCSession constructor a temporary construct? (Under what circumstances will this module not take care of logging configuration?)

Not temporary, but backwards compatible, and 'future-compatible' for modules that may not want logging to be handled (if any). Right now auth doesn't do logging yet. We can change the default to true, or perhaps remove the option not to do it completely though, if there is a preference for that.

src/lib/config/config_data.cc
findListOrMapSubSpec: Is, strictly speaking, the header correct? It finds the inner-most _spec if the specification is nested.

Added comment to this effect.

findItemInSpecList: (throwing an exception when nothing is found.) Presumably not finding the element is an unexpected occurrence? Does this make the handling of optional elements more awkward? Would returning a null pointer be better?

No, this function should only get specification data. We do not have optional specification data, so if there is an identifier that cannot be found, either the code calling it or the specification is wrong.

find_spec_part: not in the changes made here, but would use of tokens() (in util/strutil.{h,cc}) simplify the code?

Perhaps, there is the slight detail that on the last item it needs to do something else than for the ones before that. I'll take a look at that (but not now).

Comment: again not in the changes here, but some of the functions in the file are either in the anonymous namespace or are declared static. This means they do not have unit tests.

Yep. It also means we won't accidentally be using them outside of this scope, which, if i look at your previous comments, may be a very good thing :) They are so extremely specific to the one function that calls them, I honestly don't know how much value they would be for other parts. I do realize that this is not very TDD, but:

On a more general note, I think we are running against the limits of using 'direct' data types for both config values and their specifications (the ElementPtrs? in c++ and the native types in python). This was a cute idea IMO, since we can easily pass these around over any channel, but their handling is getting annoying. So I'm thinking about making a real class hierarchy for these. Anonymously namespaced helper functions should then be much less necessary (if at all). I do want to give a bit of design thought to this though, and certainly don't want to implement that as a side-effect of any other ticket such as this.

comment:12 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

All OK, please merge.

Comment: again not in the changes here, but some of the functions in the file are either in the anonymous namespace or are declared static. This means they do not have unit tests.

Yep. It also means we won't accidentally be using them outside of this scope, which, if i look at your previous comments, may be a very good thing :) They are so extremely specific to the one function that calls them, I honestly don't know how much value they would be for other parts. I do realize that this is not very TDD, but:

One scheme I have tried is to put these type of functions as static protected methods of the class. In the unit tests, declare a derived class and add public methods that do nothing more than call the superclass's protected methods. This protects them against accidental use (except for derived classes, and we have to assume that the person deriving the class knows what they're doing) but makes them available for testing.

On a more general note, I think we are running against the limits of using 'direct' data types for both config values and their specifications (the ElementPtrs?? in c++ and the native types in python).

Would the class boost::variant be of use here?

comment:13 Changed 8 years ago by jelte

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

Thanks, merged, closing ticket.

Note that merging this caused me to consider some more general logging tickets, see #1003, #1004 and #1005

Note: See TracTickets for help on using tickets.