Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#917 closed enhancement (fixed)

update stats-httpd to return only specified statistics

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

Description

The current stats-httpd returns all statistics but if statistics items are increasing, there may be an impact due to the large transmitted bytes. So update stats-httpd to return only specified statistics. There is the original comment in WeeklyMinutes20110510.

Subtickets

Change History (26)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 8 years ago by shane

  • Owner changed from naokikambe to UnAssigned
  • Status changed from new to assigned

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Sprint-20111025

comment:4 Changed 8 years ago by naokikambe

  • Estimated Difficulty changed from 0.0 to 6
  • Owner changed from UnAssigned to naokikambe
  • Status changed from assigned to accepted

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:6 Changed 8 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

Ready for reviewing.

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

/bind10/statistics/xml displays fine.

But it links to more specific pages which result in showing no data (but does have a header and empty table with headers). But the data is there:

<bind10:statistics xmlns:bind10="http://bind10.isc.org/bind10" xmlns:xsi="http://www.w3.org
/2001/XMLSchema-instance" xsi:schemaLocation="http://bind10.isc.org/bind10 /bind10/statistics/xsd">
<boot_time>2011-10-25T00:50:10Z</boot_time></bind10:statistics>

(I added newlines to show above.)

Also notice that the above has two http://bind10.isc.org URIs. (One has a space it also.) I think they should point to my local (relative) server.

comment:8 Changed 8 years ago by jreed

Nevermind on that space in schemaLocation -- I guess that is correct to list multiple.

Any ideas on why the data above is not display in browser?

On my laptop the address is http://192.168.1.2:8000/bind10/statistics/xml/Boss/boot_time

comment:9 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to naokikambe

Hello

I see the same as Jeremy. I don't know why, but my browser shows only the headers of the table, but no context. It is OK in the overview page that shows everything.

I fixed a small problem in the changelog text (and pushed), I hope you don't mind.

The test fail for me, maybe you'll know more about why this might happen:

======================================================================
FAIL: test_do_GET (__main__.TestHttpHandler)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-httpd_test.py", line 349, in test_do_GET
    check_XSL_URL_PATH(mod=None, item=None)
  File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-httpd_test.py", line 300, in check_XSL_URL_PATH
    self.assertTrue(k in itm_vo)
AssertionError: False is not true

