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-bandw...
* 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.