[tor-dev] Relay Dashboard Code Review
Damian Johnson
atagar at torproject.org
Wed Aug 5 16:53:01 UTC 2015
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.
More information about the tor-dev
mailing list