Opened 7 years ago

Closed 7 years ago

#2884 closed defect (fixed)

per zone statistics must be separated per class

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

Description

See the tables in #2158 or #2252. Items like

xfrin.[zonename].soaoutv4

should actually be

xfrin.[classname].[zonename].soaoutv4

and code should be updated accordingly.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 8

comment:2 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed

comment:3 Changed 7 years ago by naokikambe

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

comment:4 Changed 7 years ago by naokikambe

  • Owner set to naokikambe
  • Status changed from new to accepted

picking

comment:5 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed
  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

Can be reviewed now. Here is a proposed changelog entry:

nnn.    [bug]*          naokikambe
        Per-zone statistics counters are distinguished by zone class, e.g. IN,
        CH, or HS. A class name is added onto a zone name in the structure of
        per-zone statistics.
        (Trac #2884, git TBD)

comment:6 Changed 7 years ago by muks

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

comment:7 Changed 7 years ago by muks

This ticket's review was estimated at 3 points.

comment:8 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review.

comment:9 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe-san :)

Here is my review.

  • In item_title and item_description, I suggest you use "RR class name" as it's more indicative. Otherwise people may assume wrong kind of class. Or you can use the text in the manpage ("Class name of the zone, e.g. IN, CH, and HS").
  • Is there any validation done for the RR class, i.e., that it must be in ['IN', 'CH', etc.]?
  • Please rewrite the following code in get_statistics() in counters.py:
                        id_str = '%s/%%s/%s' % (cls, attr)
                        id_str1 = id_str % zone
                        id_str2 = id_str % self._entire_server
    

to something like:

                    id_str1 = '%s/%s/%s' % (cls, zone, attr)
                    id_str2 = '%s/%s/%s' % (cls, self._entire_server, attr)

As a coding style, we should be very careful not to construct format
strings from other input. They could include formatting of their own.

In a language like C, it's usually bad practise to use non-static format
strings; we can sometimes construct it with lengths and such, but it's
very bad to copy input string data into it. I doubt this can be a
security issue in Python, but it might lead to bugs if a % is somehow
introduced. The updated code above is easier to follow too.

  • In xfrout_test.py.in, it may be worth adding a TEST_RRCLASS_STR at the top to avoid the many to_text() calls.
  • I suggest some more code comment at the top of _add_counter() (maybe with an example identifier) so it is easily readable, esp. for the code following the statement:
        if has_named_set:
    

I follow what the code is doing now, but it took me 5 minutes for
running the code in my head. :)

  • The rest of the branch looks ok.
  • The ChangeLog entry looks ok. make check and lettuce pass on it. There are no Makefile.am changes, so I have not run make distcheck on it.

comment:10 in reply to: ↑ 9 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund-san,

Thank you for reviewing!

  • In item_title and item_description, I suggest you use "RR class name" as it's more indicative.

I added "RR". Please see

6d1f4e1 [2884] s/class name/RR class name/ig
  • Is there any validation done for the RR class, i.e., that it must be in ['IN', 'CH', etc.]?

No. If we want to restrict by the known names, e.g. IN, CH, and HS, we can choose a map type as another option. But in the case all of them are always shown even if any of them are never counted. So I chose the named_set by assuming another new class name would be added in future.

  • Please rewrite the following code in get_statistics() in counters.py:

...

                    id_str1 = '%s/%s/%s' % (cls, zone, attr)
                    id_str2 = '%s/%s/%s' % (cls, self._entire_server, attr)

Thank you for consideration. I applied. Please see

8288bf4 [2884] do not reuse the string of format
  • In xfrout_test.py.in, it may be worth adding a TEST_RRCLASS_STR at the top to avoid the many to_text() calls.

I replaced it. Please see

fcea4de [2884] s/TEST_RRCLASS.to_text()/TEST_RRCLASS_STR/g
  • I suggest some more code comment at the top of _add_counter() (maybe with an example identifier) so it is easily readable,

...

I follow what the code is doing now, but it took me 5 minutes for
running the code in my head. :)

Sorry for inconvenience. :( I added a note including an example. Please see

ca3f4ec [2884] add a note for _add_counter()

Regards,

comment:11 Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe-san

This looks fine now. Please go ahead and merge. :)

comment:12 Changed 7 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 17.5

Hi Mukund-san,

Thanks:) The branch has been merged. This is closing.

Note: See TracTickets for help on using tickets.