Closed Bug 1091381 Opened 5 years ago Closed 5 years ago

Have MozReview publish events to Mozilla Pulse for use in automatic static analysis

Categories

(MozReview Graveyard :: General, enhancement, P1)

x86
All
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: smacleod)

Details

Attachments

(6 files, 1 obsolete file)

ReviewBot[1] is a Review Board extension that smacleod has developed that allows for arbitrary static analysis tools to be run on a particular review request posted to Review Board (and therefore, MozReview).

It can, for example, run pep8, pylint or pyflakes on any Python changes. It can run JSLint, JSHInt, etc, on Javascript.

The possibilities are endless, and it's extremely simple to have ReviewBot use your favourite static analysis tool and turn them into review comments.

We should get one going on MozReview.

[1]: https://github.com/reviewboard/ReviewBot
More productivity-saving awesomeness waiting in the wings. I like to say "why make humans do what machines can do faster." Static analysis is a great example of how the switch to Review Board will enable faster and better reviews leading to higher-quality and more readable code that can in turn be iterated on quickly.

What integration is there between Review Board and ReviewBot? Does Review Board need to publish a queue of submitted review requests or anything? Or does ReviewBot poll? Just trying to ascertain the architectural and security implications of this.

Also, we should seriously start considering on an in-tree mechanism for defining metadata to aid review. For example, different parts of mozilla-central have different style policies. I think we should extract this metadata from files in the tree (not unlike moz.build files perhaps) to guide the actions of bots. We can also use it as a mechanism for defining "modules" and "reviewers" and possibly "watchers." Bikeshedding is needed for large and multi-policy repos like mozilla-central. We can definitely move forward on things like pyflakes for smaller, more integrated repos like version-control-tools.

I will be super excited to see this deployed.
Indeed, this is one of those things we've been talking about for a long time that is made much easier with a Review-Board-based system.  I think we should do it soon.
Severity: normal → enhancement
Priority: -- → P1
I think smacleod is probably the right choice to answer gps's questions in comment 1.
Flags: needinfo?(smacleod)
(In reply to Gregory Szorc [:gps] from comment #1) 
> What integration is there between Review Board and ReviewBot?

Review Bot is really only designed to work with Review Board at this point, so it's pretty integrated. All the configuration for tools (e.g. Which pep8 codes to ignore, max line lengths...) lives in the Review Board database and is configured through the admin panel - The schema for this configuration comes from the plugin for the tool.

The master branch is kind of in a state of rewrite atm, with only few tasks left until it is ready to release. It brings extra features like being able to manually trigger a tool on your review request self serve (from an approved list) to go along with the automatic triggering.


> Does Review Board need to publish a queue of submitted review requests
> or anything?

The Extension uses Celery[1] to queue analysis tasks (and should work with either RabbitMQ or redis as the broker).


> Just trying to ascertain the architectural and security
> implications of this.

We'd need a message queue, and some "Worker" nodes which have the Review Bot Worker code installed along with the static analysis tool(s) for that worker.

TBH though, the currently released version of Review Bot[2] isn't really suitable for our deployment. It lacks features such as configuring the automatic tools to run only on certain repos / code paths, having multiple configurations per analysis tool etc.

If we really want to get Review Bot going here we'd need to use the master branch, which needs a bit more work to get it ready for release. If we could put a little time into finishing off the Review Bot rewrite it would probably suit our needs well. (Technically we could probably run the master branch as is, but configuring it would require hand editing JSON fields in the DB - the admin interface isn't complete).


> Also, we should seriously start considering on an in-tree mechanism for
> defining metadata to aid review. For example, different parts of
> mozilla-central have different style policies. I think we should extract
> this metadata from files in the tree (not unlike moz.build files perhaps) to
> guide the actions of bots. We can also use it as a mechanism for defining
> "modules" and "reviewers" and possibly "watchers." Bikeshedding is needed
> for large and multi-policy repos like mozilla-central. We can definitely
> move forward on things like pyflakes for smaller, more integrated repos like
> version-control-tools.

Out of the box Review Bot won't support any of this. However, the way Review Bot supports a static analysis tool is through thin python wrappers that are passed configuration + review request information, are then responsible for executing some static analysis tool and parsing the results to supply comment information.

So, it would be extremely simple to write custom tool plugins which suit our needs perfectly (which could read this meta data out of the repo). These plugins can also be registered by external python packages so none of it needs to land in core Review Bot.

[1] http://www.celeryproject.org/
[2] https://github.com/reviewboard/reviewbot/tree/release-0.2.x
Flags: needinfo?(smacleod)
Attachment #8551940 - Flags: review?(mcote)
Attachment #8551940 - Flags: review?(mconley)
Attachment #8551940 - Flags: review?(gps)
/r/2729 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2731 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2733 - mozreview: Introduce module for listening to signals.
/r/2735 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2737 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2739 - mozreview: Publish review_request_published to Pulse.

Pull down these commits:

hg pull review -r d99d264cae87683e0a2bb565eed6b9085f40387a
https://reviewboard.mozilla.org/r/2735/#review1815

::: pylib/mozreview/mozreview/extension.py
(Diff revision 1)
> +        'host': '',

Shouldn't you make these settings Pulse-specific, e.g. pulse_host, etc.?
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
/r/2729 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2731 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2733 - mozreview: Introduce module for listening to signals.
/r/2735 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2737 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2739 - mozreview: Publish review_request_published to Pulse.

Pull down these commits:

hg pull review -r fb17f2b5bc745037f1e34533dae3d8905d79fdda
https://reviewboard.mozilla.org/r/2733/#review1841

