630537 - List hidden builders on TBPL

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Chris Cooper [:coop] (he/him)]

As part of the work to stop using tinderbox server for any part of Firefox
work, we need a list of which builders that are hidden in TBPL. We'll also need a UI for hiding/un-hiding those builders.

Comment 1

[Markus Stange [:mstange]]

The Build API doesn't publish a list of all builders, so whatever configuration UI we end up having can't just be a list of checkboxes; it needs to be a text-editable list of names of the hidden builders.

I think the ideal UI should provide these things:
 1. a way to state reasons for hiding a certain builder
 2. give control over the order of the list, so you can group by reason
 3. preserve history of who changed what when and why

A plain text blob in a big text field where each line is a buildername (and which allows for comments) would satisfy 1 and 2 nicely. Point 3 calls for some kind of version control system, so how about this:
Let's create a repo at (say) hg.mozilla.org/build/treestate-config/ with one text file per tree, e.g. hiddenbuilds-mozilla-central.txt. To hide or unhide a build, you'd have to clone that repo, change the right file and push. TBPL would then request http://hg.mozilla.org/build/treestate-config/raw-file/tip/hiddenbuilds-mozilla-central.txt on every refresh.
Does that sound like a useful proposal? This solution wouldn't give us an HTML UI, but maybe that's not a priority.

What do people who have actually used the Tinderbox hidden builds administration page think of this? How much would you miss a web page UI? Any other ideas?

Comment 2

[Arpad Borsos [:Swatinem]]

(In reply to comment #1)
> Let's create a repo at (say) hg.mozilla.org/build/treestate-config/ with one
> text file per tree

Interesting Idea, maybe that also fits bug 630534 comment 2.

Comment 3

[Phil Ringnalda (:philor)]

I could probably live with that, but I would hate it, and I would bitch about it every single time I used it, loudly.

My typical hiding is "someone started running a suite before it was actually ready to use, open admintree, cmd+f, jetpack, click, next, click, next, click, next, click, next, click, next, click, next, click, next, click" or "someone told me that all Android tests were going to be failing on this tree for the next couple days, find that chunk, click click click click click click click click click click click click click click." It's vanishingly rare that I'm hiding a single build on a single tree starting from looking at finished results on tbpl where I could easily copy a single buildername.

Comment 4

[Markus Stange [:mstange]]

OK, that helps. Back to the drawing board.

Comment 5

[Markus Stange [:mstange]]

Attachment: v1

I guess I kind of got carried away.

This patch doesn't go the Hg repository route, it goes the MongoDB route and adds some UI. Builder names are saved in the "builders" table and editing history is saved in the "builderhistory" table. The UI shows a list of all encountered builder names and has a filter text field (like about:config). It's purely mouse-based and thus very inaccessible.
History isn't displayed in the UI yet.

I haven't converted TBPL's filtering to the new hidden builder list yet. TBPL still uses Config.hiddenBuilds.

Comment 6

[Markus Stange [:mstange]]

Attachment: v2

Comment 7

[Armen [:armenzg]]

Do you think we could add a feature of disabling a builder for a certain amount of time? Let's say until next scheduled merge day.

If too difficult and we should forget it or if it is worth doing it but in another bug let me know.

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #7)
> Do you think we could add a feature of disabling a builder for a certain
> amount of time? Let's say until next scheduled merge day.

I don't know... have you ever needed such a feature? I'm having a hard time thinking of a concrete use case.

Also note that the hidden builder list here only makes them invisible on TBPL. It doesn't stop them from actually building (but you probably knew that).

Comment 9

[Markus Stange [:mstange]]

Attachment: quick screencast of UI that the patch implements

Comment 10

[Phil Ringnalda (:philor)]

ui-r=me, and thanks :)

Comment 11

[Arpad Borsos [:Swatinem]]

Comment on attachment 535487 [details] [diff] [review]
v2

Review of attachment 535487 [details] [diff] [review]:
-----------------------------------------------------------------

I guess catlee needs to take a look at dataimport/import-buildbot-data.py

I like the patch but I would like to see another version with the suggested mongo changes and with proper multi-word filtering.
I would also like to get your comment on the interaction with tinderboxDataLoader (which is not a good name in the future Buildbot world anyway).

Also, I thought about merging AddCommentUI into UI, simply because it always breaks when you are doing some invasive change to UI or data handling. This new AdminUI doesn’t seem to have a lot of interactions with other parts so it is probably more resistant to breakage. Avoiding to bloat up UI by another 300 LOC is also good since it is far too large already. What’s your opinion?

