932357 - WebSocket server for bug-change messages

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Bugzilla Change Notification System, defect)

Description

[Mark Côté [:mcote]]

This app will subscribe to bug-change messages on pulse and forward them to WebSocket clients.  Clients will be able to subscribe to one or more (or all) bugs.

Comment 1

[Mark Côté [:mcote]]

Attachment: Initial commits

I'm not really sure who to get to review this, so tagged a few people for review & feedback.

Also not sure what of the best way to post a review for an initial commit... I did a full diff against a blank directory, which is silly but will allow you to use Splinter for comments.  Or feel free to comment directly on the commit; the repo is at https://github.com/mozilla/bugzfeed.

As discussed at the A-Team meeting, I removed the ability to subscribe to all bugs because of its potential for misuse.

Comment 2

[Mark Côté [:mcote]]

Aw, for some reason that doesn't work in Splinter after all.  Suggestions welcome for how to review new projects like this...

Comment 3

[Mark Côté [:mcote]]

Oh also note that this depends on the patch to mozlog in bug 938823.

Comment 4

[Mark Côté [:mcote]]

The dependent fix has been applied and deployed; I updated the bugzfeed repo with the dependency (and a .gitignore).

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 832691 [details] [diff] [review]
Initial commits

I did a pretty high-level review, and didn't see any issues.  We should probably add an MPL header to the example HTML/CSS files.

Also, importing bugzfeed from bugzfeed's setup.py seems a bit sketchy, although it doesn't cause any problems here since __init__.py only defines version.

Comment 6

[Mark Côté [:mcote]]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> Comment on attachment 832691 [details] [diff] [review]
> Initial commits
> 
> I did a pretty high-level review, and didn't see any issues.  We should
> probably add an MPL header to the example HTML/CSS files.

Right, will do.  I'm not super happy with the examples; they could be cleaner, look nicer, and be more configurable, but I got bored of working on them. :)

> Also, importing bugzfeed from bugzfeed's setup.py seems a bit sketchy,
> although it doesn't cause any problems here since __init__.py only defines
> version.

Yeah, it's an idea I stole from http://www.jeffknupp.com/blog/2013/08/16/open-sourcing-a-python-project-the-right-way/ It avoids having to define the version in two places (if you wanted it to be directly accessible from client apps as well as in the package info).

Comment 7

[Peter Bengtsson [:peterbe]]

It's quite a large patch for a review. Especially since it's just a prototype. 

If it works it works and I'm r+ on things that work. 

A couple of small things caught my eye though: 

* Instead of using tornado.websocket.WebSocket you should consider using https://github.com/MrJoes/tornadio2 instead. It has the advantage that it's able to work with clients that don't support WebSockets but have to fall back o XHR "hacks" like long-polling. It also means you'll be able to use http://socket.io/ on the client side. 