::: pylib/mozreview/mozreview/pulse/__init__.py
(Diff revision 1)
> +def initialize_handlers(extension):
> +    SignalHook(extension, review_request_published,
> +               handle_review_request_published)
> +
> +def handle_review_request_published(**kwargs):
> +    review_request = kwargs.get('review_request')

If we're going to go with the two-space spacing between global scope items, then these need to be separated by another newline.

::: pylib/mozreview/mozreview/extension.py
(Diff revision 1)
> -        pass
> +        initialize_handlers(self)

Since we want to keep MozReviewExtension thin, we'll likely be doing the same kind of initialization of things in the other portions that are coming in.

initialize_handlers is too generic, in that case. Let's call it initialize_pulse_handlers, or something like that.
https://reviewboard.mozilla.org/r/2737/#review1869

::: pylib/mozreview/mozreview/decorators.py
(Diff revision 2)
> +    def _wrapped(*args, **kwargs):
> +        ext = kwargs.get('extension', None)
> +
> +        if ext is None:
> +            return
> +
> +        if not ext.settings.get('enabled', False):
> +            return
> +
> +        return fn(*args, **kwargs)

So everybody who uses this decorator needs to pass in the MozReviewExtension? That doesn't seem right. Are we planning on having this decorator work with other extensions that are not MozReviewExtension?
https://reviewboard.mozilla.org/r/2729/#review1871

::: pylib/mozreview/setup.py
(Diff revision 1)
> +    name=PACKAGE_NAME,

Drop the variable.
https://reviewboard.mozilla.org/r/2731/#review1873

::: testing/docker/builder-eggbuild/eggserver.py
(Diff revision 1)
> +    '/version-control-tools/pyblic/mozreview/dist',

pyblic?
https://reviewboard.mozilla.org/r/2739/#review1887

::: pylib/mozreview/mozreview/pulse/__init__.py
(Diff revision 2)
> +    pulse = publishers.MozReviewPublisher(**config)

I wonder what the overhead of connecting to Pulse is. If we are publishing 50 changesets, we'll have to establish 50 connections to Pulse. That's kinda crappy.

Is there an easy way to do this while preserving the connection?
Attachment #8551940 - Flags: review?(mcote) → review+
https://reviewboard.mozilla.org/r/2737/#review1905

> So everybody who uses this decorator needs to pass in the MozReviewExtension? That doesn't seem right. Are we planning on having this decorator work with other extensions that are not MozReviewExtension?

No, the decorator needs to recieve an instance of the extension. Anything wired up with SignalHandler will always recieve this instance, which is the primary use case of the decorator. I've added more explanation to the doc string.
https://reviewboard.mozilla.org/r/2739/#review1907

> I wonder what the overhead of connecting to Pulse is. If we are publishing 50 changesets, we'll have to establish 50 connections to Pulse. That's kinda crappy.
> 
> Is there an easy way to do this while preserving the connection?

When I talked to Mark about this he said nope, just always connect and disconnect. Mark what are your thoughts on this?
Attachment #8551940 - Flags: review+ → review?(mcote)
/r/2729 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2731 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2733 - mozreview: Introduce module for listening to signals.
/r/2735 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2737 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2739 - mozreview: Publish review_request_published to Pulse.

Pull down these commits:

hg pull review -r 8a57500008621fcd0a2dd7056728c4d530fcec48
Attachment #8551940 - Flags: review?(mconley) → review+
https://reviewboard.mozilla.org/r/2727/#review1909

::: pylib/mozreview/mozreview/decorators.py
(Diff revisions 2 - 3)
> +    expected an extension kwarg which is the primary use case of this decorator.

nit "expected" -> "expect"
Attachment #8551940 - Flags: review+ → review?(mconley)
/r/2729 - mozreview: Add barebones extension and packaging (Bug 1091381)
/r/2731 - mozreview: Integrate mozreview into the testing and development machinery.
/r/2733 - mozreview: Introduce module for listening to signals.
/r/2735 - mozreview: Add form to configure Mozilla Pulse settings.
/r/2737 - mozreview: Only execute signal handlers if settings.enabled is True
/r/2739 - mozreview: Publish review_request_published to Pulse.

Pull down these commits:

hg pull review -r 5954e586711fec2dc6aaffc239cbffa4641e529a
https://reviewboard.mozilla.org/r/2739/#review1911

::: pylib/mozreview/mozreview/pulse/__init__.py
(Diff revision 2)
> +    pulse = publishers.MozReviewPublisher(**config)

If you keep your MozReviewPublisher around, you can call publish() multiple times.
https://reviewboard.mozilla.org/r/2739/#review1919

> If you keep your MozReviewPublisher around, you can call publish() multiple times.

I've given myself a TODO to look into reusing connections in a later request.
Comment on attachment 8551940 [details]
MozReview Request: bz://1091381/smacleod

https://reviewboard.mozilla.org/r/2727/#review1931

Ship It!
Attachment #8551940 - Flags: review?(mcote) → review+
Final commit pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/e181ee62360b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Summary: Install ReviewBot on MozReview for rapid static analysis and feedback → Have MozReview publish events to Mozilla Pulse for use in automatic static analysis
Attachment #8551940 - Flags: review?(mconley)
Attachment #8551940 - Flags: review?(gps)
Attachment #8551940 - Flags: review+
Attachment #8551940 - Attachment is obsolete: true
Attachment #8618504 - Flags: review+
Attachment #8618505 - Flags: review+
Attachment #8618506 - Flags: review+
Attachment #8618507 - Flags: review+
Attachment #8618508 - Flags: review+
Attachment #8618509 - Flags: review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.