BIND 10 #2155: Collect query/response statistics items in Auth module

[BIND 10 Development]

#2155: Collect query/response statistics items in Auth module
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  y-aharen
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20121023
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  b10-auth                           |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => y-aharen


Comment:

 Hello

 Replying to [comment:14 y-aharen]:
 > > It seems the SIG(0) is some kind of signing algorithm or something?
 But if so, it is unclear how it relates to the surrounding code and why it
 is important for the statistics code that it is not implemented in Auth.
 > For the detail of SIG(0), please see RFC 2931.
 > Per-RRtype counter for SIG(0) is defined in BIND 9. I included it for
 BIND 9 compatibility.

 Can you add the note about compatibility with bind9, then, into the
 comment?

 > > Also, what is the reason for this bit of code having a separate block
 around it?
 > > {{{#!c++
 > >         //     note: This can only be reliable after TSIG check
 succeeds.
 > >         {
 > >             ConstEDNSPtr edns = message.getEDNS();
 > >             if (edns != NULL) {
 > >                 stats_attrs.setQueryEDNS(true, edns->getVersion() ==
 0);
 > >                 stats_attrs.setQueryDO(edns->getDNSSECAwareness());
 > >             }
 > >         }
 > > }}}
 > I've put them into the block to indicate edns is used only for
 statistics. If it is unclear and meaningless, I'll remove the surrounding
 braces.

 A comment saying so may be better, this really is not obvious what it
 means.

 > > I smell something bad if I see code like this. This is very long and
 easy to make the number of lines wrong in one way or another. And, when
 you do it wrong, nothing breaks, you just get wrong answers:

 I still don't think it is optimal solution, but the comments with numbers
 should be good enough.

 > > Also, the same test as above and most of the similar check one value
 less than what is being listed in the comments. Is it forgotten, or is it
 some trick I don't see?

 What about this comment? I didn't find it addressed anywhere.

 > > May I suggest removing useless code, like empty constructors and
 destructors?
 > > {{{#!c++
 > >     CountersTest() : counters() {}
 > >     ~CountersTest() {}
 > > }}}
 > >
 > > {{{#!c++
 > > Counters::~Counters() {}
 > > }}}
 > >
 > > {{{#!c++
 > > inline QRAttributes::~QRAttributes()
 > > {}
 > > }}}
 > >
 > > {{{#!c++
 > > inline Counter::~Counter() {}
 > > }}}
 > >
 > > {{{#!c++
 > > inline ~ConstIterator() {}
 > > }}}
 > >
 > > {{{#!c++
 > > inline CounterDictionary::~CounterDictionary() {}
 > > }}}
 > After removing some empty destructors, compiler complains that something
 is wrong. I have no idea to remove them safely, I'll leave them.

 Could you paste the error, please? I'd be interested to know what is
 happening there, because it seems it shouldn't be happening.

 > > Also, the canonical way to inline methods is not the `inline` keyword,
 but having them inside the class definition itself, like this:
 > OK, modified as figured.

 The `inline` keyword is not actually needed when the method is inside the
 class definition, it is implicit. That's why I suggested it ‒ the inline
 keywords look like waste of space O:-).

 > > The constructor and `reset` method of `QRAttributes` look very
 similar. Would it be better to just call reset from the constructor?
 > Yes, they works similarly. I referred to isc::auth::Query class, which
 also has explicit constructor and reset() member function. If it's not a
 good example, I'll fix it.

 I don't know. On one side, the construction with initializers might be
 slightly faster than calling reset(). But I'd guess the compiler would
 optimise it anyway.

 But what I don't like on this approach is the duplication of code. Once a
 new item is added there, it needs to be set to the default value at two
 places and one may forget to update one of them. So I personally prefer to
 reuse the code of reset() by calling it from the constructor, but it may
 be a subjective preference.

 Thank you

-- 
Ticket URL: <http://bind10.isc.org/ticket/2155#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development