Also, I have some less important notes about the code itself:

  • In this code, it looks unclean to decide by the text of exception:
    # Couldn't find neither specified module name nor
    # specified item name
    if str(err).startswith('Stats module: specified arguments are incorrect:'):
        self.send_error(404)
    else:
        self.send_error(500)
    
    Would it be possible to throw inherited exception in that case?
  • This is quite obvious. However, it might be more helpful to note what the parameters mean/do.
    the data which obtains from it. args are owner and name."""
    
  • The closures (get_stats_data, get_stats_spec, …) don't really have a helpful docstring. It doesn't say what the functions actually do and that the last parameter is where the result is stored.
  • The closures ‒ it is not necessary to explicitly return None, if there's no return, the function returns None by itself.
  • Is it really OK to do this? Can't the conversion to us-ascii lose some characters? Can't there be non-ascii characters in the XML?
    xml_string = str(xml.etree.ElementTree.tostring(xml_elem, encoding='utf-8'),
                     encoding='us-ascii')
    
  • The recursive closures detect where they are in the structure by the type of parameter. Wouldn't it be better to write a separate function for each of the cases? Because we should be sure what type there is when we call it recursively.
  • The test commented as # 404 NotFound (nonexistent item name) ‒ shouldn't it be tested on some module that exists? Because the module is nonexistent too.
  • Shouldn't assertEqual be used here?
    if item['item_type'] == 'list':
       self.assertTrue(len(item) == 7)
    else:
       self.assertTrue(len(item) == 6)
    

With regards

comment:11 in reply to: ↑ 7 Changed 8 years ago by naokikambe

Replying to jreed:

But it links to more specific pages which result in showing no data (but does have a header and empty table with headers). But the data is there:

It didn't change the XSL URI in the doctype of the XML document when
the module name was specified or the module name and the item name were
specified in the URI. That was a bug. I already fixed it at git
7e96227163334ecd54e506bd2cedb58d3f6cf91d. Please see it.

Also notice that the above has two http://bind10.isc.org URIs. (One has a space it also.) I think they should point to my local (relative) server.

In my understating, there is no problem. The former previous of the
space is the namespace, the latter is the absolute path name in the
server.

Regards,

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned

Hello,
Replying to vorner:

I fixed a small problem in the changelog text (and pushed), I hope you don't mind.

Thank you very much.

The test fail for me, maybe you'll know more about why this might happen:

======================================================================
FAIL: test_do_GET (__main__.TestHttpHandler)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-httpd_test.py", line 349, in test_do_GET
    check_XSL_URL_PATH(mod=None, item=None)
  File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-httpd_test.py", line 300, in check_XSL_URL_PATH
    self.assertTrue(k in itm_vo)
AssertionError: False is not true

The path names of the XSL/XHTML structure in the ElementTree obejct
were incorrect. The path name ended with the unnecessary slash. I
already fixed them at git f8a64959bc5f3ddf68ba4d01bee092bf4f1f9558. Please see it.

  • In this code, it looks unclean to decide by the text of exception:
    # Couldn't find neither specified module name nor
    # specified item name
    if str(err).startswith('Stats module: specified arguments are incorrect:'):
        self.send_error(404)
    else:
        self.send_error(500)
    
    Would it be possible to throw inherited exception in that case?

I added a new exception StatsHttpdDataError. It is raised when an
error due to the specified data is occurred. If it's raised,
stats-httpd will return 404 notfound. Please see git df02b63fe1176c572a7eee996921f211ca970953.

  • This is quite obvious. However, it might be more helpful to note what the parameters mean/do.
    the data which obtains from it. args are owner and name."""
    
  • The closures (get_stats_data, get_stats_spec, …) don't really have a helpful docstring. It doesn't say what the functions actually do and that the last parameter is where the result is stored.

I added the more docstrings. Please see git 8e8607c6faa34d9493a831054ecb64281f1f06c7 and git
4ab7d17edc10ce4f7b834709aa009aba4db9d877. If they are not enough, please let me know.

  • The closures - it is not necessary to explicitly return None, if there's no return, the function returns None by itself.

I removed such return Nones. Please see git 50e96053742a30584f91a6bdb4b788977cd166bf.

  • Is it really OK to do this? Can't the conversion to us-ascii lose some characters? Can't there be non-ascii characters in the XML?
    xml_string = str(xml.etree.ElementTree.tostring(xml_elem, encoding='utf-8'),
                     encoding='us-ascii')
    

Yes, it loses non-ascii characters. That was added in #1021 but that
might be a work around. However I think that such non-ascii characters
wouldn't be acceptable in the system. If we make that acceptable, we
have to totally review the specification of the whole system. I know
such non-ascii characters are urgent into the future, but do we need
it now?

  • The recursive closures detect where they are in the structure by the type of parameter. Wouldn't it be better to write a separate function for each of the cases? Because we should be sure what type there is when we call it recursively.

Such internal functions are temporary in this ticket. I'd like to
this task would be bigger. Sorry, please give me any suggestions if
you have.

  • The test commented as # 404 NotFound (nonexistent item name) - shouldn't it be tested on some module that exists? Because the module is nonexistent too.

I added such testcases. Please see git cc48074a9fec60ef9ba69991549f9e167e620225.

  • Shouldn't assertEqual be used here?
    if item['item_type'] == 'list':
       self.assertTrue(len(item) == 7)
    else:
       self.assertTrue(len(item) == 6)
    

I fixed it. Please see git f1e08d75cabc45454a9bde86158dc8c7348d7f9d.

Best

comment:13 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:14 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to naokikambe

Hello

I can confirm the statistics are shown now.

Replying to naokikambe:

Replying to vorner:

  • In this code, it looks unclean to decide by the text of exception:
    # Couldn't find neither specified module name nor
    # specified item name
    if str(err).startswith('Stats module: specified arguments are incorrect:'):
        self.send_error(404)
    else:
        self.send_error(500)
    
    Would it be possible to throw inherited exception in that case?

