[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-dev] Relay Dashboard Code Review



Hi Cristobal, again apologies for the delay in getting you feedback. Just gave
RWSD a whirl and worked great - nice work!

From a UI perspective only feedback on what we have so far is...

  * Both graphs just have the title of 'Graph component'. Better would be to
    say if it's inboud or outbound.

  * Graph's y-axis should indicate its units.

  * Personally I kinda like Vidalia's line graph better than bar graphs for
    this, but obviously just something to experiment with. Lots of options.

    https://upload.wikimedia.org/wikipedia/commons/e/ef/Tor-non-exit-relay-bandwidth-usage.jpg

  * Only having fifteen seconds of information is pretty short. Actually, if
    you raise it to something high (like several minutes of data) then you
    might want to consider prepopulating bandwidth information from before
    RWSD started. Here's how Nyx does it...

    https://gitweb.torproject.org/nyx.git/tree/nyx/graph_panel.py#n244


Unfortunately I'm still a bit of an Angular noob so I can't comment much on
those bits. Arturo: do you have any thoughts?

Otherwise, here's some feedback on the code!


============================================================
run_client.py
============================================================

 23         settings = dict(
 24             template_path=os.path.join(os.path.dirname(__file__),
"client/templates"),
 25             static_path=os.path.join(os.path.dirname(__file__),
"client/static"),
 26             autoescape=None,
 27         )
 28         handlers = [
 29             (r"/", MainHandler),
 30         ]
 31         cyclone.web.Application.__init__(self, handlers, **settings)

Very minor thing, but little odd using dict() that way. This could also be...

  handlers = [
    (r"/", MainHandler),
  ]

  cyclone.web.Application.__init__(self, handlers, {
    template_path: os.path.join(os.path.dirname(__file__), "client/templates"),
    static_path: os.path.join(os.path.dirname(__file__), "client/static"),
    autoescape: None,
  })

Again, minor and up to you. run_server.py has the same. This is similar to
what I do all throughout my codebases...

  https://gitweb.torproject.org/nyx.git/tree/nyx/graph_panel.py#n71

------------------------------------------------------------

 37     reactor.listenTCP(int(config.get('client.port')), Application())

Actually, you don't need the int() if you provide a default. If you call...

  config.get('client.port', 9051)

... then the config will give you an int, and emit a warning if the config
has something else...

  https://stem.torproject.org/api/util/conf.html#stem.util.conf.Config.get

Same in run_server.py.

============================================================
run_server.py
============================================================

 35         if(dual_mode()):

Good idea on this config option. In python you don't use parentheses for
conditionals, unless you're doing some grouping.

Speaking of which, might I suggest baking pyflakes and pep8 checks into your
project? Even if you don't have unit tests yet these can speed development
by catching minor issues, and help the codebase have a more uniform style.

Stem provides a couple utilities that should make this easy. Here's an
example of using them...

  https://gitweb.torproject.org/nyx.git/tree/run_tests.py

