BIND 10 #2155: Collect query/response statistics items in Auth module
BIND 10 Development
do-not-reply at isc.org
Mon Aug 27 13:04:42 UTC 2012
#2155: Collect query/response statistics items in Auth module
-------------------------------------+-------------------------------------
Reporter: | Owner: y-aharen
y-aharen | Status: reviewing
Type: | Milestone:
enhancement | Sprint-20120904
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
So, from the most serious. It does not compile for me:
{{{
/bin/sed -e "s|@@LOCALSTATEDIR@@|/home/vorner/testing/bind10/var|"
auth.spec.pre >auth.spec
../../../src/bin/auth/statistics_items.h:75:13: error:
‘{anonymous}::SocketCounterItemName’ defined but not used [-Werror=unused-
variable]
../../../src/bin/auth/statistics_items.h:250:13: error:
‘{anonymous}::QRCounterItemName’ defined but not used [-Werror=unused-
variable]
cc1plus: all warnings being treated as errors
make[6]: *** [statistics.o] Error 1
make[6]: *** Waiting for unfinished jobs....
make[6]: Leaving directory `/home/vorner/work/bind10/src/bin/auth'
}}}
Also, distcheck does not pass, for a different reason (is the new
statistics_items.h in AUTH_SOURCES in the makefile? ):
{{{
g++ -DHAVE_CONFIG_H -I. -I../../../../src/bin/auth -I../../..
-I../../../../src/lib -I../../../src/lib -I../../../../src/bin
-I../../../src/bin -I../../../../src/lib/dns -I../../../src/lib/dns
-I../../../../src/lib/cc -I../../../src/lib/cc
-I../../../../src/lib/asiolink -I../../../src/lib/asiolink -DOS_LINUX
-I../../../../ext/asio -I../../../../ext/coroutine
-DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-strings -Woverloaded-
virtual -Wno-sign-compare -Werror -fPIC -g -O2 -MT statistics.o -MD -MP
-MF .deps/statistics.Tpo -c -o statistics.o
../../../../src/bin/auth/statistics.cc
../../../../src/bin/auth/statistics.cc:16:35: fatal error:
auth/statistics_items.h: No such file or directory
compilation terminated.
make[7]: *** [statistics.o] Error 1
make[7]: Leaving directory
`/home/vorner/work/bind10/bind10-devel-20120712/_build/src/bin/auth'
}}}
I also must agree with jelte and jinmei that the conversion from opcodes
to the enum by long if-else if-else if-… sequence is rather ugly, for
following reasons:
* It is long code that is not easy to read (the branches are almost the
same, but they differ slightly, making the concentration drift away while
reading them and miss important things, like that this one has + 1
somewhere in it).
* It is fragile. It depends crucially on the order of the enum. If
someone changes the order of the enum (by accident, or something), odd
things like increments of completely unrelated counters can happen.
* As jinmei noted, it may be performance problem.
* It is not needed. What it does is saving few bytes by compacting some
opcodes together, but that is very small amount.
So I propose this:
* Have several large enough arrays (one for RRType, one for the opcodes,
etc).
* Index by the values from DNS directly. This is faster and much simpler
to read.
* If the compression of multiple opcodes or other things is needed, do it
during the report generating (when sending the statisticts). This is much
less performance sensitive place and can be actually coded in a simpler
way by defining constants of names and indices in the array to sum
together, like this (more like a pseudo-code than really valid C++ and the
numbers are made up a little, but it's the idea that matters):
{{{#!c++
static Definition definitions[] = {
{"QUERY", {0, 1, 2}},
{"NOTIFY", {4, 5}},
{"OTHER", {3}}
};
…
for definition(definitions) {
unsigned sum = 0;
for i (definition.indices) {
sum += counter_array[i];
}
if (definition.name == "OTHER") {
for i (6..) {
sum += counter_array[i];
}
}
result.push(definition.name, sum);
}
}}}
* Also, we would be able to provide detailed statistics that are not
compressed, if asked for, just by providing the raw data of the array.
Aside from these, there are several style and comment issues:
The resumeServer method header:
* I think you used wrong verb, I have no idea what you meant.
{{{c++
/// \param stats_attrs query/response attributes for statistics which
is
/// not explained in \p messsage
}}}
* Also, the fact that the stats_attrs are passed as reference and
modified as a side effect is somewhat counter-intuitive (as it is an input
parameter) and ugly. If you come up with no better idea than that, it
would deserve a comment explaining the odd fact and the reasons for it.
This bit of code:
{{{#!c++
// statistics: check TSIG attributes
// SIG(0) is currently not implemented in Auth
stats_attrs.setQuerySig(true, false,
( tsig_error == TSIGError::NOERROR() ));
}}}
* What is the thing with SIG(0)? Is it my lack of knowledge about DNS
that it makes no sense to me?
* The parentheses around the == comparison are both unneeded and wrongly
placed according to our style (in our style, they would have no spaces
inside them). Maybe remove them completely?
Why are there the extra braces around the EDNS thing?
{{{#!c++
// statistics: check EDNS
// 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());
}
}
// statistics: check OpCode
// note: This can only be reliable after TSIG check succeeds.
stats_attrs.setQueryOpCode(opcode.getCode());
}}}
The `answerHasSent` looks as wrong in English. Maybe `answerWasSent` or
`answerIsSent`?
Spacing in `void registerStatisticsValidator (Counters::validator_type
validator);` is wrong.
When looking at this code, it looks like it is missing UDP. I know the
other parts compute it when needed, but a comment here that it is not
forgotten, but will be computed, would be nice.
{{{#!c++
CountersImpl::inc(const QRAttributes& qrattrs, const Message& response) {
// protocols carrying request
if (qrattrs.req_ip_version_ == AF_INET) {
server_qr_counter_.inc(QR_REQUEST_IPV4);
} else if (qrattrs.req_ip_version_ == AF_INET6) {
server_qr_counter_.inc(QR_REQUEST_IPV6);
}
if (qrattrs.req_transport_protocol_ == IPPROTO_TCP) {
server_qr_counter_.inc(QR_REQUEST_TCP);
}
}}}
Why is this code commented out? Is it deprecated? If so, shouldn't it be
deleted completely? If it is just broken, a comment with FIXME or TODO
mark would be in place:
{{{#!c++
CountersImpl::inc(const QRAttributes& qrattrs, const Message& response) {
// protocols carrying request
if (qrattrs.req_ip_version_ == AF_INET) {
server_qr_counter_.inc(QR_REQUEST_IPV4);
} else if (qrattrs.req_ip_version_ == AF_INET6) {
server_qr_counter_.inc(QR_REQUEST_IPV6);
}
if (qrattrs.req_transport_protocol_ == IPPROTO_TCP) {
server_qr_counter_.inc(QR_REQUEST_TCP);
}
}}}
The doxygen comment for the `QRAttributes` class should be outside the
class, not inside ‒ this documents the friend declaration now.
The `setOrigin` method actually can throw an allocation exception, because
it assigns std::string object.
Again, commented-out code in the tests:
{{{#!c++
#endif
#if 0
// DISABLED: these interfaces, namely,
// Counters.inc(const Counters::ServerCounterType&) and
// Counters.inc(const isc::dns::Opcode&) and
// Counters.inc(const isc::dns::Rcode&) has been removed.
}}}
If the interfaces were removed completely, should the tests be just
removed? Otherwise, for how long will the tests be there?
Also, it looks slightly strange to end one if(0) and then start it again
right away ‒ wouldn't it be enough to have just one big (provided there's
a reason to leave them there)?
--
Ticket URL: <http://bind10.isc.org/ticket/2155#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list