I added a new exception StatsHttpdDataError. It is raised when an
error due to the specified data is occurred. If it's raised,
stats-httpd will return 404 notfound. Please see git df02b63fe1176c572a7eee996921f211ca970953.

This introduces one new problem: the STATHTTPD_SERVER_ERROR log message is used twice, which shouldn't happen. Could you create a new one for the 404 error?

  • Is it really OK to do this? Can't the conversion to us-ascii lose some characters? Can't there be non-ascii characters in the XML?
    xml_string = str(xml.etree.ElementTree.tostring(xml_elem, encoding='utf-8'),
                     encoding='us-ascii')
    

Yes, it loses non-ascii characters. That was added in #1021 but that
might be a work around. However I think that such non-ascii characters
wouldn't be acceptable in the system. If we make that acceptable, we
have to totally review the specification of the whole system. I know
such non-ascii characters are urgent into the future, but do we need
it now?

I guess it's not needed now. It just looked suspicious, so I pointed it out. If there are no non-ascii characters expected for now, it should work.

  • The recursive closures detect where they are in the structure by the type of parameter. Wouldn't it be better to write a separate function for each of the cases? Because we should be sure what type there is when we call it recursively.

Such internal functions are temporary in this ticket. I'd like to
this task would be bigger. Sorry, please give me any suggestions if
you have.

It's probably OK if they are temporary.

Thank you

comment:15 follow-up: Changed 8 years ago by jreed

Thanks. It works now. I do get specific URIs per item.

More comments:

1) The first letter of a item is capitalized so no longer matches up with original name nor name in the URI. Maybe should not be capitalized.

2) The final URI still has a hyperlink for itself on the item name. Maybe no hyperlink is needed to itself?

3) On specific item, it has plural "Names"; maybe should be "Name" (no "s").

4) Maybe the specific item XML should contain the full name (or path?) to the specific item? The URI does have it, but I think the XML should also. For example, what component is this for <bind10:statistics ...><boot_time>2011-10-25T00:50:10Z</boot_time></bind10:statistics> ? Or is looking at the xsi:schemaLocation the correct way to identify an item?

5) If we do decide to list all the hierarchy to a specific item (such as foo > bar > baz > item), then it would be nice if the stylesheet made it so each level had an hyperlink to that level.

6) Is the copyright and permission statement needed for the XML data? I don't think we should copyright the generated data.

This is very useful! Thank you for this.

comment:16 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:17 in reply to: ↑ 14 Changed 8 years ago by naokikambe

Hello,

Replying to vorner:

This introduces one new problem: the STATHTTPD_SERVER_ERROR log message is used twice, which shouldn't happen. Could you create a new one for the 404 error?

I added a new logging id for it. Please see ebe4e57805eda25ca347e0a9db8adad11fb3d4b5.

I guess it's not needed now. It just looked suspicious, so I pointed it out. If there are no non-ascii characters expected for now, it should work.

I added FIXMEs for it. Please see git ab47b771999bd12171e65a8a3fb2ee512b709c4b.

It's probably OK if they are temporary.

I added TODOs for it. Please see git 2139076757c1a14ecce96eafd1388f978732f8aa.

Thank you

comment:18 in reply to: ↑ 15 Changed 8 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned

Replying to jreed:

Thanks. It works now. I do get specific URIs per item.

You're welcome and thank you for reviewing.

1) The first letter of a item is capitalized so no longer matches up with original name nor name in the URI. Maybe should not be capitalized.

The capitalized item name displayed on the XSL and the item name in the part of URI aren't exactly same. The former is defined in each spec file. We can know the original item name in the hyperlink.

2) The final URI still has a hyperlink for itself on the item name. Maybe no hyperlink is needed to itself?

Sorry, I cannot do that in this ticket. I added a FIXME for that. Please see 7491d9ecb3ff7913a68f8db454374960a2f34330.

3) On specific item, it has plural "Names"; maybe should be "Name" (no "s").

I removed that. Please see git de07e6a0ab66de4d3c7720dc93bc7d9198c9d26b.

