Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1613 closed task (complete)

auth per rcode statistics

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20120221
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 9.25 Internal?: no

Description


Subtickets

Change History (16)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120221

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jelte

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

Ready for review, it is essentially a copy of the recently added per opcode code.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 follow-up: Changed 8 years ago by jinmei

general

(Just a comment) Code/tests/document for both opcode and rcode seem
quite resemble and may suggest unification. But considering this is
probably tentative experiment it's probably okay.

auth.spec.pre.in

  • 'NXRRSET' is missing within the parentheses?

"item_description": "The number of total responses with rcode 8 ()"

auth_srv.cc

  • Some cases are not counted, and some cases (where the response is not generated) are incorrectly counted. Former example is this:
        if (tsig_error != TSIGError::NOERROR()) {
            makeErrorMessage(message, buffer, tsig_error.toRcode(), tsig_context);
            server->resume(true);
            return;
        }
    
    and the latter:
            if (qtype == RRType::AXFR()) {
                send_answer = impl_->processXfrQuery(io_message, message, buffer,
                                                     tsig_context);
    
    (and when send_answer is set to false). I'd suggest some wrapper method that takes true/false (for resume or not) and the message, and does
    void
    AuthSrv::theWrapper(server, with_answer, message) {
        if (with_answer) {
           impl_->counters_.inc(message.getRcode());
        }
        server.resume(with_answer);
    
    Also, we need to have tests that would fail without these corrections.

statistics_unittest.cc

There are a couple of trivial typo due to (seeming) copy-paste. I've
fixed them directly.

others

I think we need a changelog entry for this.

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

general

(Just a comment) Code/tests/document for both opcode and rcode seem
quite resemble and may suggest unification. But considering this is
probably tentative experiment it's probably okay.

auth.spec.pre.in

  • 'NXRRSET' is missing within the parentheses?

"item_description": "The number of total responses with rcode 8 ()"

ack, added

auth_srv.cc

  • Some cases are not counted, and some cases (where the response is not generated) are incorrectly counted. Former example is this:

Ack, added a private method like your proposed wrapper.

Also, we need to have tests that would fail without these
corrections.

Naturally. I even added a way to test them more thoroughly than they were (also checks if for all counters are zero), perhaps we can do something similar for opcode, but we should probably wait on that possible refactor.

BTW, not all paths are covered; I don't think there is currently a way to trigger the Exception catchall (the one that causes SERVFAIL after fromWire on auth_srv.cc:431). I tried to add a wiredata-mangling feature to our test system but then i discovered that all the actual parsing code at this point can only throw ProtocolErrors?. Oh well.

statistics_unittest.cc

There are a couple of trivial typo due to (seeming) copy-paste. I've
fixed them directly.

Thanks

others

I think we need a changelog entry for this.

Oh right, of course;

XXX. [func] jelte
b10-auth now also experimentally supports statistics counters of the rcode reponses it sends. The counters can be shown as rcode.<code name>, where code name is the lowercase textual representation of the rcode (e.g. "noerror", "formerr", etc.).
Same note applies as for opcodes, see changelog entry 364.

Speaking of which, we should probably also have a ticket that allows you to do 'Stats show Auth opcode' (i.e. only show opcode, but all of them), and perhaps even a more general 'don't show zeroes'. Also looking forward to discussing more general ways to do statistics now, btw :)

comment:10 in reply to: ↑ 9 Changed 8 years ago by jinmei

Replying to jelte:

The revised version basically looks okay. Some final comments below.
Any possible changes due to those comments will be very minor, so
please feel free to merge it once you complete it.

auth_srv.cc

  • Some cases are not counted, and some cases (where the response is not generated) are incorrectly counted. Former example is this:

Ack, added a private method like your proposed wrapper.

Maybe we'd implement it within the impl class to make sure it's
inlined since this helper method is called for almost all requests and
may be performance sensitive. But maybe we can simply trust the
compiler, so I'd leave it to you.

Also, we need to have tests that would fail without these
corrections.

Naturally. I even added a way to test them more thoroughly than they were (also checks if for all counters are zero), perhaps we can do something similar for opcode, but we should probably wait on that possible refactor.

BTW, not all paths are covered; I don't think there is currently a way to trigger the Exception catchall (the one that causes SERVFAIL after fromWire on auth_srv.cc:431). I tried to add a wiredata-mangling feature to our test system but then i discovered that all the actual parsing code at this point can only throw ProtocolErrors?. Oh well.

Mangled wire data are actually expected to result in FORMERR, so
that's natural. I guess if you make some kind of short buffer data in
RDATA that triggers an exception in the input buffer class, then
you'll see other types of exception. But actually I think we should
convert such cases to FORMERR (I should probably create a ticket), so
trying to add a test for that case may not make sense anyway.

We could even introduce a faked RR (RDATA) class and register it in
the rrparamregistry (whose "from wire" constructor would throw
isc::Unexpected or something) and have the auth server parse a message
containing it, but that's probably too much for the purpose of this
test.

So, in conclusion, I'm okay with the current set of tests.

others

Speaking of which, we should probably also have a ticket that allows you to do 'Stats show Auth opcode' (i.e. only show opcode, but all of them), and perhaps even a more general 'don't show zeroes'. Also looking forward to discussing more general ways to do statistics now, btw :)

At least the first one seems to make sense.

Some other minor points in the code:

  • in srv_test.cc, why did you change the indentation here?
     void
     SrvTestBase::createRequestPacket(Message& message,
    -                                 const int protocol, TSIGContext* context)
    +                                    const int protocol, TSIGContext* context)
     {
    
    The original indentation seems correct (per guideline) to me.
  • I made a few test methods const member functions.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 follow-up: Changed 8 years ago by jinmei

Oh, and one more minor point: I guess we'd eventually move the rcode
stat check to the common srv_test:

TEST_F(AuthSrvTest, ednsBadVers) {
    ednsBadVers();
    checkAllRcodeCountersZeroExcept(Rcode::BADVERS(), 1);
}

but right now b10-resolver doesn't support statistic in the first
place, so it's okay to place them in the auth tests.

comment:13 Changed 8 years ago by jelte

Replying to jinmei:

Replying to jelte:

The revised version basically looks okay. Some final comments below.
Any possible changes due to those comments will be very minor, so
please feel free to merge it once you complete it.

Ack, added a private method like your proposed wrapper.

Maybe we'd implement it within the impl class to make sure it's
inlined since this helper method is called for almost all requests and
may be performance sensitive. But maybe we can simply trust the
compiler, so I'd leave it to you.

oh, good idea, done.

Also, we need to have tests that would fail without these
corrections.

Naturally. I even added a way to test them more thoroughly than they were (also checks if for all counters are zero), perhaps we can do something similar for opcode, but we should probably wait on that possible refactor.

BTW, not all paths are covered; I don't think there is currently a way to trigger the Exception catchall (the one that causes SERVFAIL after fromWire on auth_srv.cc:431). I tried to add a wiredata-mangling feature to our test system but then i discovered that all the actual parsing code at this point can only throw ProtocolErrors?. Oh well.

Mangled wire data are actually expected to result in FORMERR, so
that's natural. I guess if you make some kind of short buffer data in
RDATA that triggers an exception in the input buffer class, then
you'll see other types of exception. But actually I think we should
convert such cases to FORMERR (I should probably create a ticket), so
trying to add a test for that case may not make sense anyway.

given the way it works now, I can't see a way to get the servfail other than a bug in our code (in which case servfail is fine imo). If we didn't already parse the header earlier, a 10-octet packet might trigger it, but we do parse the header earlier.

We could even introduce a faked RR (RDATA) class and register it in
the rrparamregistry (whose "from wire" constructor would throw
isc::Unexpected or something) and have the auth server parse a message
containing it, but that's probably too much for the purpose of this
test.

yes, i think that would be overdoing it a bit :) (the only purpose imo would be to get more code coverage, really)

So, in conclusion, I'm okay with the current set of tests.

others

Speaking of which, we should probably also have a ticket that allows you to do 'Stats show Auth opcode' (i.e. only show opcode, but all of them), and perhaps even a more general 'don't show zeroes'. Also looking forward to discussing more general ways to do statistics now, btw :)

At least the first one seems to make sense.

my reasoning behind the second one was that, for instance, if you don't run ddns, a lot of rcodes are irrelevant (and should be 0 anyway), so you may not be interested in them. But maybe there are better ways to achieve that. Either way, IMO the list of items is already getting too large to usefully do 'show them all' on the command line.

Some other minor points in the code:

  • in srv_test.cc, why did you change the indentation here?
     void
     SrvTestBase::createRequestPacket(Message& message,
    -                                 const int protocol, TSIGContext* context)
    +                                    const int protocol, TSIGContext* context)
     {
    
    The original indentation seems correct (per guideline) to me.

oh, sorry, artifact of changing it to use the mangler, and not reverting it completely. Fixed.

  • I made a few test methods const member functions.

thanks.

comment:14 in reply to: ↑ 12 Changed 8 years ago by jelte

Replying to jinmei:

Oh, and one more minor point: I guess we'd eventually move the rcode
stat check to the common srv_test:

TEST_F(AuthSrvTest, ednsBadVers) {
    ednsBadVers();
    checkAllRcodeCountersZeroExcept(Rcode::BADVERS(), 1);
}

but right now b10-resolver doesn't support statistic in the first
place, so it's okay to place them in the auth tests.

Yeah I would've already done so if that had not been the case, so i agree :)

comment:15 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 8

comment:16 Changed 8 years ago by jinmei

  • Total Hours changed from 8 to 9.25
Note: See TracTickets for help on using tickets.