Opened 9 years ago

Closed 8 years ago

#738 closed enhancement (complete)

Conversion of b10-auth to use new logging interface

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

Description

Examining the b10-auth code and changing any logging calls to use the new logging interface.

This involves:

  • Extracting messages into a separate message file. (This may be a unique file for b10-auth or it may be a file shared between separate programs).
  • Using the new logging calls.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by shane

  • Estimated Difficulty changed from 0.0 to 6
  • Milestone changed from Year 3 Task Backlog to Sprint-20110419
  • Priority changed from major to minor

comment:2 Changed 9 years ago by stephen

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

comment:3 Changed 8 years ago by shane

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

comment:4 Changed 8 years ago by stephen

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

comment:5 Changed 8 years ago by stephen

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

Messages output to stderr have been substituted by the new-style logging.

Also changes was the removal of the "verbose_mode" flag - this is not needed with the logging code.

comment:6 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

First of, the distcheck fails with this:

g++ -DHAVE_CONFIG_H -I. -I../../../../../src/bin/auth/benchmarks -I../../../..  -I../../../../../src/lib -I../../../../src/lib -I../../../../../src/bin -I../../../../src/bin  -I../../../../../ext/asio -I../../../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2 -MT auth_log.o -MD -MP -MF .deps/auth_log.Tpo -c -o auth_log.o `test -f '../auth_log.cc' || echo '../../../../../src/bin/auth/benchmarks/'`../auth_log.cc
In file included from ../../../../../src/bin/auth/benchmarks/../auth_log.cc:17:0:
../../../../../src/bin/auth/benchmarks/../auth_log.h:19:27: fatal error: auth_messages.h: No such file or directory
compilation terminated.
make[7]: *** [auth_log.o] Error 1
make[7]: *** Waiting for unfinished jobs....
In file included from ../../../../../src/bin/auth/benchmarks/../statistics.cc:16:0:
../../../../../src/bin/auth/auth_log.h:19:27: fatal error: auth_messages.h: No such file or directory
compilation terminated.
make[7]: *** [statistics.o] Error 1
In file included from ../../../../../src/bin/auth/benchmarks/../auth_srv.cc:62:0:
../../../../../src/bin/auth/auth_log.h:19:27: fatal error: auth_messages.h: No such file or directory
compilation terminated.
make[7]: *** [auth_srv.o] Error 1
mv -f .deps/query_bench.Tpo .deps/query_bench.Po
mv -f .deps/auth_config.Tpo .deps/auth_config.Po
make[7]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build/src/bin/auth/benchmarks'
make[6]: *** [all-recursive] Error 1
make[6]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build/src/bin/auth'
make[5]: *** [all] Error 2
make[5]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build/src/bin/auth'
make[4]: *** [all-recursive] Error 1
make[4]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build/src/bin'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build/src'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110519/_build'
make: *** [distcheck] Error 1

I guess this is some problem with ordering.

Then, I'm thinking, the arg(exc.what()) is quite common. Would it help if we defined template specialization for exception?

And, finally, few minor points:

  • AUTH_COMMAND_FAILED ‒ It is not clear about which command it talks. I guess it's about the command channel commands, but admin oblivious of the architecture might think which DNS commands there are.
  • AUTH_LOAD_TSIG says the TSIGs are accessed, which is not exact. They are being requested from the configuration database.
  • AUTH_LOAD_ZONE ‒ should „names zone“ be „named zone“?
  • „For some reason, the authoritative server has no session with the statistics module is available.“ ‒ this sounds odd, aren't there two verbs?
  • AUTH_RECEIVED_SENDSTATS ‒ issues → issued?
  • Indentation?
        } catch (const Exception& ex) {
    -        if (verbose_mode_) {
    -            cerr << "[b10-auth] failed to notify Zonemgr: " << ex.what() << endl;
    -        }
    +            LOG_ERROR(auth_logger, AUTH_ZONEMGR_COMMS).arg(ex.what());
             return (false);
    
  • I think this should be LOG_ERROR, after all, failing command should be rare and better if people complain that it is too verbose than if it is rotten inside and nobody knows.
     // TODO: SHould this be LOG_ERROR?
             LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_COMMAND_FAILED)
                       .arg(command_id).arg(ex.what());
    

With regards

comment:8 Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

First of, the distcheck fails with this...

This turned out to be auth_log.h containing the line '#include "auth_messages.h"'. auth_messages.h is created in the build directory, and so wasn't being found when the source and build directories were different. Changing the file specification to '<auth/auth_messages.h>' fixed the problem.

Then, I'm thinking, the arg(exc.what()) is quite common. Would it help if we defined template specialization for exception?

I think it would. We talked about logging exceptions some time ago (should an exception be automatically logged when raised?) and it might be worth discussing the whole issue on bind10-dev.

And, finally, few minor points:...

Typos and indentation fixed, some messages re-worded.

I think this should be LOG_ERROR, after all, failing command should be rare ...

Agreed. It was only LOG_DEBUG because in the original code, the message was only being issued if verbose_mode was set.

comment:9 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

It looks good, please merge.

Thanks

comment:10 Changed 8 years ago by stephen

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

Merged as commit c021505a1a0d6ecb15a8fd1592b94baff6d115f4

Note: See TracTickets for help on using tickets.