BIND 10 #2157: Create an interface to pass statistics counters in Auth module
BIND 10 Development
do-not-reply at isc.org
Fri Jan 11 07:10:52 UTC 2013
#2157: Create an interface to pass statistics counters in Auth module
-------------------------------------+-------------------------------------
Reporter: y-aharen | Owner:
Type: enhancement | jinmei
Priority: medium | Status:
Component: b10-auth | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130122
Sub-Project: DNS | Resolution:
Estimated Difficulty: 8 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by y-aharen):
* owner: y-aharen => jinmei
Comment:
Hello,
Replying to [comment:43 jinmei]:
> Replying to [comment:41 y-aharen]:
> > > - `MessageAttributes` get method: `const` isn't necessary for the
> > > parameters.
> > Sorry I couldn't get it: does it mean the return value of
> > getRequestOpCode() isn't necessary to be const?
>
> Oops, I should probably mean set methods like this:
> {{{#!cpp
> void setRequestEDNS0(const bool is_edns_0) {
> bit_attributes_[REQ_IS_EDNS_0] = is_edns_0;
> }
> }}}
> and actually, in these cases where they also have definitions, `const`
> is not necessarily redundant because it indicates the parameter
> is mutable throughout the method. For such one-line function,
> however, that should be obvious at a glance, so I'd still omit it, but
> that's a matter of taste and I'd leave it to you.
OK. I'll leave them const to make sure the parameter is immutable as
it's a setter.
> > > > > - this description also applies to the "referral" case.
> > > > > {{{
> > > > > qrynxrrset QR_QRYNXRRSET Number of queries
received by the b10-auth server resulted in NOERROR but the number of
answer RR == 0.
> > > > > }}}
> > > > Corrected as described above.
> > >
> > > I don't see any change on this.
> > I misunderstood you've pointed it didn't counted not only for
"queries"
> > (and it was fixed).
> > The description comes from BIND 9. Is it OK to mention AA bit? Or
another
> > approach to describe it (and QryReferral)?
>
> Mostly forgot, but I guess I meant the description isn't accurate. To
> be so, one way is to clarify it's the case where AA is set, but I
> don't much care how we resolve it as long as it's clear.
OK, I clarified it's the case where AA is set (git 3d3d75a).
> > > '''statistics.h'''
> > >
> > > - this comment for `Counters::inc` doesn't seem to be correct
> > > {{{#!cpp
> > > /// This method is mostly exception free. But it may still throw
a
> > > /// standard exception if memory allocation fails inside the
method.
> > > }}}
> > I think it's correct; it can throw std::bad_alloc from
> > isc::statistics::Counter::Counter().
>
> Where does it construct `isc::statistics::Counter::Counter()`?
Sorry I was confused. I removed the comment (git 7f75e58).
> > > > > - Why is `stats_attrs` defined as part of `AuthSrvImpl`?
> > >
> > > > It's for optimization similar to isc::auth::Query. It improved the
> > > > overhead to around 3% with other optimizations as mentioned in
bind10-dev
> > > > list.
> > > > However, using std::bitset saves some memory, so I updated the
branch.
> > >
> > > Does that particular change (defining it in `AuthSrvImpl`) really
> > > contribute to the performance improvement?
> > >
> > > So, unless this particular change really improves the performance,
I'd
> > > generally avoid this approach.
> > OK, I updated my branch (git 2afe338).
> > I've compared query rate with query_bench using root zone, 50% of
> > non-existence queries. It shows 1.72% of performance drop.
>
> Hmm, it's hard to believe...maybe we should perform benchmark check on
> the actual qps. Hopefully it's marginal.
I hope so too. Anyway, I'll leave it as is.
> > > - generate_docfile: at least for me it produces garbage:
> > > {{{
> > > b'<variablelist><varlistentry><term>request.v4</term>...'
> > > }}}
> > > This also suggests why we need a test.
> > I couldn't reproduce it with Python 3.1 and Python 3.3.
> > Could you please let me know your environment?
>
> It's Python 3.2 on Mac OS X 10.8, but this doesn't happen with the
> revised code. I guess the addition for the prettier print indirectly
> solves it.
OK, thank you for your information.
> '''statistics.{h,cc}'''
>
> - about `Opcode`: I'd like to avoid using a valid type (like
> RESERVED15) for placeholder until it's set. It can be an easy
> source of bug that we leak that value. There are several ways to
> avoid that, but if the overhead is not big I suggest
> boost::optional.
> - type of opcde, maybe boost::optional<Opcode>?
OK, I updated the branch to use boost::optional (git 566a0c7).
> - `using xxx` in a header file is a dangerous practice and I'd avoid it:
> {{{#!cpp
> using isc::dns::Opcode;
> }}}
OK, removed using declaration (git 7eb39cf).
> - `TransportProtocol` sounds a bit awkward because these are enums and
> essentially identifiers, not "protocols". Maybe something like
> `TransportProtocolID` or `TransportProtocolType`?
OK, renamed to TransportProtocolType. Also, renamed IPVersion to
IPVersionType (git 370c09f).
> - I didn't notice this before, but some of REQ_IS_xxx's sound awkward.
> {{{#!cpp
> enum BitAttributes {
> REQ_IS_EDNS_0, // request is EDNS ver.0
> REQ_IS_DNSSEC_OK, // DNSSEC OK (DO) bit is set in
request
> REQ_IS_TSIG, // request is signed with valid TSIG
> REQ_IS_BADSIG, // request is signed but bad
signature
> RES_IS_TRUNCATED, // response is truncated
> RES_IS_TSIG_SIGNED, // response is TSIG signed
> BIT_ATTRIBUTES_TYPES
> };
> }}}
> REQ_IS_EDNS_0 and REQ_IS_TSIG are particularly awkward. They are
> not "EDNS" or "TSIG", they are a request that has EDNS or (signed
> with) TSIG. Please revisit the naming. Same applies to the
> comments.
I updated the branch (git 89cc0b5).
> - `MessageAttributes` constructor: I suspect I don't have to
> initialize bit_attributes_ explicitly.
OK, I removed explicit initialization (git cb24062).
> - setRequestTransportProtocol(): I personally think assert() is too
> strong:
> {{{#!cpp
> assert(transport_protocol != TRANSPORT_UNSPEC);
> }}}
> because we cannot control what is passed to the method.
> Conventionally we throw in such cases.
I thought it is a programming error if *_UNSPEC is passed to the setter,
so I made it assertion failure. I updated the branch to throw an exception
instead of assert (git 76759ae).
> - "EDNS attributes" sounds too broad for what this method does.
> method name sounds misleading too (as if it returns an EDNS object
> or something). Same for setRequestEDNS0 and `RequestSigTSIG`.
> and `ResponseSigTSIG`.
> {{{#!cpp
> /// \brief Return EDNS attribute of the request.
> /// \return true if EDNS version of the request is 0
> /// \throw None
> bool getRequestEDNS0() const {
> return (bit_attributes_[REQ_IS_EDNS_0]);
> }
> }}}
> - `setRequestSig`: if it's specific to TSIG, I'd rename it
> `setRequestTSIG` or setRequestTSIGAttributes` or something.
> - `setResponseSigTSIG`: do we need the `is_tsig_signed` parameter?
>
> '''auth_srv.cc'''
>
> - These don't seem to be very accurate:
> {{{#!cpp
> stats_attrs.setRequestIPVersion(
> io_message.getRemoteEndpoint().getFamily() == AF_INET ?
> MessageAttributes::IP_VERSION_IPV4 :
> MessageAttributes::IP_VERSION_IPV6);
> stats_attrs.setRequestTransportProtocol(
> io_message.getRemoteEndpoint().getProtocol() == IPPROTO_UDP ?
> MessageAttributes::TRANSPORT_UDP :
> MessageAttributes::TRANSPORT_TCP);
> }}}
> in that, technically, there's no guarantee that it's AF_INET6 if
> it's not AF_INET. To be precise the logic should be something like:
> {{{#!cpp
> switch (getFamily()) {
> case AF_INET6:
> setIPversion(IPv6);
> break;
> case AF_INET:
> setIPversion(IPv4);
> break;
> default
> throw Unexpected;
> }
> }}}
> Also, I'm not sure if it makes much sense to do this in auth_srv.
> Even if it's enum it's not that much type safe (invalid value still
> can be passed via cast), and it looks like `AuthSrv` is handling what
> the statistics specific module should care about.
Is it OK to revert the change above and pass address family and transport
protocol directly to setRequestIPVersion and setRequestTransportProtocol?
> - processUpdate: why adding the parameter if you don't use it?
> {{{#!cpp
> AuthSrvImpl::processUpdate(const IOMessage& io_message,
> MessageAttributes&)
> }}}
Definitely the parameter is not used. I removed it (git 6445413).
> '''gen-statisticsitems.py'''
> - convert_list in generate_docfile: at least to me this is not
intuitive:
> {{{#!python
> if not prev is None:
> prev.tail = prev.tail.rstrip()
> }}}
> I'd suggest changing it to `if prev is not None`
OK, I changed to it (git b3804a1).
> - convert_list in generate_docfile: I don't understand the purpose of
> these changes:
> {{{#!diff
> - command.tail = ' '
> + command.tail = ''
> ...
> + sim_para.text = sim_para.text.rstrip()
> + if not prev is None:
> + prev.tail = prev.tail.rstrip()
> }}}
> please leave comments. Also, in general, making a single big commit
> like this one just saying "address review comment" is not good.
I added notes (git acc630c).
> - generate_cxx: (discussion)
>
> >> generate_cxx: if I understand it correctly, its convert_list()
doesn't work correctly only up to 1 recursion.
>
> > I think it should work. Anyway, if it doesn't work, unittest, systest
and/or lettuce fails.
>
> I just noticed my original text was broken, so rephrasing it:
> convert_list() doesn't work correctly with two or more recursive
> calls. If you still it should work, could you explain what kind of
> output is expected if the recursion at this point happens twice?
> {{{#!cpp
> msg_counter_types, item_names_current_, item_names = \
> convert_list(item['child'], msg_counter_types,
> item_names_current_, item_names)
> }}}
> as for the argument if it doesn't, as a last resort defense that's
> true (and I suspect in this case it wouldn't even compile), but, in
> general, errors should be caught at as early point as possible: we
> prefer test failure over run time surprise in a production
> environment; prefer compile-time error over test failure; and prefer
> code generator failure over compile error on the resulting code. It's
> not really good to leave it open if we are not sure if it works in
> this scenario.
>
> So, if we know it really works with any number of recursion, that's
> fine; otherwise, we should either fix it or explicitly fail on the
> unexpected cases. My current guess is it doesn't work, and in which
> case I'd just make the script fail explicitly.
If there are items in 2 or more depth, the recursion at the point happens
twice.
{{{
level1 stats_1stlevel test: 1st level of statistics =
foo STATS_TEST_1_FOO item foo level 1
level2 stats_2ndlevel test: 2nd level of
statsitics =
foo STATS_TEST_2_FOO item foo level 2
bar STATS_TEST_2_BAR item bar level 2
;
bar STATS_TEST_1_BAR item bar level 1
;
}}}
From the definition it will generate the following C++ codes:
{{{#!cpp
enum MSGCounterType {
STATS_TEST_1_FOO, ///< item foo level 1
STATS_TEST_2_FOO, ///< item foo level 2
STATS_TEST_2_BAR, ///< item bar level 2
STATS_TEST_1_BAR, ///< item bar level 1
// End of counter types
MSG_COUNTER_TYPES ///< The number of defined counters
};
const struct CounterSpec stats_2ndlevel[] = {
{ "foo", NULL, STATS_TEST_2_FOO },
{ "bar", NULL, STATS_TEST_2_BAR },
{ NULL, NULL, NOT_ITEM }
};
const struct CounterSpec stats_1stlevel[] = {
{ "foo", NULL, STATS_TEST_1_FOO },
{ "level2", stats_2ndlevel, NOT_ITEM },
{ "bar", NULL, STATS_TEST_1_BAR },
{ NULL, NULL, NOT_ITEM }
};
const struct CounterSpec msg_counter_tree[] = {
{ "level1", stats_1stlevel, NOT_ITEM },
{ NULL, NULL, NOT_ITEM }
};
}}}
For MSGCounterType, it should have all leaf nodes and it's as expected.
For CounterSpec, they should be a tree with all branch nodes and leaf
nodes. There should be a branch node "stats_1stlevel" which has 2 leaf
nodes "foo" and "bar" with corresponding MSGCounterType and a branch
node "level2". There should be a branch node "stats_2ndlevel" which has
2 leaf ndes "foo" and "bar" with corresponding MSGCounterType. They're
as expected.
> '''b10-auth.xml.pre'''
>
> - Can we now resolve this also?
> {{{
> <!-- TODO: missing stats docs. See ticket #1721 -->
> }}}
Yes it has been resolved (git 9d16915).
> '''statisticsitems.py'''
>
> - I'd rename it to, e.g., gen-statisticsitems_test.py in order to
clarify
> it's the tests for gen-statistics, not some helper module for other
> tests or tests for something other than gen-statistics.
OK, I renamed it to gen-statisticsitems_test.py (git ba18b3b).
> - per our name convention for Python `testXMLfile` would have to be
> named `test_xml_file`.
OK, I renamed it (git bd6e48f).
> '''auth_srv_unittest.cc'''
>
> - `queryCounterTruncTest`: it seems to use sqlite3 data source. If
> so, it should be skipped for static-link environments like this:
> {{{#!cpp
> TEST_F(AuthSrvTest,
> #ifdef USE_STATIC_LINK
> DISABLED_chQueryWithInMemoryClient
> #else
> chQueryWithInMemoryClient
> #endif
> )
> }}}
OK, updated as directed (git 98fd407).
> - By "It would also be good if we simplify and unify the verbose
> pattern seen in (e.g.) queryCounterUDPNormal." in my previous
> comment, I intended to introduce something like a unified
> comprehensive check like `checkCountersAreInitialized()`
> with parameterized exception lists to specify the counters that are
> expected to have non 0 values. The current revised version seems
> to not only "simplify" it but also actually "remove" some cases that
> were previously tested (confirming some counters are actually
> unchanged).
OK, I updated to confirm some counters are actually unchanged as tested
in lettuce test (git c7c5323).
> '''statistics_unittest.cc.pre'''
>
> - To guard EXPECT_DEATH, I suggest using EXPECT_DEATH_IF_SUPPORTED
> instead of `ifdef`
> - Regarding the previous point, check also the comment on the tested
> code; I thought assert() was too harsh in this case.
OK, death tests are removed as there's no assert().
> - result of bit operation should be directly used in a boolean
> context (http://bind10.isc.org/wiki/BIND9CodingGuidelines "Testing
> Bits"). I've fixed them myself.
OK, thank you.
> - naming this `af` is a bit awkward because it's not an `AF_xxx` value:
> {{{#!cpp
> const enum MessageAttributes::IPVersion af = (i & 1) != 0 ?
> MessageAttributes::IP_VERSION_IPV4 :
> MessageAttributes::IP_VERSION_IPV6;
> }}}
OK, I renamed it to ipversion (git 3eb8564).
Replying to [comment:44 jinmei]:
> Comments on the rest of the branch. This completes this round of
> my revised review.
>
> '''counter_dict.h'''
>
> - there are some unnecessary ';'s (also in counter.h). I removed them
> myself.
OK, thank you.
> Unrelated to this branch:
> - counter_dict.h should have more documentation.
> - (partly because of the lack of doc) I'm not sure about the intended
> usage of this file and `CounterDictionary`, but in general, we
> should use an unnamed namespace in a header file:
> {{{#!cpp
> namespace {
> typedef boost::shared_ptr<isc::statistics::Counter> CounterPtr;
> typedef std::map<std::string, CounterPtr> DictionaryMap;
> }
> }}}
>
> '''(lettuce) queries.feature'''
>
> It took quite a long time to complete this feature (almost 2 minutes
> in my environment), and despite the cost most test cases seem to be
> boring (repeating the same cases which are quite likely to succeed if
> others do). While the duration of the test is less critical in system
> tests, the cost-benefit balance with the current setup doesn't seem to
> be reasonable. Can't it be more sophisticated?
>
> For example, if we expect only a few counters to have non 0 values,
> I'd unify this condition into a single line of condition rather than
> repeating things like this:
> {{{
> Then the statistics counter v6 in the category request for the
zone _SERVER_ should be 0
> }}}
OK, I added a new terrain and it unifies a set of expected items
(git 9371828, git d4e61c2).
> '''bindctl/tests.sh'''
>
> If I understand it correctly, the statistics related part of this test
> is reasonably covered by lettuce tests like queries.feature
> (correct?). Since the bindctl test has another, non-statistics
> related issue, and is now disabled in master, I'd rather remove
> statistics related checks from it at this stage than tweaking it to
> make work with additional hacks. See also #2568.
Yes they are covered by lettuce tests. I won't update bindctl/tests.sh
as it is disabled in master. It will be disabled after merge.
Thanks,
--
Ticket URL: <http://bind10.isc.org/ticket/2157#comment:46>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list