* The way you use optparse is not ideal. If anything, use argparse. (see http://docs.python.org/2/library/optparse.html)
Even better would be to use tornado's own wrapper. See https://github.com/peterbe/toocool/blob/master/app.py#L16-L18 and https://github.com/peterbe/toocool/blob/master/app.py#L65 for ideas. 

* Your setup.py imports bugzfeed which is dangerous in case you, some day, do other imports in the `__init__.py`. Also, I see `long_description = read('README.md')` but I don't see `README.md` being included in the package. I think you need to have a `MANIFEST.in` file containing the line `include README.md`

* The BugSubscriptions is awesome! 

* You have a file called `websocket.py` that imports `from tornado import websocket`. Seems a dangerous clash of module names. Perhaps change to `import tornado.websocket ...  tornado.websocket.WebSocketHandler`

* I never actually tried running it. Would it even be possible to run locally?

Comment 8

[:glob ✱]

i'd like to see your example code use best practices and only ask bmo for the fields which it's interested in, rather than always requesting all fields.  this is especially important for the attachment request, which returns the payload by default.

Comment 9

[Mark Côté [:mcote]]

(In reply to Peter Bengtsson [:peterbe] from comment #7)
> It's quite a large patch for a review. Especially since it's just a
> prototype. 
> 
> If it works it works and I'm r+ on things that work. 
> 
> A couple of small things caught my eye though: 

This is all I was looking for, general feedback.  I find with new projects like this, we only start reviewing changes after v1.0 is released, without giving the developer a chance to improve or fix things before v1.0.  And sure enough, this looks like valuable feedback, so thanks. :)

> * Instead of using tornado.websocket.WebSocket you should consider using
> https://github.com/MrJoes/tornadio2 instead. It has the advantage that it's
> able to work with clients that don't support WebSockets but have to fall
> back o XHR "hacks" like long-polling. It also means you'll be able to use
> http://socket.io/ on the client side. 

Ah very cool, I will look into that.

> * The way you use optparse is not ideal. If anything, use argparse. (see
> http://docs.python.org/2/library/optparse.html)
> Even better would be to use tornado's own wrapper. See
> https://github.com/peterbe/toocool/blob/master/app.py#L16-L18 and
> https://github.com/peterbe/toocool/blob/master/app.py#L65 for ideas. 

Actually I started using argparse when I was doing the pulse shim, but the BMO development box, which I believe has the same OS as BMO proper, doesn't yet have Python 2.7 in it (ugh, I know).  So I went with optparse for the shim.  Then I wasn't really thinking clearly and went with optparse for the WebSocket server...which is not intended to run directly on a BMO box, so that requirement doesn't apply.  However I didn't know about tornado's wrapper; I'll check that out.

> * Your setup.py imports bugzfeed which is dangerous in case you, some day,
> do other imports in the `__init__.py`. Also, I see `long_description =
> read('README.md')` but I don't see `README.md` being included in the
> package. I think you need to have a `MANIFEST.in` file containing the line
> `include README.md`

Yeah, it is a bit weird.  I stole the idea from http://www.jeffknupp.com/blog/2013/08/16/open-sourcing-a-python-project-the-right-way/; the general idea is that you only have to specify the version in one place and make it available to both the package metadata and within the package itself.  But it is weird, and you aren't the first one to point that out, so maybe I'll take it out.

Not sure if I need to package the README.md, since it should only be read when creating a package, as the long description.  Might make sense to only keep it in the source.  But I'll think about that.

> * You have a file called `websocket.py` that imports `from tornado import
> websocket`. Seems a dangerous clash of module names. Perhaps change to
> `import tornado.websocket ...  tornado.websocket.WebSocketHandler`

Oops yeah, good point.

> * I never actually tried running it. Would it even be possible to run
> locally?

Oh for sure.  Check out HACKING.md. :)  The shim is not yet writing to Pulse, so you'd have to have your own Pulse server and a local BMO box, or at least the shim and a MySQL db, but it should be deployed to bugzilla-dev soon, for testing, at which point you should be able to run the server locally and see changes on bugzilla-dev.

Comment 10

[Mark Côté [:mcote]]

(In reply to Byron Jones ‹:glob› from comment #8)
> i'd like to see your example code use best practices and only ask bmo for
> the fields which it's interested in, rather than always requesting all
> fields.  this is especially important for the attachment request, which
> returns the payload by default.

Oops, yup, good point.

Comment 11

[Peter Bengtsson [:peterbe]]

> 
> Not sure if I need to package the README.md, since it should only be read
> when creating a package, as the long description.  Might make sense to only
> keep it in the source.  But I'll think about that.
> 
If you run `python setup.py sdist` it'll create a zip file that won't contain the README.md but it will contain a setup.py that depends on README.md being available.

Comment 12

[Jonathan Griffin (:jgriffin)]

> Yeah, it is a bit weird.  I stole the idea from http://www.jeffknupp.com/blog/2013/08/16/open-sourcing-
> a-python-project-the-right-way/; the general idea is that you only have to specify the version in one 
> place and make it available to both the package metadata and within the package itself.  But it is 
> weird, and you aren't the first one to point that out, so maybe I'll take it out.

FWIW, we do this in gaiatest in a slightly different way that won't break if __init__.py changes:

https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/setup.py

Comment 13

[Peter Bengtsson [:peterbe]]

Or there's this idea:
https://github.com/mozilla/configman/blob/master/setup.py#L12
https://github.com/mozilla/configman/blob/master/configman/__init__.py#L41

And my personal favorite:
https://github.com/peterbe/premailer/blob/master/setup.py#L39

Comment 14

[Mark Côté [:mcote]]

I see tornado has logging too.  Didn't realize it contained so many util packages.  I'll check them all out.

A bit annoying that the arg usage doesn't seem to be configurable for positional args, but I should probably find a better way to specify the config file anyway.

Comment 15

[:glob ✱]

Comment on attachment 832691 [details] [diff] [review]
Initial commits

i can't really comment on the python code, clearing feedback flag.

Comment 16

[Mark Côté [:mcote]]

Applied fixes for everything except this:

> * Instead of using tornado.websocket.WebSocket you should consider using
> https://github.com/MrJoes/tornadio2 instead. It has the advantage that it's
> able to work with clients that don't support WebSockets but have to fall
> back o XHR "hacks" like long-polling. It also means you'll be able to use
> http://socket.io/ on the client side. 

The README.md for that project says that TornadIO2 is no longer supported due to SocketIO 0.8 being abandoned.  So probably doesn't make sense for me to switch to it, at least right now.

Comment 17

[Mark Côté [:mcote]]

Comment on attachment 832691 [details] [diff] [review]
Initial commits

I've gotten enough feedback here; unflagging dkl (but, dkl, feel free to look at the code if you ever want to. :)

Comment 18

[Mark Côté [:mcote]]

The initial version is done; deployment is tracked in bug 952881.