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.
Damian Johnson <atagar@torproject.org> writes:
Very minor thing, but little odd using dict() that way.
Personally, I don't find it that odd ;) and prefer it since braces can define sets or dicts in Python, and using dict(...) makes it a little more explicit. BTW, your counter-example is a syntax error; you need to quote the keys, which is another reason I prefer the kwarg-style dict definition...
Yes, you should really never have a bare "except:" to avoid catching things like MemoryError or KeyboardInterrupt yourself.
How about just: def get_websockets(ws_type=None): return WEBSOCKETS.get(ws_type, None) -- meejah
BTW, where is this code coming from? I looked on github but only see the proposal stuf (which is using Tornado not Cyclone, so I'm guessing that's not the right stuff?)
Sorry for the noise, I found the code ("develop" branch of https://github.com/leivaburto/rwsd) and add a couple more comments: 1. The "WEBSOCKETS" global seems like it wants to be a class instead: you've basically got a bunch of static functions (add_websocket, etc) that manipulate a global variable (WEBSOCKETS) which will be hard to mock, etc. Making this its own class that encapsulates the websockets and contains methods ("init_websockets" becomes the constructor, ".add_websocket()" becomes a ".add" method etc) will make it easier to mock and use. IMO. So, e.g., instead of calling init_websockets() in run_server, you'd instantiate a new WebSocketTracker() or whatever and pass it to your Application where your handlers can then get at it. 2. I didn't look at any threading very closely, but in general if you're interacting with a threaded/syncrhonous library like Stem you're going to want to do that via deferToThread()/callFromThread() and so forth to avoid blocking the reactor for "too long" and to avoid calling reactor methods from any thread that isn't the main one. Practically speaking, with a 1:1 relay-to-local-Tor/Stem instance this might not really matter very much (i.e. if there's always 0 or 1 web browser clients it probably doesn't matter how long you pause the reactor). Cheers, meejah
On 05/08/15 15:21, meejah wrote:
Hi! Thank you both Damian and meejah for your comments! I'll review your feedback and fix the issues found. The code is hosted on Github: https://github.com/leivaburto/rwsd/tree/develop - Best, Cristobal
Links to raw source code on Google Code will go away in January 2016. "After January 2016 you will no longer be able to access source code from a version control client; … Most importantly, project URLs (e.g. http://code.google.com/p/vim) will continue to work, although links to raw source code like https://vim.googlecode.com/hg/README.txt will not. But again, Google Code will continue to work as normal until at least January 2016." https://code.google.com/p/support/wiki/ReadOnlyTransition Tim (teor) Tim Wilson-Brown (teor) teor2345 at gmail dot com pgp ABFED1AC https://gist.github.com/teor2345/d033b8ce0a99adbc89c5 teor at blah dot im OTR D5BE4EC2 255D7585 F3874930 DB130265 7C9EBBC7
participants (4)
-
Cristobal -
Damian Johnson -
meejah -
teor