4) Maybe the specific item XML should contain the full name (or path?) to the specific item? The URI does have it, but I think the XML should also. For example, what component is this for <bind10:statistics ...><boot_time>2011-10-25T00:50:10Z</boot_time></bind10:statistics> ? Or is looking at the xsi:schemaLocation the correct way to identify an item?

It also displays the module name and the item name in each level. Please see git 63f04832f2604868133a23d110ce6df5a9707993.

5) If we do decide to list all the hierarchy to a specific item (such as foo > bar > baz > item), then it would be nice if the stylesheet made it so each level had an hyperlink to that level.

Sorry, I cannot do that in this ticket. It can be added until the level of the first item name. I added a FIXME for that. Please see 7491d9ecb3ff7913a68f8db454374960a2f34330.

6) Is the copyright and permission statement needed for the XML data? I don't think we should copyright the generated data.

I removed that. Please see git 489a53541118413b38865c8a3cf84b24b8b7dfe2.

Thanks,

comment:19 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:20 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to naokikambe

Hello.

The code looks OK. I'm not sure if Jeremy wants to have a look too, though.

Anyway, two questions, more out of curiosity than of anything else:

  • The 88147da513fdb22eb4e430390746f36c96304c7e (evaluation of the requested URI path) ‒ it is OK and probably more correct than the previous version. But what led you to reconsider/change it?
  • The things you say can't be done in this ticket ‒ is it because the size of the ticket, or is there some other reason?

Anyway, it looks OK to merge, so unless you wan't to add some more changes, feel free to close it.

Thank you

comment:21 in reply to: ↑ 20 Changed 8 years ago by naokikambe

Replying to vorner:

Thank you for the final comments.

  • The 88147da513fdb22eb4e430390746f36c96304c7e (evaluation of the requested URI path) ‒ it is OK and probably more correct than the previous version. But what led you to reconsider/change it?

Sorry, that was directly related to neither your comment nor Jeremy's. That was what I found unexpectedly.

  • The things you say can't be done in this ticket ‒ is it because the size of the ticket, or is there some other reason?

It's due to the task size. A ticket is too small to apply any requirements.

I try to wait for Jeremy's approval.

Thanks,

comment:22 Changed 8 years ago by naokikambe

  • Owner changed from naokikambe to jreed

comment:23 follow-up: Changed 8 years ago by jreed

  • Owner changed from jreed to naokikambe

I know this ticket title is for "stats-httpd" but how to query for specific stats from bindctl?

Also I still think the item name should not be capitalized -- I think it should be identical unless the stats and stats-httpd worked with case-insensitive (it does not now).

Anyways, those issues and others can be dealt with later.

Thanks for this work. You may marge and close ticket. Let us open new tickets for any other situations related to this. Also I will work on some documentation if needed.

comment:24 in reply to: ↑ 23 Changed 8 years ago by naokikambe

Replying to jreed:

I know this ticket title is for "stats-httpd" but how to query for specific stats from bindctl?

Querying to stats for the specific item by bindctl was already
implemented in #1175(formerly #930). stats-httpd just uses this
interface. Sorry for the confusion.

> Stats show owner=Boss name=boot_time
"2011-11-16T01:27:52Z"
>

Also I still think the item name should not be capitalized -- I think it should be identical unless the stats and stats-httpd worked with case-insensitive (it does not now).

Sorry for my poor explanation.
The capitalized name is used mainly in xsl for the user
view. OTOH the identical name (not capitalized) is used in any
other places including xml, stats, stats-httpd and so on. I don't
think the identical name is always user-friendly. For example, we
cannot put whitespaces in the identical name. However it might not
be so important. Eventually we can easily make the capitalized names
same as the identical names. In order to do that, we just rename the
value of item_title to the same value of item_name in each spec
file. Anyway I'll lay this issue as it is.

Anyways, those issues and others can be dealt with later.
Thanks for this work. You may marge and close ticket. Let us open new tickets for any other situations related to this. Also I will work on some documentation if needed.

I put TODO and FIXME tags in the codes for those issues.
I'll merge later. Thank you for documentation in advance.

comment:25 Changed 8 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed

I've merged the branch and I'm closing the ticket.

comment:26 Changed 8 years ago by naokikambe

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