Planet MozReview

May 08, 2017

Byron Jones

servo, firefox, and synchronisation

What do you do when you have two very different projects that both want to maintain their independence but developers need to land coordinated changes across them?

a quick introduction

I'm currently working on a project that's outside of my normal realm; "bi-directional" synchronisation between a Mercurial repository and one hosted on GitHub.

But before I dive too deep there's some terms and processes that I should define.

mozilla-central

A large mercurial host repository that stores the majority of the Firefox source.

mozilla-integration branches

Called "branches" internally, these are actually separate Mercurial repositories. Reviewed code is landed onto an integration branch (historically mozilla-inbound) which then triggers Firefox's test suite.

sheriffs

They look after the "tree" (the repository). One task they perform is monitoring the test results on the integration branches, and either merge into mozilla-central if it passes, or backout the commit if it causes failures.

servo, gecko, stylo, and quantum

Servo is a browser written in rust under Mozilla's research umbrella. It was designed from the ground up to take advantage of modern CPU and GPU architectures. Gecko is the core engine that drives Firefox, from parsing HTML to rendering the page. Stylo is a project to bring Servo's modern CSS/layout engine into Firefox. Quantum is a large collection of work that plans on delivering significant changes to Firefox's core engine (aka "the Firefox platform" or just "the platform").

autolanders

These take code that has passed review and land them on the target repository. Servo's autolander "homu" gates landed on a successful run of Servo's tests (continuous integration, or CI), while Gecko's autolander "autoland" just transplants code from the review to the integration repository.

Servo is developed on GitHub, while Gecko's lives in a Mercurial repository. For a multitude of good reasons it was decided that neither moving Servo to Mercurial or Gecko to GitHub would be viable. What we needed was a way for changes made on GitHub be synchronised to Mercurial and vice-versa.

the problem

The core problem at hand is it's fairly common for Stylo patches to touch both Servo and Gecko at the same time. These changes need to land simultaneously on the integration branch in order for the Firefox tests to pass. In the current environment this is not possible. Changes first need to land on Servo via a GitHub pull request, wait for Servo's tests to run and for homu to land the code (~45 minutes), watch the integration branch for the Servo changes to be overlaid, ignore the build/test failures, and finally push the Gecko changes to fix the code and (hopefully) return tests to a passing state.

Needless to say this is a sub-optimal experience for both developers and sheriffs.

in the beginning - unification

Long before I was involved the plan called for unification of the Servo and Firefox autolanders.

unified

Large parts of homu would be rewritten into Gecko's autoland, and changes to both GitHub and Mercurial would funnel through this central autolander. It would kick off tests for both Servo and Gecko, and would land to both repositories only if tests on both repositories passed.

This plan was ambitious and relied Gecko's test suite returning a boolean pass/fail result.

autolander development

As it happens both of the project's autolanders were developed rapidly and grew organically as demands grew. As a result working on them can present issues, especially when implementing non-trivial changes. homu doesn't have any real tests, which adds risk to any large scale changes. Meanwhile autoland's development environment can be a nightmare at times; look at it the wrong way and you might lose the next two hours of your day getting your environment up and running again.

It became clear that somebody from outside of the autoland team was not going to be able to extend it to merge the two landers without heroic efforts.

gps and glob join the party

At the end of last year, gps and I were called in to provide whatever help we could to this project. gps is an expert in all things source control related, and I was familiar with gecko's autoland. While gps dove straight into developing servo-vcs-sync (see below), I acted in a supporting role for the existing engineer, however I wasn't called upon much in that capacity.

My involvement really ramped up when gps took one and a half months of leave at the start of this year, and I was called in to cover for him as best as I could.

servo-vcs-sync is born

gps realised that implementing uni-direction synchronisation from GitHub to Mercurial would be an important first step, and would ease a lot of the issues the Stylo team were having with their manual vendoring in of the servo code. He worked diligently on this and servo-vcs-sync was deployed on the Friday before his long leave.

servo-vcs-sync

This system and works well, but it wasn't without some teething problems; in fact it failed on its first weekend, generating ~10k error emails before the server was shutdown (pans out the failure retry interval was accidentally set to 100ms instead of 5 minutes - ouch).

In the time that gps was away I was able to resolve issues and add some missing functionality and it's been stable for some time.

a change of ownership

At the same time it was realised that the existing engineer was unable to work on the project and ownership was transferred to me. Development had not progressed beyond the planning stage, which gave me the opportunity to really look at the plan with a fresh set of eyes.

It also became evident that a critical part of this project - the ability for the Firefox's test suite to return a boolean pass or fail - was much harder than expected, and would not be ready within a reasonable time-frame.

What was needed was a new plan.

a new plan, or two

Being brought into the project late meant was good and bad. I had a lot to learn, but I also didn't have any preconceived notions of how this should be done; I was able to focus on the end goal.

I set up meetings and spoke with anyone who would listen to gather advice. I drew diagrams, we discussed, I discarded and drew more diagrams. At every increment complexity was shaved off.

For example early designs called for a new service to co-ordinate the two autolanders - the idea was for changes to register patches with the coordinator, which gathered results and ensured patches landed in the correct order across both repositories.

alloy

and here were are

After much discussion, especially around how best to manage backouts, we've settled on the following plan.

When a change is requested to be landed through autoland the following will happen:

  1. The list of files modified is checked to determine if files in the servo/ directory are touched
  2. If this is the case, the request is not landed by autoland, instead it notes the gecko changes then sends a message to the servo-vcs-sync service
  3. servo-vcs-sync creates a pull request on the servo/servo GitHub repository
  4. Homu tests and lands the pull request as per the existing process
  5. servo-vcs-sync is triggered by the changes to the GitHub repository
  6. Any gecko changes are mixed into the commit before it is pushed to the integration/autoland repository

Backouts:

  1. A sheriff performs a normal backout on the integration/autoland repository
  2. The mercurial server notifies servo-vcs-sync via pulse
  3. servo-vcs-sync creates a pull request on the servo/servo GitHub repository with the highest priority
  4. Homu lands the PR before any other, including those in-flight
  5. The change flows through to gecko via the normal GitHub → Mercurial flow

The following diagrams illustrate the flow of data across the systems.

Landing:

landing

Backout:

backout

I'm currently working on implementing this, which thus far mostly involves shaving yaks.

May 08, 2017 02:40 PM

January 13, 2017

Mark Coté

Project Conduit

In 2017, Engineering Productivity is starting on a new project that we’re calling “Conduit”, which will improve the automation around submitting, testing, reviewing, and landing commits. In many ways, Conduit is an evolution and course correction of the work on MozReview we’ve done in the last couple years.

Before I get into what Conduit is exactly, I want to first clarify that the MozReview team has not really been working on a review tool per se, aside from some customizations requested by users (line support for inline diff comments). Rather, most of our work was building a whole pipeline of automation related to getting code landed. This is where we’ve had the most success: allowing developers to push commits up to a review tool and to easily land them on try or mozilla-central. Unfortunately, by naming the project “MozReview” we put the emphasis on the review tool (Review Board) instead of the other pieces of automation, which are the parts unique to Firefox’s engineering processes. In fact, the review tool should be a replaceable part of our whole system, which I’ll get to shortly.

We originally selected Review Board as our new review tool for a few reasons:

However, we’ve since realized that Review Board has some big downsides, at least with regards to Mozilla’s engineering needs:

In addition, the direction that the upstream project is taking is not really in line with what our users are looking for in a review tool.

So, we’re taking a step back and evaluating our review-tool requirements, and whether they would be best met with another tool or by a small set of focussed improvements to Review Board. Meanwhile, we need to decouple some pieces of MozReview so that we can accelerate improvements to our productivity services, like Autoland, and ensure that they will be useful no matter what review tool we go with. Project Conduit is all about building a flexible set of services that will let us focus on improving the overall experience of submitting code to Firefox (and some other projects) and unburden us from the restrictions of working within Review Board’s extension system.

In order to prove that our system can be independent of review tool, and to give developers who aren’t happy with Review Board access to Autoland, our first milestone will be hooking the commit repo (the push-to-review feature) and Autoland up to BMO. Developers will be able to push a series of one or more commits to the review repo, and reviewers will be able to choose to review them either in BMO or Review Board. The Autoland UI will be split off into its own service and linked to from both BMO and Review Board.

(There’s one caveat: if there are multiple reviewers, the first one gets to choose, in order to limit complexity. Not ideal, but the problem quickly gets much more difficult if we fork the reviews out to several tools.)

As with MozReview, the push-to-BMO feature won’t support confidential bugs right away, but we have been working on a design to support them. Implementating that will be a priority right after we finish BMO integration.

We have an aggressive plan for Q1, so stay tuned for updates.

January 13, 2017 05:56 PM

October 13, 2016

Mark Coté

MozReview UI refactoring

In Q3 the MozReview team started to focus on tackling various usability issues. We started off with a targetted effort on the “Finish Review” dialog, which was not only visually unappealing but difficult to use. The talented David Walsh compressed the nearly full-screen dialog into a dropdown expanded from the draft banner, and he changed button names to clarify their purpose. We have some ideas for further improvements as well.

David has now embarked on a larger mission: reworking the main review-request UI to improve clarity and discoverability. He came up with some initial designs and discussed them with a few MozReview users, and here’s the result of that conversation:

This design provides some immediate benefits, and it sets us up for some future improvements. Here are the thoughts behind the changes:

The commits table, which was one of the first things we added to stock Review Board, was never in the right place. All the surrounding text and controls reflect just the commit you are looking at right now. Moving the table to a separate panel above the commit metadata is a better, and hopefully more intuitive, representation of the hierarchical relationship between commit series and individual commit.

The second obvious change is that the commit table is now collapsed to show only the commit you are currently looking at, along with its position (e.g. “commit 3 of 5”) and navigation links to previous and next commits. This places the emphasis on the selected commit, while still conveying the fact that it is part of a series of commits. (Even if that series is actually only one commit, it is still important to show that MozReview is designed to operate on series.) To address feedback from people who like always seeing the entire series, it will be possible to expand the table and set that as a preference.

The commit title is still redundant, but removing it from the second panel left the rest of the information there looking rather abandoned and confusing. I’m not sure if there is a good fix for this.

The last functional change is the addition of a “Quick r+” button. This fixes the annoying process of having to select “Finish Review”, set the dropdown to “r+”, and then publish. It also removes the need for the somewhat redundant and confusing “Finish Review” button, since for anything other than an r+ a reviewer will most likely want to leave one or more comments explaining their action. The “Quick r+” button will probably be added after the other changes are deployed, in part because we’re not completely satisfied with its look and position.

The other changes are cosmetic, but they make various data and controls look much slicker while also being more compact.

We are also noodling around with a further enhancement:

This is a banner containing data about the current commit, which will appear when the user scrolls past the commits table. It provides a constant reminder of the current commit, and we may put in a way to skip up to the commits table and/or navigate between commits. We may also fold the draft/“Finish Review” banner into this as well, although we’re still working out what that would look like. In any case, this should help avoid unnecessary scrolling while also presenting a “you are here” signpost.

As I mentioned, these changes are part of an on-going effort to improve general usability. This refactoring gets us into position to tackle more issues:

MozReview usage is slowly ticking upwards, as more and more Mozillians are seeing the value of splitting their work up into a series of small, atomic commits; appreciating the smooth flow of pushing commits up for review; and especially digging the autoland functionality. We’re now hard at work to make the whole experience delightful.

October 13, 2016 01:20 AM

July 04, 2016

Byron Jones

adding "reviewer delegation" to MozReview

Background

One of the first projects I worked on when I moved to the MozReview team was "review delegation". The goal was to add the ability for a reviewer to redirect a review request to someone else. It turned out to be a change that was much more complicated than expected.

MozReview is a Mozilla developed extension to the open source Review Board product; with the primary focus on working with changes as a series of commits instead of as a single unified diff.  This requires more than just a Review Board extension, it also encompasses a review repository (where reviews are pushed to), as well as Mercurial and Git modules that drive the review publishing process.  Autoland is also part of our ecosystem, with work started on adding static analysis (eg. linters) and other automation to improve the code review workflow.

I inherited the bug with a patch that had undergone a single review. The patch worked by exposing the reviewers edit field to all users, and modifying the review request once the update was OK'd. This is mostly the correct approach, however it had two major issues:

  1. According to Review Board and Bugzilla, the change was always made by the review's submitter, not the person who actually made the change
  2. Unlike other changes made in Review Board, the review request was updated immediately instead of using a draft which could then be published or discarded

Permissions

Review Board has a simple permissions model - the review request's submitter (aka patch author) can make any change, while the reviewers can pretty much only comment, raise issues, and mark a review as "ready to ship" / "fix it". As you would expect, there are checks within the object layer to ensure that these permission boundaries are not overstepped.  Review Board's extension system allows for custom authorisation and permissions check, however the granularity of these are course: you can only control if a user can edit a review request as a whole, not on a per-field level.

In order to allow the reviewer list to be changed, we need to tell Review Board that the submitter was making the change.

Performing the actions as the submitter instead of the authenticated user is easy enough, however when the changes were pushed to Bugzilla they were attributed to the wrong user. After a few false starts, I settled on storing the authenticated user in the request's meta data, performing the changes as the submitter, and updating the routines that touch Bugzilla to first check for a stored user and make changes as that user instead of the submitter. Essentially "su".

This worked well except for on the page where the meta data changes are displayed - here the change was incorrectly attributed to the review submitter. Remember that under Review Board's permissions model only the review submitter can make these changes, so the name and gravatar were hard-coded in the django template to use the submitter. Given the constraints a Review Board extension has to operate in, this was a problem, and developing a full audit trail for Review Board would be too time consuming and invasive. The solution I went with was simple: hide the name and gravatar using CSS.

Drafts and Publishing

How Review Board works is, when you make a change, a draft is (almost) always created which you can then publish or discard. Under the hood this involves duplicating the review request into a draft object, against which modifications are made. A review request can only have one draft, which isn't a problem because only the submitter can change the review.

Of course for reviewer delegation to work we needed a draft for each user. Tricky.

The fix ended up needing to take a different approach depending on if the submitter was making the change or not.

When the review submitter updates the reviewers, the normal review board draft is used, with a few tweaks that show the draft banner when viewing any revision in the series instead of just the one that was changed. This allows us to correctly cope with situations where the submitter makes changes that are broader than just the reviewers, such as pushing a revised patch, or attaching a screenshot.

When anyone else updates the reviewers, a "draft" is created in local storage in their browser, and a fake draft banner is displayed. Publishing from this fake draft banner calls the API endpoint that performs the permissions shenanigans mentioned earlier.

screenshot-2016-07-01-14-47-33

Overall it has been an interesting journey into how Review Board works, and highlighted some of the complications MozReview hits when we need to work against Review Board's design. We've been working with the Review Board team to ease some of these issues, as well as deviating where required to deliver a Mozilla-focused user experience.

July 04, 2016 07:20 AM

April 22, 2016

Mark Coté

How MozReview helps

A great post on code review is making its rounds. It’s started some discussion amongst Mozillians, and it got me thinking about how MozReview helps with the author’s points. It’s particularly interesting because apparently Twitter uses Review Board for code reviews, which is a core element of the whole MozReview system.

The author notes that it’s very important for reviewers to know what reviews are waiting on them, but also that Review Board itself doesn’t do a good job of this. MozReview fixes this problem by piggybacking on Bugzilla’s review flags, which have a number of features built around them: indicators, dashboards, notification emails, and reminder emails. People can even subscribe to the reminders for other reviewers; this is a way managers can ensure that their teams are responding promptly to review requests. We’ve also toyed around with the idea of using push notifications to notify people currently using Bugzilla that they have a new request (also relevant to the section on being “interrupt-driven”).

On the submitter side, MozReview’s core support for microcommits—a feature we built on top of Review Board, within our extensions—helps “keep reviews as small as possible”. While it’s impossible to enforce small commits within a tool, we’ve tried to make it as painless as possible to split up work into a series of small changes.

The MozReview team has made progress on automated static analysis (linters and the like), which helps submitters verify that their commits follow stylistic rules and other such conventions. It will also shorten review time, as the reviewer will not have to spend time pointing out these issues; when the review bots have given their r+s, the reviewer will be able to focus solely on the logic. As we continue to grow the MozReview team, we’ll be devoting some time to finishing up this feature.

April 22, 2016 03:39 PM

March 23, 2016

Byron Jones

mozreview and inline comments on the diff view

when a comment is left on a review in review board/mozreview it is currently displayed as a small square in the left column.

screenshot-2016-03-23-23-25-33

our top reviewers have strongly indicated that this is suboptimal and would prefer to match what most other code review systems do in displaying comments as an inline block on the diff. i agree — review comments are important and deserve more attention in the user interface; they should be impossible to miss.

while the upstream review board team have long said that the current display is in need of fixing, there is minimal love for the inline comments approach.

recently we worked on a plan of attack to appease both our reviewers and upstream's requirements.  :smacleod, :mconley and i talked through a wide assortment of design goals and potential issues.  we have a design document and i've started mocking up the approach in html:

inline-comments-mock-up

we also kicked off discussions with the upstream review board development team, and are hopeful that this is a feature that will be accepted upstream.

March 23, 2016 03:39 PM

February 08, 2016

Greg Szorc

MozReview Git Support and Improved Commit Mapping

MozReview - Mozilla's Review Board based code review tool - now supports ingestion from Git. Previously, it only supported Mercurial.

Instructions for configuring Git with MozReview are available. Because blog posts are not an appropriate medium for documenting systems and processes, I will not say anything more here on how to use Git with MozReview.

Somewhat related to the introduction of Git support is an improved mechanism for mapping commits to existing review requests.

When you submit commits to MozReview, MozReview has to decide how to map those commits to review requests in Review Board. It has to choose whether to recycle an existing review request or create a new one. When recycling, is has to pick an appropriate one. If it chooses incorrectly, wonky things can happen. For example, a review request could switch to tracking a new and completely unrelated commit. That's bad.

Up until today, our commit mapping algorithm was extremely simple. Yet it seemed to work 90% of the time. However, a number of people found the cracks and complained. With Git support coming online, I had a feeling that Git users would find these cracks with higher frequency than Mercurial users due to what I perceive to be variations in the commit workflows of Git versus Mercurial. So, I decided to proactively improve the commit mapping before the Git users had time to complain.

Both the Git and Mercurial MozReview client-side extensions now insert a MozReview-Commit-ID metadata line in commit messages. This line effectively defines a (likely) unique ID that identifies the commit across rewrites. When MozReview maps commits to review requests, it uses this identifier to find matches. What this means is that history rewriting (such as reordering commits) should be handled well by MozReview and should not confuse the commit mapping mechanism.

I'm not claiming the commit mapping mechanism is perfect. In fact, I know of areas where it can still fall apart. But it is much better than it was before. If you think you found a bug in the commit mapping, don't hesitate to file a bug. Please have it block bug 1243483.

A side-effect of introducing this improved commit mapping is that commit messages will have a MozReview-Commit-ID line in them. This may startle some. Some may complain about the spam. Unfortunately, there's no better alternative. Both Mercurial and Git do support a hidden key-value dictionary for each commit object. In fact, the MozReview Mercurial extension has been storing the very commit IDs that now appear in the commit message in this dictionary for months! Unfortunately, actually using this hidden dictionary for metadata storage is riddled with problems. For example, some Mercurial commands don't preserve all the metadata. And accessing or setting this data from Git is painful. While I wish this metadata (which provides little value to humans) were not located in the commit message where humans could be bothered by it, it's really the only practical place to put it. If people find it super annoying, we could modify Autoland to strip it before landing. Although, I think I like having it preserved because it will enable some useful scenarios down the road, such as better workflows for uplift requests. It's also worth noting that there is precedent for storing unique IDs in commit messages for purposes of commit mapping in the code review tool: Gerrit uses Change-ID lines.

I hope you enjoy the Git support and the more robust commit to review request mapping mechanism!

February 08, 2016 11:05 AM

February 02, 2016

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1243246] Attachment data submitted via REST API must always be base64 encoded
  • [1213424] The Bugzilla autocomplete dropdown should expand the width to show the full text of a match
  • [1241667] Trying to report a bug traps the user in an infinite loop
  • [1188236] "Congratulations on having your first patch approved" email should be clearer about how to get the patch landed.
  • [1243051] Create one off script to output cpanfile with all modules and their current versions to be used for version pinning
  • [1244604] configure nagios alerting for the bmo/reviewboard connector
  • [1244996] add a script to manage a user's settings
discuss these changes on mozilla.tools.bmo.

February 02, 2016 07:23 AM

January 20, 2016

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1236161] when converting a BMP attachment to PNG fails a zero byte attachment is created
  • [1231918] error handler doesn't close multi-part responses
discuss these changes on mozilla.tools.bmo.

January 20, 2016 07:33 AM

January 13, 2016

Greg Szorc

Making MozReview Faster by Disregarding RESTful Design

When I first started writing web services, I was a huge RESTful fan boy. The architectural properties - especially the parts related to caching and scalability - really jived with me. But as I've grown older and gained experienced, I've realized that RESTful design, like many aspects of software engineering, is more of a guideline or ideal than a panacea. This post is about one of those experiences.

Review Board's Web API is RESTful. It's actually one of the better examples of a RESTful API I've seen. There is a very clear separation between resources. And everything - and I mean everything - is a resource. Hyperlinks are used for the purposes described in Roy T. Fielding's dissertation. I can tell the people who authored this web API understood RESTful design and they succeeded in transferring that knowledge to a web API.

Mozilla's MozReview code review tool is built on top of Review Board. We've made a number of customizations. The most significant is the ability to submit a series of commits as one logical review series. This occurs as a side-effect of a hg push to the code review repository. Once your changesets are pushed to the remote repository, that server issues a number of Review Board Web API HTTP requests to reviewboard.mozilla.org to create the review requests, assign reviewers, etc. This is mostly all built on the built-in web API endpoints offered by Review Board.

Because Review Board's Web API adheres to RESTful design principles so well, turning a series of commits into a series of review requests takes a lot of HTTP requests. For each commit, we have to perform something like 5 HTTP requests to define the server state. For series of say 10 commits (which aren't uncommon since we try to encourage the use of microcommits), this can add up to dozens of HTTP requests! And that's just counting the HTTP requests to Review Board: because we've integrated Review Board with Bugzilla, events like publishing result in additional RESTful HTTP requests from Review Board to bugzilla.mozilla.org.

At the end of the day, submitting and publishing a series of 10 commits consumes somewhere between 75 and 100 HTTP requests! While the servers are all in close physical proximity (read: low network latencies), we are reusing TCP connections, and each HTTP request completes fairly quickly, the overhead adds up. It's not uncommon for publishing commit series to take over 30s. This is unacceptable to developers. We want them to publish commits for review as quickly as possible so they can get on with their next task. Humans should not have to wait on machines.

Over in bug 1220468, I implemented a new batch submit web API for Review Board and converted the Mercurial server to call it instead of the classic, RESTful Review Board web APIs. In other words, I threw away the RESTful properties of the web API and implemented a monolith API doing exactly what we need. The result is a drastic reduction in net HTTP requests. In our tests, submitting a series of 20 commits for review reduced the HTTP request count by 104! Furthermore, the new API endpoint performs all modifications in a single database transaction. Before, each HTTP request was independent and we had bugs where failures in the middle of a HTTP request series left the server in inconsistent and unexpected state. The new API is significantly faster and more atomic as a bonus. The main reason the new implementation isn't yet nearly instantaneous is because we're still performing several RESTful HTTP requests to Bugzilla from Review Board. But there are plans for Bugzilla to implement the batch APIs we need as well, so stay tuned.

(I don't want to blame the Review Board or Bugzilla maintainers for their RESTful web APIs that are giving MozReview a bit of scaling pain. MozReview is definitely abusing them almost certainly in ways that weren't imagined when they were conceived. To their credit, the maintainers of both products have recognized the limitations in their APIs and are working to address them.)

As much as I still love the properties of RESTful design, there are practical limitations and consequences such as what I described above. The older and more experienced I get, the less patience I have for tolerating architecturally pure implementations that sacrifice important properties, such as ease of use and performance.

It's worth noting that many of the properties of RESTful design are applicable to microservices as well. When you create a new service in a microservices architecture, you are creating more overhead for clients that need to speak to multiple services, making changes less transactional and atomic, and making it difficult to consolidate multiple related requests into a higher-level, simpler, and performant API. I recommend Microservice Trade-Offs for more on this subject.

There is a place in the world for RESTful and microservice architectures. And as someone who does a lot of server-side engineering, I sympathize with wanting scalable, fault-tolerant architectures. But like most complex problems, you need to be cognizant of trade-offs. It is also important to not get too caught up with architectural purity if it is getting in the way of delivering a simple, intuitive, and fast product for your users. So, please, follow me down from the ivory tower. The air was cleaner up there - but that was only because it was so distant from the swamp at the base of the tower that surrounds every software project.

January 13, 2016 03:25 PM

January 11, 2016

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1233878] tracking flags don't show up in the view of the bug right after filing
  • [1224001] Add push connector for Aha.io
  • [1237188] add support for servicenow to the 'see also' field
  • [1236955] [form.mdn] Please add component drop-down to custom form
  • [1232913] The attachment links don't look like links
  • [1237185] hide 'cab review' custom field behind a "click through" to direct people to servicenow
discuss these changes on mozilla.tools.bmo.

January 11, 2016 03:13 PM

January 04, 2016

Mark Coté

Review Board history

A few weeks ago, mdoglio found an article from six years ago comparing Review Board and Splinter in the context of GNOME engineering. This was a fascinating read because, without having read this article in advance, the MozReview team ended implementing almost everything the author talked about.

Firstly, I admit the comparison isn’t quite fair when you replace bugzilla.gnome.org with bugzilla.mozilla.org. GNOME doesn’t use attachment flags, which BMO relies heavily on. I haven’t ever submitted a patch to GNOME, but I suspect BMO’s use of review flags makes the review process at least a bit simpler.

The first problem with Review Board that he points out is that the “post-review command-line leaves a lot to be desired when compared to git-bz”. This was something we did early on in MozReview, all be it with Mercurial instead: the ability to push patches up to MozReview with the hg command. Admittedly, we need an extension, mainly because of interactions with BMO, but we’ve automated that setup with mach mercurial-setup to reduce the friction. Pushing commits is the area of MozReview that has seen the fewest complaints, so I think the team did a great job there in making it intuitive and easy to use.

Then we get to what the author describes as “a more fundamental problem”: “a review in Review Board is of a single diff”. As he continues, “More complex enhancements are almost always done as patchsets [emphasis his], with each patch in the set being kept as standalone as possible. … Trying to handle this explicitly in the Review Board user interface would require some substantial changes”. This was also an early feature of MozReview, implemented at the same time as hg push support. It’s a core philosophy baked into MozReview, the single biggest feature that distinguishes MozReview from pretty much every other code-review tool out there. It’s interesting to see that people were thinking about this years before we started down that road.

An interesting aside: he says that “a single diff … [is] not very natural to how people work with Git”. The article was written in 2009, as GitHub was just starting to gain popularity. GitHub users tend to push fix-up commits to address review points rather than editing the original commits. This is at least in part due to limitations present early on in GitHub: comments would be lost if the commit was updated. The MozReview team, in fact, has gotten some push back from people who like working this way, who want to make a bunch of follow-up commits and then squash them all down to a single commit before landing. People who strongly support splitting work into several logical commits and updating them in place actually tend to be Mercurial users now, especially those that use the evolve extension, which can even track bigger changes like commit reordering and insertion.

Back to Review Board. The author moves onto how they’d have to integrate Review Board with Bugzilla: “some sort of single-sign-on across Bugzilla and Review Board”, “a bugzilla extension to link to reviews”, and “a Review Board extension to update bugs in Bugzilla”. Those are some of the first features we developed, and then later improved on.

There are other points he lists that we don’t have, like an “automated process to keep the repository list in Review Board in sync with the 600+ GNOME repositories”. Luckily many people at Mozilla work on just one repo: mozilla-central. But it’s true that we have to add others manually.

Another is “reduc[ing] the amount of noise for bug reporters”, which you get if you confine all patch-specific discussion to the review tool. We don’t have this yet; to ease the transition to Review Board, we currently mirror pretty much everything to Bugzilla. I would really like to see us move more and more of code-related discussion to Review Board, however. Hopefully as more people transition to using MozReview full time, we can get there.

Lastly, I have to laugh a bit at “it has a very slick and well developed web interface for reviewing and commenting on patches”. Clearly we thought so as well, but there are those that prefer the simplicity of Splinter, even in 2015, although probably mostly from habit. Trying to reconcile these two views is very challenging.

January 04, 2016 10:19 PM

October 22, 2015

Byron Jones

moving from bugzilla to mozreview

for the next couple of quarters (at least) i'll be shifting my attention full time from bugzilla to mozreview. this switch involves a change of language, frameworks, and of course teams. i'm looking forward to new challenges.

one of the first things i've done is sketch out a high level architectural diagram of mozreview and its prime dependencies:

mozreview-architectural-diagram

mozreview exists as an extension to reviewboard, using bugzilla for user authentication, ldap to check commit levels, with autoland pushing commits automatically to try (and to mozilla-central soon).  there's mecurial extensions on both the client and server to make pushing things easer, and there are plans to perform static analysis with bots.

October 22, 2015 07:53 PM

Mark Coté

MozReview's Parental issues

As mentioned in my previous post on MozReview, one of the biggest sources of confusion is the way we present the “squashed” diffs, that is, the diff that show all of the changes in a commit series, the sum of all the proposed changes. We also refer to these as “parent” review requests, since they function as something to hold all the commits together. They are stored in MozReview as separate review requests, similar to the individual commits.

The confusion results from several things:

There are a few simple things we can do to fix these problems: use better link names, put a big “This is an overview of the commit series” message, and/or put a warning “You must review individual commits” on the review dialog. But really, we need to step back and think about the way we present the squashed diffs, and if they even make sense as a concept in MozReview.

To reiterate, squashed diffs provide a complete view of a whole commit series. The concept of a commit series doesn’t exist in core Review Board (nor does it exist in many other code-review tools), but it’s central to the idea of the repository-centric approach (like in GitHub pull requests). We added this concept by storing metadata resulting from pushes to tie commit series together with a parent, and we added UI elements like the commits table.

There are three broad ways we can deal with squashed diffs going forward. We need to settle on one and make the associated UI changes to make our model clear to users.

  1. Remove squashed diffs altogether.

    This is the simplest option. Squashed diffs aren’t actually technically necessary, and they can distract reviewers from the individual commits, which is where they should be spending most of their time, since, in most cases, this is how the code will be landing in the main repository. Some other repository-centric review tools, like Critic, don’t have the concept of an overview diff, so there are precedents. However, it might be a bit heavy handed to tell reviewers that they can’t view all the commits as a single diff (at least, without pulling them down locally).

  2. Continue to allow reviews, of some sort, on squashed diffs.

    This is what we have now: reviewers can leave reviews (at the moment, comments only) on squashed diffs. If we decide we want to continue to allow users to leave reviews on squashed diffs, we’ll need to both figure out a better UI to distinguish them from the individual commits and also settle several open questions:

    • Should reviewers be able to grant ship its (i.e. r+s) on squashed diffs? This would imply that the commits probably haven’t been reviewed individually, which would defeat the purpose of a commit-centric system. That said, reviewer time is very important, so we could have a trade off to support more work flows.

    • Conversely, should reviewers be able to leave comments on the parent diff? For simplicity, we could allow reviewers to leave a “ship it” review on a squashed diff that would apply to all commits but force them to leave any comments on diffs on the commits themselves. This would essentially remove the ability to review squashed diffs themselves but would leave the convenience of saying “this is all good”.

    • If we do want to allow review comments on squashed diffs, how should they be consolidated with the reviews on individual commits? Right now, reviews (general comments and comments on diffs) for the squashed diff and all commits are all on separate pages/views. Giving one view into all activity on a commit series would be ideal if we want to support squashed-diff reviews. Arguably, this would be valuable even if we didn’t have reviews on squashed diffs.

    For comparison, GitHub pull requests support this model. There are three tabs in a pull request: “Files changed”, which is the squashed diff; “Commits”, which is a list of commits with links to the individual commit diffs; and “Conversation”, which shows comments on the commits and on the squashed diff (along with other events like updates to the commits). The way they are presented is a little confusing (comments on the squashed diff are just labelled “<user> commented on the diff”, whereas comments on the diffs are of the form “<user> commented on <file> in <commit hash>”), but it is a useful single view. However, note that pull requests do not have the concept of a “ship it” or “r+”, which makes the GitHub interface simpler.

    This approach would support multiple reviewer work flows, but it is also the most complicated, both in terms of UX and technical implementation, and it waters down the philosophy behind MozReview.

  3. Provide read-only overview diffs.

    The third approach is to keep squashed diffs but make them read only. They could be used as reference, to get a big picture of the whole series, but since they are read only, they would be easily distinguishable from commits and would force reviewers to look at the individual commits. This is really just option 1 above, with a reference view of the whole series. It would be more work than option 1 but less than option 2, and would preserve the philosophy.

The MozReview team has been leaning towards option 3. We have a mock-up that strips away a lot of the UI that would be useless in this scenario and makes the intention clear. It’s not the prettiest, but it wouldn’t take too much work to get here:

However, we’d like to hear user feedback before making any decisions. Whichever option we go with, we’ll come up with a plan to get there that ideally will have incremental improvements, depending on the complexity of the full solution, so that we can start to fix things right away.

October 22, 2015 06:29 PM

October 14, 2015

Greg Szorc

Lowering the Barrier to Pushing to MozReview

Starting today, a Mozilla LDAP account with Mercurial SSH access is no longer required to hg push into MozReview to initiate code review with Mozilla projects.

The instructions for configuring your client to use MozReview have been updated to reflect how you can now push to MozReview over HTTP using a Bugzilla API Key for authentication.

This change effectively enables first-time contributors to use MozReview for code review. Before, you had to obtain an LDAP account and configure your SSH client, both of which could be time consuming processes and therefore discourage people from contributing. (Or you could just use Bugzilla/Splinter and not get the benefits of MozReview, which many did.)

I encourage others to update contribution docs to start nudging people towards MozReview over Bugzilla/patch-based workflows (such as bzexport).

Bug 1195856 tracked this feature.

October 14, 2015 12:30 PM

October 13, 2015

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1172418] user fields are cleared when navigating back to a page (eg. after an error)
  • [1211703] help docs not available for User Preferences
  • [1212721] gravatar container for comment 0 can be too wide
  • [1207379] Button edges are hard to see at most zoom-levels, with bmo "experimental user interface"
  • [1204410] Buttons besides 'User Story' overflow if the user story is a one-liner
  • [1181044] linkification isn't applied to the user-story field
  • [1200765] Make login UX mobile friendly to assist mobile authentication workflow
  • [1202281] BMO login being reset for every browser session
  • [1213033] make the TOTP MFA code field type=number on mobile
  • [1029800] Release Tracking Report doesn't return bugs with tracking flags set to --- when searching for "not fixed" values
  • [1178094] Update crash signatures to match new Socorro signature generation
  • [1150358] cannot remove other people from the cc list
  • [1149899] Remember expanded/collapsed sections
  • [1197805] Always show "never email me about this bug" checkbox
  • [1211891] needinfo requests where the current user is the requestee shouldn't be editable
  • [1199089] add support for duo-security

discuss these changes on mozilla.tools.bmo.

October 13, 2015 06:03 AM

October 06, 2015

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1209332] Make the master kick-off bug "Confidential Mozilla Employee Bug" by default
  • [1209971] Suggested reviewers should exclude the current user from the list displayed
  • [1200958] group owners should always be able to view group membership reports for their groups
  • [1196620] support automatic removal of inactive users from groups
  • [1210654] "MozReview Requests" is not shown in the page after submitting a change
  • [1210762] User stories aren't saved as part of the "remember values as bookmarkable template"
  • [1198519] Add link to bug history page to the top-right drop-down menu
  • [1210246] bugzilla.mozilla.org help link is busted
  • [1210690] Display only commits and only relevant data
  • [1205748] Can't mark inaccessible bug dependent on a regression it caused
  • [1164063] show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches
  • [1211750] Changing password with MFA turned on will not work
discuss these changes on mozilla.tools.bmo.

October 06, 2015 07:28 AM

September 30, 2015

Mark Coté

Fixing MozReview's sore spots

MozReview was intentionally released early, with a fairly minimal feature set, and some ugly things bolted onto a packaged code-review tool. The code-review process at Mozilla hasn’t changed much since the project began—Splinter, a slightly fancier UI than dealing with raw diffs, notwithstanding. We knew this would be a controversial subject, with a variety of (invariably strong) opinions. But we also knew that we couldn’t make meaningful progress on a number of long-desired features, like autolanding commits and automatic code analysis, without moving to a modern repository-based review system. We also knew that, with the incredible popularity of GitHub, many developers expect a workflow that involves them pushing up commits for review in a rich web UI, not exporting discrete patches and looking at almost raw diffs.

Rather than spending a couple years off in isolation developing a polished system that might not hit our goals of both appealing to future Mozillians and increasing productivity overall, we released a very early product—the basic set of features required by a push-based, repository-centric code-review system. Ironically, perhaps, this has decreased the productivity of some people, since the new system is somewhat raw and most definitely a big change from the old. It’s our sincere belief that the pain currently experienced by some people, while definitely regrettable and in some cases unexpected, will be balanced, in the long run, by the opportunities to regularly correct our course and reach the goal of a world-class code review-and-submission system that much faster.

And so, as expected, we’ve received quite a bit of feedback. I’ve been noticing a pattern, which is great, because it gives us insight into classes of problems and needs. I’ve identified four categories, which interestingly correspond to levels of usage, from basic to advanced.

Understanding MozReview’s (and Review Board’s) models

Some users find MozReview very opaque. They aren’t sure what many of the buttons and widgets do, and, in general, are confused by the interface. This caught us a little off-guard but, in retrospect, is understandable. Review Board is a big change from Splinter and much more complex. I believe one of the sources of most confusion is the overall review model, with its various states, views, entry points, and exit points. Splinter has the concept of a review in progress, but it is a lot simpler.

We also had to add the concept of a series of related commits to Review Board, which on its own has essentially a patch-based model, similar to Splinter’s, that’s too limited to build on. The relationship between a parent review request and the individual “child” commits is the source of a lot of bewilderment.

Improving the overall user experience of performing a review is a top priority for the next quarter. I’ll explore the combination of the complexity of Review Board and the commit-series model we added in a follow-up post.

Inconveniences and lack of clarity around some features

For users who are generally satisfied by MozReview, at least, enough to use it without getting too frustrated, there are a number of paper cuts and limitations that can be worked around but generate some annoyance. This is an area we knew we were going to have to improve. We don’t yet have parity with Splinter/Bugzilla attachments, e.g. reviewers can’t delegate review requests, nor can they mark specific files as reviewed. There are other areas that we can go beyond Bugzilla, such as being able to land parts of a commit series (this is technically possible in Bugzilla by having separate patches, but it’s difficult to track). And there are specific things that Review Board has that aren’t as useful for us as they could be, like the dashboard.

This will also be a big part of the work in the next quarter (at least).

Inability to use MozReview at all due to technological limitations

The single biggest item here is lack of support for git, particularly a git interface for hg repos like mozilla-central. There are many people interested in using MozReview, but their work flows are based around git using git-cinnabar. gps and kanru did some initial work around this in bug 1153053; fleshing this out into a proper solution isn’t a small task, but it seems clear that we’ll have to finish it regardless before too long, if we want MozReview to be the central code-review tool at Mozilla. We’re still trying to decide how this fits into the above priorities; more users is good, but making existing users happier is as well.

Big-ticket items

As mentioned at the beginning of this post, the main reason we’re building a new review tool is to make it repository-centric, that is, based around commits, not isolated patches. This makes a lot of long-desired tools and features much more feasible, including autoland, automatic static analysis, commit rewriting to automatically include metadata like reviewers, and a bunch of other things.

This has been a big focus for the last few months. We’ve had autoland-to-try for a little while now, and autoland-to-inbound is nearly complete. We have a generic library for static analysis with which we’ll be able to build various review bots. And, of course, the one big feature we started with, the ability push commits to MozReview instead of exporting standalone patches, which by itself is both more convenient and preserves more information.

After autoland-to-inbound we’ll be putting aside other big features for a little while to concentrate on general user experience so that people enjoy using MozReview, but rest assured we’ll be back here to build more powerful workflows for everyone.

September 30, 2015 06:15 PM

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1207926] change treeherder@bots.tld to orangefactor@bots.tld
  • [1204683] Add whoami endpoint
  • [1208135] security not being mailed when bugs change core-security-release state
  • [1199090] add printable recovery 2fa codes
  • [1204623] timestamp on flags should reference the latest updated activity, not the first
  • [1209745] Update get_permissions.html.tmpl to reflect new self-canconfirm process
discuss these changes on mozilla.tools.bmo.

September 30, 2015 07:42 AM

September 23, 2015

Byron Jones

happy bmo push day!

the following changes have been pushed to bugzilla.mozilla.org:

  • [1203991] change the suggested fxos 2fa app (again)
  • [1204704] Backport upstream testsuite changes to test against bug 1202447
  • [1205683] Feature request: STR and regression-range pulldowns
  • [1199087] extend 2fa protection beyond login
  • [1204645] the 'last seen' value in the group membership report should use a profile's last-seen date, not the cookie
discuss these changes on mozilla.tools.bmo.

September 23, 2015 05:24 AM

September 14, 2015

Mark Coté

MozReview Meet-Up

Two weeks ago the MozReview developers got together to do some focussed hacking. It was a great week, we got a lot of stuff done, and we clarified our priorities for the coming months. We deployed several new features and improvements during the week, and we made good progress on several other goals.

For this week, we actually opted to not go to a Mozilla office and instead rented an AirBNB in Montreal—our own little hacker house! It was a great experience. There was no commuting (except for me, since I live here and opted to sleep at my house) and no distractions. One evening when we were particularly in the zone, we ordered a bunch of pizzas instead of going out, although we made sure to take breaks and go outside regularly on all the other days, in order to refresh our minds. Five out of five participants said they’d do it again!

See also dminor’s post about the week, with a bit more detail on what he worked on.

What we pushed out

My main contribution to the week was finally switching MozReview to Bugzilla’s API-key-based authentication-delegation system. I’d been working on this for the last couple months when I found time, and it was a big relief to finally see it in action. I won’t go into detail here, since I’ve already blogged about it and announced it to dev.platform.

gps, working amazingly fast as always, got API-key support working on the command line almost immediately after UI support was deployed. No more storing credentials or login cookies!

Moving on, we know the MozReview’s UI could… stand some improvement, to say the least. So we split off some standalone work from Autoland around clarifying the status of reviews. Now in the commit table you’ll see something like this:

This warrants a bit of explanation, since we’re reusing some terms from Bugzilla but changing them a little.

r+ indicates at least one ship-it, subject to the following:

The reason for the L3 requirement is for Autoland. Since a human won’t necessarily be looking at your patch between the time that a review is granted and the commit landing in tree, we want some checks and balances. If you have L3 yourself, you’re trusted enough to not require an L3 reviewer, and vice versa. We know this is a bit different from how things work right now with regards to the checkin-needed flag, so we’re going to open a discussion on mozilla.governance about this policy.

If one or more reviewers have left any issues, the icon will be the number of open issues beside an exclamation mark on a yellow backgroud. If that or any other reviewer has also left a ship-it (the equivalent of an “r+ with minor issues”), the issue count will switch to an r+ when all the issues have been marked as fixed or dropped.

If there are no open issues nor any ship-its, a grey r? will be displayed.

We’ve also got some work in progress to make it clearer who has left what kind of review that should be live in a week or two.

We also removed the ship-it button. While convenient if you have nothing else to say in your review, it’s caused confusion for new users, who don’t understand the difference between the “Ship It!” and “Review” buttons. Instead, we now have just one button, labelled “Finalize Review”, that lets you leave general comments and, if desired, a ship-it. We plan on improving this dialog to make it easier to use if you really just want to leave just a ship-it and no other comments.

Finally, since our automation features will be growing, we moved the Autoland-to-try button to a new Automation menu.

Where we’re going

As alluded to above, we’re actively working on Autoland and trying to land supporting features as they’re ready. We’re aiming to have this out in a few weeks; more posts to come.

Much of the rest of our plan for the next quarter or two is around user experience. For starters, MozReview has to support the same feature set as Splinter/Bugzilla. This means implementing things like marking files as reviewed and passing your review onto someone else. We also need to continue to improve the user interface by further differentiating between parent review requests, which are intended only to provide an overview of the whole commit series, and child review requests, which is where the actual reviews should happen. Particularly confusing is when there’s only one child, which means the parent and the child are nearly indistinguishable (of course in this case the parent isn’t really needed, but hiding or otherwise doing away with the parent in this case could also be confusing).

And then we have some other big-ticket items like static analysis, which we started a while back; release-management tools; enabling Review Board’s rich emails; partial-series landing (being able to land earlier commits as they are reviewed without confusing MozReview in the next push); and, of course, git support, which is going to be tricky but will undoubtedly make a number of people happy.

Our priorities are currently documented on our new road map, which we’ll update at least once or twice a quarter. In particular, we’ll be going over it again soon once we have the results from our engineering-productivity survey.

September 14, 2015 07:26 PM

July 16, 2015

Greg Szorc

MozReview Statistics July 2015

As of today, ~15.6% of commits landing in Firefox in July have gone through MozReview or have been produced on machines that have used MozReview. This is still a small percentage of overall commits. But, signs are that the percentage is going up. Last month, about half as many commits exhibited the same signature. It's only July 16 and we've already passed the total from June.

What I find interesting is the differences between commits that have gone through MozReview versus the rest. When you look at the diff statistics (a quick proxy of change size), we find that MozReview commits tend to be smaller. The median adds as reported by diff stat (basically lines that were changed) is 12 for MozReview versus 17 elsewhere. The average is 58 for MozReview versus 100 elsewhere. For number of files modified, MozReview averages 2.59 versus elsewhere's 2.71. (These numbers exclude some specific large commits that appeared to be bulk imports of external projects and drove up the non-MozReview figures.)

It's entirely possible the root cause behind the discrepancy is a side-effect of the population of MozReview users: perhaps MozReview users just write smaller commits. However, I'd like to think it's because MozReview makes it easier to manage multiple commits and people are taking advantage of that (this is an explicit design goal of MozReview). Whatever the root cause, I'm glad diffs are smaller. As I've written about before, smaller commits are easier to review and land, thus enabling projects to move faster.

I have a quarterly goal to remove the requirement for a Mozilla LDAP account to push to MozReview. That will allow first time contributors to use MozReview. This will be a huge win, as we can do much more magic in the MozReview world than we can from vanilla Bugzilla (automatic bug filing, automatic reviewer assignment, etc). Unofficially, I'd like to have more than 50% of Firefox commits go through MozReview by the end of the year.

July 16, 2015 02:00 PM

July 07, 2015

Greg Szorc

Publish When Pushing to MozReview

A lot of people contributed some really great feedback about MozReview at Whistler. One of the most frequent requests was for the ability to publish submitted review requests without having to open a browser. I'm pleased to report that as of yesterday, this feature is implemented! If reviewers have been assigned to all your review requests, Mercurial will now prompt you to publish the review requests during hg push. It should just work.

As part of this change, we also introduced more advanced feature negotiation into the handshake between client and server. This means we now have a mechanism for detecting out-of-date client installations. This will enable us to more aggressively drop backwards compatibility (making server-side development easier) while simultaneously ensuring that more people are running modern and hopefully better versions of the client code. This should translate to moving faster and a better experience for everyone.

July 07, 2015 02:55 PM

May 29, 2015

Greg Szorc

Important Changes to MozReview

This was a busy week for MozReview! There are a number of changes people need to be aware of.

Support for Specifying Reviewers via Commit Messages

MozReview will now parse r?gps syntax out of commit messages to set reviewers for pushed commits.

Read the docs for more information, including why we are preferring r? to r=.

Since it landed, a number of workflow concerns have been reported. See the bug tree for bug 1142251 before filing a bug to help avoid duplicates.

Thank Dan Minor for the feature!

Review Attachment/Flag Per Commit

Since the beginning of MozReview, there was one Bugzilla attachment / review flag per commit series. This has changed to one attachment / review flag per commit.

Before, you needed to grant Ship It on the parent/root review request in order to r+ the MozReview review request. Now, you grant Ship It on individual commits and these turn into individual r+ on Bugzilla. To reinforce that reviewing the parent/root review request doesn't do anything meaningful any more, the Ship It button and checkbox have been removed from the parent/root review request.

The new model more closely maps to how code review in Bugzilla has worked at Mozilla for ages. And, it is a superior workflow for future workflows we're trying to enable.

We tried to run a one-time migration script to convert existing parent/root attachments/review flags to per-commit attachments/flags. However, there were issues. We will attempt again sometime next week. In the interim, in-flight review requests may enter an inconsistent state if they are updated. If a new push is performed, the old parent/root attachment/review flag may linger and per-commit attachments/flags will be created. This could be confusing. The workaround is to manually clear the r? flag from the parent/root attachment or wait for the migration script to run in a few days.

Mark Côté put in a lot of hard work to make this change happen.

r? Flags Cleared After Review

Before, submitting a review without granting Ship It wouldn't do anything to the r? flag: the r? flag would linger.

Now, submitting review without granting Ship It will clear the r? flag. We think the new default is better for the majority of users. However, we recognize it isn't always wanted. There is a bug open to provide a checkbox to keep the r? flag present.

Metadata Added to Changesets

If you update to the tip of the version-control-tools repository (you should do this every week or so to stay fresh - use mach mercurial-setup to do this automatically), metadata will automatically be added to commits when working with MozReview-enabled repositories.

Specifically, we insert a hidden, unique, random ID into every changeset. This ID can be used to map commits to each other. We don't use this ID yet. But we have plans.

A side-effect of this change is that you can no longer push to MozReview if you have uncommitted local changes. If this is annoying, use hg shelve and hg unshelve to create and undo temporary commits. If this is too annoying, complain by filing a bug and we can look into doing this automatically as part of pushing.

What's Next?

We're actively working on more workflow enhancements to make MozReview an even more compelling experience.

I'm building a web service to query file metadata from moz.build files. This will allow MozReview to automatically file bugs in proper components based on what files changed. Once code reviewer metadata is added to moz.build files, it will enable us to assign reviewers automatically as well. The end goal here is to lower the number of steps needed to turn changed code into a landing. This will be useful when we turn GitHub pull requests into MozReview review requests (random GitHub contributors shouldn't need to know who to flag for review, nor should they be required to file a bug if they write some code). Oh year, we're working on integrating GitHub pull requests.

Another area of focus is better commit tracking and partially landed series. I have preliminary patches for automatically adding review URL annotations to commit messages. This will enable people to easily go from commit (message) to MozReview, without having to jump through Bugzilla. This also enables better commit tracking. If you e.g. perform complicated history rewriting, the review URL annotation will enable the MozReview server to better map previously-submitted commits to existing review requests. Partially landed series will enable commits to land as soon as they are reviewed, without having to wait on the entire series. It's my strong belief that if a commit is granted review, it should land as soon as possible. This helps prevent bit rot of ready-to-land commits. Landings also make people feel better because you feel like you've accomplished something. Positive feedback loops are good.

Major work is also being done to overhaul the web UI. The commit series view (which is currently populated via XHR) will soon be generated on the server and served as part of the page. This should make pages load a bit faster. And, things should be prettier as well.

And, of course, work is being invested into Autoland. Support for submitting pushes to Try landed a few weeks ago. Having Autoland actually land reviewed commits is on the radar.

Exciting times are ahead. Please continue to provide feedback. If you see something, say something.

May 29, 2015 04:20 PM

January 27, 2015

Greg Szorc

Commit Part Numbers and MozReview

It is common for commit messages in Firefox to contains strings like Part 1, Part 2, etc. See this push for bug 784841 for an extreme multi-part example.

When code review is conducted in Bugzilla, these identifiers are necessary because Bugzilla orders attachments/patches in the order they were updated or their patch title (I'm not actually sure!). If part numbers were omitted, it could be very confusing trying to figure out which order patches should be applied in.

However, when code review is conducted in MozReview, there is no need for explicit part numbers to convey ordering because the ordering of commits is implicitly defined by the repository history that you pushed to MozReview!

I argue that if you are using MozReview, you should stop writing Part N in your commit messages, as it provides little to no benefit.

I, for one, welcome this new world order: I've previously wasted a lot of time rewriting commit messages to reflect new part ordering after doing history rewriting. With MozReview, that overhead is gone and I barely pay a penalty for rewriting history, something that often produces a more reviewable series of commits and makes reviewing and landing a complex patch series significantly easier.

January 27, 2015 08:17 PM

January 24, 2015

Greg Szorc

Automatic Python Static Analysis on MozReview

A bunch of us were in Toronto last week hacking on MozReview.

One of the cool things we did was deploy a bot for performing Python static analysis. If you submit some .py files to MozReview, the bot should leave a review. If it finds violations (it uses flake8 internally), it will open an issue for each violation. It also leaves a comment that should hopefully give enough detail on how to fix the problem.

While we haven't done much in the way of performance optimizations, the bot typically submits results less than 10 seconds after the review is posted! So, a human should never be reviewing Python that the bot hasn't seen. This means you can stop thinking about style nits and start thinking about what the code does.

This bot should be considered an alpha feature. The code for the bot isn't even checked in yet. We're running the bot against production to get a feel for how it behaves. If things don't go well, we'll turn it off until the problems are fixed.

We'd like to eventually deploy C++, JavaScript, etc bots. Python won out because it was the easiest to integrate (it has sane and efficient tooling that is compatible with Mozilla's code bases - most existing JavaScript tools won't work with Gecko-flavored JavaScript, sadly).

I'd also like to eventually make it easier to locally run the same static analysis we run in MozReview. Addressing problems locally before pushing is a no-brainer since it avoids needless context switching from other people and is thus better for productivity. This will come in time.

Report issues in #mozreview or in the Developer Services :: MozReview Bugzilla component.

January 24, 2015 11:30 PM

January 16, 2015

Greg Szorc

Bugzilla and the Future of Firefox Development

Bugzilla has played a major role in the Firefox development process for over 15 years. With upcoming changes to how code changes to Firefox are submitted and reviewed, I think it is time to revisit the central role of Bugzilla and bugs in the Firefox development process. I know this is a contentious thing to say. Please, gather your breath, and calmly read on as I explain why I believe this.

The current Firefox change process defaults to requiring a Bugzilla bug for everything. It is rare (and from my experience frowned upon) when a commit to Firefox doesn't reference a bug number. We've essentially made Bugzilla and a bug prerequisites for changing anything in the Firefox version control repository. For the remainder of this post, I'm going to say that we require a bug for any change, even though that statement isn't technically accurate. Also, when I say Bugzilla, I mean bugzilla.mozilla.org, not the generic project.

Before I go on, let's boil the Firefox change process down to basics.

At the heart of any change to the Firefox source repository is a diff. The diff (a representation of the differences between a set of files) is the smallest piece of data necessary to represent a change to the Firefox code. I argue that anything more than the vanilla diff is overhead and could contribute to process debt. Now, there is some essential overhead. Version control tools supplement diffs with metadata, such as the author, commit message, and date. Mozilla has also instituted a near-mandatory code review policy, where changes need to be signed off by a set of trusted individuals. I view both of these additions to the vanilla diff as essential for Firefox development and non-negotiable. Therefore, the bare minimum requirements for changing Firefox code are a diff plus metadata (a commit/patch) and (almost always) a review/sign-off. That's it. Notably absent from this list is a Bugzilla bug. I argue that a bug is not strictly required to change Firefox. Instead, we've instituted a near-universal policy that we should have bugs. We've chosen to add overhead and process debt - interaction with Bugzilla - to our Firefox change process.

Now, this choice to require all changes be associated with bugs has its merits. Bugs provide excellent anchor points for historical context and for additional information after the change has been committed and is forever set in stone in the repository (commits are immutable in Mercurial and Git and you can't easily attach metadata to the commit after the fact). Bugs are great to track relationships between different problems or units of work. Bugs can even be used to track progress towards a large feature. Bugzilla components also provide a decent mechanism to follow related activity. There's also a lot of tooling and familiar process standing on top of the Bugzilla platform. There's a lot to love here and I don't want diminish the importance of all these things.

When I look to the future, I see a world where the current, central role of Bugzilla and bugs as part of the Firefox change process begin to wane. I see a world where the benefits to maintaining our current Bugzilla-centric workflow start to erode and the cost of maintaining it becomes higher and harder to justify. You actually don't have to look too far into the future: that world is already here and I've already started to feel the pains of it.

A few days ago, I blogged about GitHub and its code first approach to change. That post was spun off from an early draft of this post (as were the posts about Firefox contribution debt and utilizing GitHub for Firefox development). I wanted to introduce the concept of code first because it is central to my justification for changing how we do things. In summary, code first capitalizes on the fact that any change to software involves code and therefore puts code front and center in the change process. (In hindsight, I probably should have used the term code centric, because that's how I want people to think about things.) So how does code first relate to Bugzilla and Firefox development?

Historically, code review has occurred in Bugzilla: upload a patch to Bugzilla, ask for review, and someone will look at it. And, since practically every change to Firefox requires review, you need a bug in Bugzilla to contain that review. Thus, one way to view a bug is as a vehicle for code review. Not every bug is just a code review, of course. But a good number of them are.

The only constant is change. And the way Mozilla conducts code review for changes to Firefox (and other projects) is changing. We now have MozReview, a code review tool that is not Bugzilla. If we start accepting GitHub pull requests, we may perform reviews exclusively on GitHub, another tool that is not Bugzilla.

(Before I go on, I want to quickly point out that MozReview is nowhere close to its final form. Parts of MozReview are pretty bad right now. The maintainers all know this and we have plans to fix it. We'll be in Toronto all of next week working on it. If you don't think you'll ever use it because parts are bad today, I ask you to withhold judgement for a few more months.)

In case you were wondering, the question on whether Bugzilla should always be used for code review for Firefox has been answered and that answer is no. People, including maintainers of Bugzilla, realized that better-than-Splinter/Bugzilla code review tools exist and that continuing to invest time to develop Bugzilla/Splinter into a best-in-class code review tool would be better spent integrating Bugzilla with an existing tool. This is why we now have a Review Board based code review tool - MozReview - integrated with Bugzilla. If you care about code quality and more powerful workflows, you should be rejoicing at this because the implementation of code review in Bugzilla does not maximize optimal outcomes.

The world we're moving to is one where code review occurs outside of Bugzilla. This raises an important question: if Bugzilla was being used primarily as a vehicle for code review, what benefit and/or role should Bugzilla play when code review is conducted outside of Bugzilla?

I posit that there are a class of bugs that won't need to exist going forward because bugs will provide little to no value. Put another way, I believe that a growing number of commits to the Firefox repository won't reference bugs.

Come with me on a journey to the future.

MozReview is purposefully being designed in a code and repository centric way. To initiate the formal process for considering a change to code, you push to a Mercurial (or Git!) repository. This could be directly to Mozilla's review repository. If I have my way, this could even be kicked off by submitting a pull request on GitHub or Bitbucket. No Bugzilla attachment uploading here: our systems talk in terms of repositories and commits. Again, this is by design: we don't want submitting code to Mozilla to be any harder than hg push or git push so as to not introduce process debt. If you have code, you'll be able to send it to us.

In the near future, MozReview will stop cross-posting detailed review updates to Bugzilla. Instead, we'll use Review Board's e-mail feature to send its flavor of emails. These will have rich HTML content (or plain text if you insist) and will provide a better experience than Bugzilla ever will. We'll adopt the model of tools like Phabricator and GitHub and only post summaries or links of activity, not full content, to bugs. You may be familiar with the concept as applied to the web: it's called hyperlinking.

Work is being invested into Autoland. Autoland is an automated landing queue that pushes/lands commits semi-automatically once they are ready (have review, pass automation, etc). Think of Autoland as a bot that does all the labor intensive and menial actions around pushing that you do now. I believe Autoland will eventually handle near 100% of pushes to the Firefox repository. And, if I have my way, Autoland will result in the abolishment of integration branches and merge commits in the Firefox repository. Good riddance.

MozReview and Autoland will be highly integrated. MozReview will be the primary user interface for interacting with Autoland. (Some of this should be in place by the end of the quarter.)

In this world, MozReview and its underlying version control repositories essentially become a database of all submitted, pending, and discarded commits to Firefox. The metaphorical primary keys of this database are not bug numbers: they are code/commits. (Code first!) Some of the flags stored in this database tell Autoland what it should do. And the MozReview user interface (and API) provide a mechanism into controlling those flags.

Landing a change in Firefox will be initiated by a simple action such as clicking a checkbox in MozReview. (That could even be the Grant Review checkbox.) Commits cleared for landing will be picked up by Autoland and eventually automatically pushed to the Firefox repository (assuming the build and test automation is happy, of course). Once Autoland takes control, humans are just passengers. We won't be bothered with menial tasks like updating the commit message to reflect a review was performed: this will happen automatically inside MozReview or Autoland. (Although, there's a chance we may adopt some PGP-based signing to more strongly convey review for some code changes in order to facilitate stronger auditing and trust guarantees. Stay tuned.) Likewise, if a commit becomes associated with a bug, we can add that metadata to the commit before it is landed, no human involvement necessary beyond specifying the link in the MozReview web UI (or API). Autoland/MozReview will close review requests and/or bugs automatically. (Are you excited about performing less work yet?)

When commits are added to MozReview, MozReview will read metadata from the repository they came from to automatically determine an appropriate reviewer. (We plan to leverage moz.build files for this in the case of Firefox.) This should eliminate a lot of process debt around choosing a reviewer. Similar metadata will also be used to determine what Bugzilla component a change is related to, static analysis rules to use to critique the phsyical structure of the change, and even automation jobs that should be executed given the set of files that changed. The use of this metadata will erode significant process debt around the change contribution workflow.

As commits are pushed into MozReview/Autoland, the systems will be intelligent about automatically tracking dependencies and facilitating complex development workflows that people run into on a daily basis.

If I create a commit on top of someone else's commit that hasn't been checked in yet, MozReview will detect the dependency between my changes and the parent ones. This is an advantage of being code first: by interfacing with repositories rather than patch files, you have an explicit dependency graph embedded in the repository commit DAG that can be used to aid machines in their activities.

It will also be possible to partially land a series of commits. If I get review on the first 5 of 10 commits but things stall on commit 6, I can ask Autoland to land the already-reviewed commits so they don't get bit rotted and so you have partial progress (psychological studies show that a partial reward for work results in greater happiness through a sense of accomplishment).

Since initiating actions in MozReview is light weight (just hg push), itch scratching is encouraged. I don't know about you, but in the course of working on the Firefox code base, I frequently find myself wanting to make small, 15-30s changes to fix something really minor. In today's world, the overhead for these small changes is often high. I need to upload a separate patch to Bugzilla. Sometimes I even need to create a new bug to hold that patch. If that patch depends on other work I did, I need to set up bug dependencies then worry about landing everything in the right order. All of a sudden, the overhead isn't worth it and my positive intentions go unacted on. Multiplied by hundreds of developers over many years, and you can imagine the effect on software quality. With MozReview, the overhead for itch scratching like this is minor. Just make a small commit, push, and the system will sort everything out. (These small commits are where I think a bugless process really shines.)

This future world revolves around code and commits and operations on them. While MozReview has review in its name, it's more than a review tool: it's a database and interface to code and its state.

In this code first world, Bugzilla performs an ancillary role. Bugzilla is still there. Bugs are still there. MozReview review requests and commits link to bugs. But it is the code, not bugs, that are king. If you want to do anything with code, you interact with the code tools. And Bugzilla is not one of them.

Another way of looking at this is that nearly everything involving code or commits becomes excised from Bugzilla. This would relegate Bugzilla to, well, an issue/bug tracker. And - ta da - that's something it excels at since that's what it was originally designed to do! MozReview will provide an adequate platform to discuss code (a platform that Bugzilla provides today since it hosts code review). So if not Bugzilla tools are handling everything related to code, do you really need a bug any more?

This is the future we're trying to build with MozReview and Autoland. And this is why I think bugs and Bugzilla will play a less central role in the development process of Firefox in the future.

Yes, there are many consequences and concerns about making this shift. You would be rational to be skeptical and doubt that this is the right thing to do. I have another post in the works that attempts to outline some common conerns and propose solutions to many of them. Before writing a long comment pointing out every way in which this will fail to work, I encourage you to wait for that post to be published. Stay tuned.

January 16, 2015 10:50 AM

December 04, 2014

Greg Szorc

The Mozlandia Tree Outage and Code Review

You may have noticed the Firefox trees were closed for the better part of yesterday.

Long story short, a file containing URLs for Firefox installers was updated to reference https://ftp.mozilla.org/ from http://download-installer.cdn.mozilla.net/. The original, latter host is a CDN. The former is not. When thousands of clients started hitting ftp.mozilla.org, it overwhelmed the servers and network, causing timeouts and other badness.

The change in question was accidental. It went through code review. From a code change standpoint, procedures were followed.

It is tempting to point fingers at the people involved. However, I want us to consider placing blame elsewhere: on the code review tool.

The diff being reviewed was to change the Firefox version number from 32.0 to 32.0.3. If you were asked to review this patch, I'm guessing your eyes would have glanced over everything in the URL except the version number part. I'm pretty sure mine would have.

Let's take a look at what the reviewer saw in Bugzilla/Splinter (click to see full size):

And here is what the reviewer would have seen had the review been conducted in MozReview:

Which tool makes the change of hostname more obvious? Bugzilla/Splinter or MozReview?

MozReview's support for intraline diffs more clearly draws attention to the hostname change. I posit that had this patch been reviewed with MozReview, the chances are greater we wouldn't have had a network outage yesterday.

And it isn't just intraline diffs that make Splinter/Bugzilla a sub-optimal code review tool. I recently blogged about the numerous ways that using Bugzilla for code revie results in harder reviews and buggier code. Every day we continue using Bugzilla/Splinter instead of investing in better code review tools is a day severe bugs like this can and will slip through the cracks.

If there is any silver lining to this outage, I hope it is that we need to double down on our investment in developer tools, particularly code review.

December 04, 2014 08:40 AM