::: js/BuildbotDBUser.js
@@ +27,5 @@
>  
> +  openAdminUI: function BuildbotDBUser_openAdminUI(tree) {
> +    var branch = Config.treeInfo[tree].buildbotBranch;
> +    HiddenBuildsAdminUI.open(branch);
> +  },

I get the degree of abstraction here, but I’m not very happy to see purely data-parsing oriented classes doing UI interactions.

::: js/HiddenBuildsAdminUI.js
@@ +29,5 @@
> +      self._adjustSelectionToFilter();
> +      self._unhideSelectedBuilders();
> +    });
> +    $("#builderList").delegate("li", "click", function builderListDoubleClick(e) {
> +      if (e.detail == 2)

I had to read up what that detail meant. I think binding the DOM3 dblclick event would be more clear. I’m not sure about the browser support however.

I’m also not sure if it would be a good idea to toggle the state on *all* selected builders when you double-click a single one of them.

But does this method work at all? As far as I read it, the mousedown handler is invoked first, selecting a single builder, then the double-click is triggered in which you toggle its state. Maybe I’m missing something here. I need to test this live.

@@ +58,5 @@
> +      self._updateSelection();
> +      $(e.target).focus();
> +
> +      $("#builderList > li").bind("mouseenter", function mouseDraggedOver() {
> +        self._selectedBuilders = preDragSelection.slice();

This preDragSelection confused me a bit until I figured out it is used to deselect items when you are making the drag-selection window smaller. A comment should make this clear.

@@ +59,5 @@
> +      $(e.target).focus();
> +
> +      $("#builderList > li").bind("mouseenter", function mouseDraggedOver() {
> +        self._selectedBuilders = preDragSelection.slice();
> +        self._selectFromFocusedTo(this, e.metaKey || e.ctrlKey);

Toggle on Ctrl+Drag is nice!

@@ +106,5 @@
> +        var startIndex = Math.min(index1, index2);
> +        var endIndex = Math.max(index1, index2);
> +        for (var i = startIndex; i <= endIndex; i++) {
> +          if (i == index1)
> +            continue; // exclude the focused item

Just name the vars focusedIndex and clickedIndex, that would also make this comment obsolete.
Well clicked is not a good name either, since it is also used for drag-selection.
Maybe fromIndex/toIndex? Meaning *from* the previously selected item *to* the now selected one. So from/to has semantic meaning whereas start/end maps that to the actual list order.

@@ +145,5 @@
> +      var name = builder.buildername || builder.name;
> +      var search = name.toLowerCase() +
> +        " " + (builder.newHidden ? "hidden" : "visible") +
> +        (builder.hidden != builder.newHidden ? " changed" : "");
> +      return search.indexOf(filter) != -1;

Including the hidden state and changed state in the matching string is a good idea, so you can filter for hidden builders.

But you should probably split the filter by whitespace and test each substring separately. So you can filter for, say "jetpack hidden", and that should also match "jetpack blahblah hidden" which is not the case right now.

@@ +227,5 @@
> +
> +  _unhideSelectedBuilders: function HiddenBuildsAdminUI__unhideSelectedBuilders() {
> +    var self = this;
> +    self._selectedBuilders.forEach(function (name) {
> +      self._getBuilder(name).newHidden = !self._isSelected(name);

_isSelected just makes an indexOf check which is redundant since you are iterating over _selectedBuilders.
Just set this true/false directly here and above to remove that confusion.

@@ +249,5 @@
> +      if (builder.hidden != builder.newHidden) {
> +        updates.push({
> +          name: builder.name,
> +          action: builder.newHidden ? "hide" : "unhide"
> +        });

Maybe a dictionary of name: action pairs would be better suited?

::: php/getBuilderHistory.php
@@ +17,5 @@
> +$mongo->tbpl->builderhistory->ensureIndex(array('name' => true));
> +$result = $mongo->tbpl->builderhistory->find(
> +            array('name' => $_GET['name']),
> +            array('_id' => 0, 'name' => 0, 'ip' => 0));
> +echo json_encode(iterator_to_array($result->sort(array('date' => true)), false)) . "\n";

Again, you are not using MongoDB properly here.
I know the paradigm shift from relational DBs is hard. I guess a good rule of thumb is this: If you have 1<->n cardinality, as in one builder with many history entries, it is better to just save the history as a subarray of the builder instead of creating a new collection for those as you would do in a relational DB.

::: php/updateBuilders.php
@@ +51,5 @@
> +  $current = $mongo->tbpl->builders->findOne(
> +    array('name' => $name), array('hidden' => 1));
> +  if ($current === NULL)
> +    continue;
> +  $currentlyHidden = isset($current['hidden']) && $current['hidden'];

!empty()

@@ +55,5 @@
> +  $currentlyHidden = isset($current['hidden']) && $current['hidden'];
> +  if (($currentlyHidden && $action != 'unhide') ||
> +      (!$currentlyHidden && $action != 'hide'))
> +    continue;
> +  $newHidden = $action == 'hide' ? 1 : 0;

Make this a bool.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 535487 [details] [diff] [review]
v2

Catlee can review the next version when I've fixed the database format.

Comment 13

[Markus Stange [:mstange]]

Attachment: v3

(In reply to comment #11)
> I would also like to get your comment on the interaction with
> tinderboxDataLoader (which is not a good name in the future Buildbot world
> anyway).

I can't think of a good solution to that problem at the moment, so I'd like to leave it for a follow-up bug. We'll have to rethink our whole naming stuff anyway - even the name TBPL itself loses its meaning once we have the Buildbot backend.


> Also, I thought about merging AddCommentUI into UI, simply because it always
> breaks when you are doing some invasive change to UI or data handling.

Yes please :-)
Not in this bug, though.

> This
> new AdminUI doesn’t seem to have a lot of interactions with other parts so
> it is probably more resistant to breakage. Avoiding to bloat up UI by
> another 300 LOC is also good since it is far too large already. What’s your
> opinion?

I don't know. The original idea was to use an MVC style model, but things like HiddenBuildsAdminUI don't really honor that model, except if we break it up into seperate data / UI pieces... should we?

> ::: js/BuildbotDBUser.js
> @@ +27,5 @@
> >  
> > +  openAdminUI: function BuildbotDBUser_openAdminUI(tree) {
> > +    var branch = Config.treeInfo[tree].buildbotBranch;
> > +    HiddenBuildsAdminUI.open(branch);
> > +  },
> 
> I get the degree of abstraction here, but I’m not very happy to see purely
> data-parsing oriented classes doing UI interactions.

Me neither, but I'd like to worry about that another time.
> 
> ::: js/HiddenBuildsAdminUI.js
> @@ +29,5 @@
> > +      self._adjustSelectionToFilter();
> > +      self._unhideSelectedBuilders();
> > +    });
> > +    $("#builderList").delegate("li", "click", function builderListDoubleClick(e) {
> > +      if (e.detail == 2)
> 
> I had to read up what that detail meant. I think binding the DOM3 dblclick
> event would be more clear.

Oh, good, that's what I was looking for. (I had only tried "doubleclick" and then not bothered to look up the real event name.)

Browser support seems to be sufficiently good:
http://www.quirksmode.org/dom/events/click.html#t06

> I’m also not sure if it would be a good idea to toggle the state on *all*
> selected builders when you double-click a single one of them.
> 
> But does this method work at all? As far as I read it, the mousedown handler
> is invoked first, selecting a single builder, then the double-click is
> triggered in which you toggle its state.

Right, that's exactly what happens. So a double click destroys the selection and only toggles one item.
Except when you hold shift or ctrl. Just don't do that.

> Maybe I’m missing something here. I
> need to test this live.

Here you are: http://tests.themasta.com/mocktbpl/?usebuildbot=1&rev=75907ce0bcbb
(I don't have a server that can do Mongo / Python / etc., so I've hardcoded all php script return values.)

> @@ +58,5 @@
> > +      self._updateSelection();
> > +      $(e.target).focus();
> > +
> > +      $("#builderList > li").bind("mouseenter", function mouseDraggedOver() {
> > +        self._selectedBuilders = preDragSelection.slice();
> 
> This preDragSelection confused me a bit until I figured out it is used to
> deselect items when you are making the drag-selection window smaller. A
> comment should make this clear.

Added a comment.

> @@ +106,5 @@
> > +        var startIndex = Math.min(index1, index2);
> > +        var endIndex = Math.max(index1, index2);
> > +        for (var i = startIndex; i <= endIndex; i++) {
> > +          if (i == index1)
> > +            continue; // exclude the focused item
> 
> Just name the vars focusedIndex and clickedIndex, that would also make this
> comment obsolete.
> Well clicked is not a good name either, since it is also used for
> drag-selection.
> Maybe fromIndex/toIndex? Meaning *from* the previously selected item *to*
> the now selected one. So from/to has semantic meaning whereas start/end maps
> that to the actual list order.

Good idea! I went with fromIndex/toIndex.

> @@ +145,5 @@
> > +      var name = builder.buildername || builder.name;
> > +      var search = name.toLowerCase() +
> > +        " " + (builder.newHidden ? "hidden" : "visible") +
> > +        (builder.hidden != builder.newHidden ? " changed" : "");
> > +      return search.indexOf(filter) != -1;
> 
> Including the hidden state and changed state in the matching string is a
> good idea, so you can filter for hidden builders.
> 
> But you should probably split the filter by whitespace and test each
> substring separately. So you can filter for, say "jetpack hidden", and that
> should also match "jetpack blahblah hidden" which is not the case right now.

Done.

> @@ +227,5 @@
> > +
> > +  _unhideSelectedBuilders: function HiddenBuildsAdminUI__unhideSelectedBuilders() {
> > +    var self = this;
> > +    self._selectedBuilders.forEach(function (name) {
> > +      self._getBuilder(name).newHidden = !self._isSelected(name);
> 
> _isSelected just makes an indexOf check which is redundant since you are
> iterating over _selectedBuilders.
> Just set this true/false directly here and above to remove that confusion.

Done, good catch.

> @@ +249,5 @@
> > +      if (builder.hidden != builder.newHidden) {
> > +        updates.push({
> > +          name: builder.name,
> > +          action: builder.newHidden ? "hide" : "unhide"
> > +        });
> 
> Maybe a dictionary of name: action pairs would be better suited?

Done.

> ::: php/getBuilderHistory.php
> @@ +17,5 @@
> > +$mongo->tbpl->builderhistory->ensureIndex(array('name' => true));
> > +$result = $mongo->tbpl->builderhistory->find(
> > +            array('name' => $_GET['name']),
> > +            array('_id' => 0, 'name' => 0, 'ip' => 0));
> > +echo json_encode(iterator_to_array($result->sort(array('date' => true)), false)) . "\n";
> 
> Again, you are not using MongoDB properly here.
> I know the paradigm shift from relational DBs is hard. I guess a good rule
> of thumb is this: If you have 1<->n cardinality, as in one builder with many
> history entries, it is better to just save the history as a subarray of the
> builder instead of creating a new collection for those as you would do in a
> relational DB.

Thanks. Done.

> ::: php/updateBuilders.php
> @@ +51,5 @@
> > +  $current = $mongo->tbpl->builders->findOne(
> > +    array('name' => $name), array('hidden' => 1));
> > +  if ($current === NULL)
> > +    continue;
> > +  $currentlyHidden = isset($current['hidden']) && $current['hidden'];
> 
> !empty()
> 
> @@ +55,5 @@
> > +  $currentlyHidden = isset($current['hidden']) && $current['hidden'];
> > +  if (($currentlyHidden && $action != 'unhide') ||
> > +      (!$currentlyHidden && $action != 'hide'))
> > +    continue;
> > +  $newHidden = $action == 'hide' ? 1 : 0;
> 
> Make this a bool.

Done and done.

Comment 14

[Markus Stange [:mstange]]

Attachment: part 2, v1: Use the dynamic list for filtering

Comment 15

[Arpad Borsos [:Swatinem]]

Comment on attachment 536586 [details] [diff] [review]
v3

Review of attachment 536586 [details] [diff] [review]:
-----------------------------------------------------------------

Well yes, in the classic textbook MVC the controller handles all the events and such, so we are far from that. AdminUI is pretty self-contained, so ripping it apart would just create breakage.
Anyhow, we need to somehow figure out how to keep Data decoupled, in the future.
Be sure to r?catlee for the python parts.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 536586 [details] [diff] [review]
v3

Requesting review from catlee on the import-buildbot-data.py change:
The UI needs a list of known builders per tree, so this script just adds unknown builders to a db table whenever it encounters them in the run data.

Comment 17

[Arpad Borsos [:Swatinem]]

Comment on attachment 536587 [details] [diff] [review]
part 2, v1: Use the dynamic list for filtering

Review of attachment 536587 [details] [diff] [review]:
-----------------------------------------------------------------

::: php/getRevisionBuilds.php
@@ +14,5 @@
>  if (!isset($_GET['rev']) || !$_GET['rev'])
>    die("No revision set.");
>  
>  $mongo = new Mongo();
> +$noIgnore = isset($_GET['noignore']) && $_GET['noignore'] == '1';

move this to the other input validation code above.

Comment 18

[Chris AtLee [:catlee]]

Comment on attachment 536586 [details] [diff] [review]
v3

Looks ok to me