Oh, and it'll catch the trailing whitespaces that's slipped in. ;)

  % grep -R ' $' rwsd/* | grep -v 'Binary file' | wc -l
  17

------------------------------------------------------------

 45 @uses_settings
 46 def main(config, dual = True):

The 'dual = True' argument is unused. You use the dual_mode() util function
instead.

------------------------------------------------------------

 87         except:
 88             log.msg("unable to attach listeners, controller off")
 89     except:
 90         log.msg("unable to start tor controller from rwsd")

_conn_listener() is a neat approach. I like it.

Generally it's good idea to both only catch the exception types you need, and
include the exception in the message so there's some hint for troubleshooting.
The second, for instance, is in effect just calling stem.connection.connect(),
which only raises ValueErrors (for invalid arguments)...

  https://stem.torproject.org/api/connection.html#stem.connection.connect

Seeing how you use it here you probably want Controller.from_port() instead.
As we discussed earlier connect() prints errors to stdout and returns None
if it can't connect. However, you're not checking if it returns None so
the second try/catch will hit an AttributeError.

I suspect there *might* be a bug around double attaching listeners when tor is
connected then reconnected, but not sure.

============================================================
util/__init__.py
============================================================

 49 def msg(message, config, **attr):

Unused, but feel free to leave it alone if you plan on using it later.

============================================================
server/handlers/main.py
============================================================

Your run_client.py defines a MainHandler but then it seems you have an unused
copy of it here too. Since run_server.py uses it too maybe drop it from
run_client.py and use this instead?

============================================================
server/handlers/status.py
============================================================

 14 def _handle_status_event(controller, state, timestamp):
 15     if state == State.CLOSED:
 16         output = {'status': 'off'}
 17     else:
 18         output = {'status': 'on'}
 19
 20     send_data(WebSocketType.STATUS, output)

Certainly fine, but can condense this if you'd like.

  def _handle_status_event(controller, state, timestamp):
    send_data(WebSocketType.STATUS, {'status': 'off' if state ==
State.CLOSED else 'on'})

============================================================
server/handlers/graph.py
============================================================

 24     if read_total and write_total:
 25         output.update({'read_total': read_total})
 26         output.update({'write_total': write_total})

No need to call update() for single values. This is the same as...

  if read_total and write_total:
    output.update['read_total'] = read_total
    output.update['write_total'] = write_total

Same for more calls below.

============================================================
server/handlers/base.py
============================================================

 33 def init_websockets():
 34     global WEBSOCKETS
 35     WEBSOCKETS = {}
 36     for ws_type in WebSocketType:
 37         WEBSOCKETS.update({ws_type: []})

Could also be...

  def init_websockets():
    global WEBSOCKETS
    WEBSOCKETS = dict([(ws_type, []) for ws_type in WebSocketType])

------------------------------------------------------------

 40 def get_websockets(ws_type = None):
 41     global WEBSOCKETS
 42     if ws_type:
 43         if WEBSOCKETS[ws_type]:
 44             return WEBSOCKETS[ws_type]
 45         else:
 46             return None
 47     else:
 48         return WEBSOCKETS

The inner conditional made me pause for a second until I realized it's to
convert empty lists to None. Are you familiar with dictionary's get() method?
It allows you to specify a default, and might allow this to be improved a
bit.

  def get_websockets(ws_type = None):
    websocket = WEBSOCKETS.get(ws_type, [])
    return websocket if websocket else None

Also, you only need the 'global' keyword if you're doing assignment. Your
init_websockets() is the only function that needs it.

============================================================
client/static/css/*
client/static/font/*
============================================================

Perfectly fine, but our readme should say we're bundling a copy of angular,
bootstrap, and awesome. Also, please note their licenses.

I assume bundling is the usual approach for this?

============================================================
client/static/img/body-bg.png
============================================================

Is this just a solid color? If so, any reason it's an image rather than a css
background color property?

============================================================
client/static/js/*
============================================================

Please separate our custom javascript from bundled modules. This way it's
clear what is ours verses a copy.

============================================================
client/static/js/bandwidthGraph.controller.js
============================================================

  9 function bandwidthGraph($scope, bandwidthWebsocket) {
 10     activate();
 11     $scope.readData.data = bandwidthWebsocket.read_data;
 12     $scope.writtenData.data = bandwidthWebsocket.written_data;

Ahh, so we're only using read_data and written_data. Our bandwidth handler is
doing a bit of extra work each second to fetch additional data from tor. Fine
for now, but we should later drop those GETINFO calls if they remain unused.

Is the activate() getting called on every bandwidth event? On first glance
looks like $scope.readData and $scope.writtenData are getting reset each time.

============================================================
client/templates/index.html
============================================================

 10 <link href="http://fonts.googleapis.com/css?family=Open+Sans:400italic,600italic,400,600";
rel="stylesheet">
 ...
 15 <!-- Le HTML5 shim, for IE6-8 support of HTML5 elements -->
 16 <!--[if lt IE 9]>
 17       <script
src="http://html5shim.googlecode.com/svn/trunk/html5.js";></script>
 18     <![endif]-->

Fine for now, but cross domain fetches to google (googleapis.com and
googlecode.com) are likely a no-go for our final version. Also, it might
also be unwise to link to Google Code since it's being phased out...

  https://code.google.com/p/support/wiki/ReadOnlyTransition

Doesn't matter in the short term (being read-only is, of course, fine)
but sounds like that may go away eventually.

------------------------------------------------------------

 19 <style>
 20     .chart {
 21         width: 100%;
 22         height: 300px;
 23     }
 24 </style>

Should we start our own application.css in static for things like this? No
doubt this'll just be the first of many.

------------------------------------------------------------

 68 <div class="main">
 69   <div class="main-inner">
 70     <div class="container">
 71       <div class="row" ng-controller="bandwidthGraph">
 ...

Angular supports templates which store html snippets. Essentially partials.
Maybe that would help break this up? It would be nice if our base index.html
was dead simple, and pulled in modular components which do the actual work.
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev