1151337 - Perfherder graphs should use OptionCollectionModel

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

Currently we use the Angular $http function to get the option collection information (which we use to distinguish pgo vs. non-pgo builds, among other things). This isn't terrible, but it would be better to use the Treeherder option collection model API which hides some of the complexity and internal details.

You only need to modify the treeherder-ui module to fix this bug (see the user story field for information on how to set that up).

Steps to implementation:

1. Include the js file by adding `<script src="js/models/option_collection.js"></script>` to perf.html just before we include perf.js (currently that's here: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/perf.html#L34)

2. Use dependency injection to add the ThOptionCollectionModel class to the perf controller. See the sheriffing panel for an example of this: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/js/controllers/sheriff.js#L8

3. Modify perf.js to create the optionCollectionMap via a call to the option collection model. Currently that code is here: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/js/perf.js#L662 . You should be able to more or less just cut & paste the code from the sheriffing panel here to create the data structure: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/js/controllers/sheriff.js#L108

4. To verify it's working, try adding a series (via the add series button) from mozilla-inbound from the perf view (http://localhost:8000/app/perf.html#/graphs if running a local server). Type "tp5o summary" into the box, you should see some entries for both "pgo" and "opt" if you implemented things correctly.

Comment 1

[srinivas]

Hi wlach,
         I would like to work on this bug. I know angular js a bit.

Comment 2

[William Lachance (:wlach)]

Great! I've assigned the bug to you. Feel free to ask here or irc if you have questions (note that irc might be inactive on weekends eastern/pacific time).

Comment 3

[William Lachance (:wlach)]

Ok in discussions on irc we found out that this is a bit more difficult than expected because ThOptionCollectionModel is actually part of the treeherder module, which we don't include in perfherder.

My understanding of angular is incomplete, but we should probably seperate out two modules: treeherderui (for the UI) and treeherder (for backend services that should be reused). For now though, let's use the same
technique we used with thServiceDomain and just copy the relevant code from option_collection.js into perf.js with treeherder replaced with perf and an appropriate comment to let people know that we're copypasting. Example: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/js/perf.js#L10

Comment 4

[srinivas]

wlach: i tried copying the the relevant code from option_collection.js into perf.js but the perf ThOptionCollectionModel  factory  contains thurl factory, which is in treeherder module again and it may contain similar  recursive dependent factories in it. I would like to suggest that, inject treeherder module into perf module and use all the services/factories provided in treeherder.

Comment 5

[William Lachance (:wlach)]

This sort of makes sense, but I don't think we want to drag all the dependencies of treeherder into the perf module (there's a lot of stuff we really don't want). I'm going to create a seperate treeherder.models module we can use for this stuff, stay tuned...

Comment 6

[William Lachance (:wlach)]

Filed bug 1152376 with a PR. This bug should be easier once that lands. :)

Comment 7

[William Lachance (:wlach)]

Ok bug 1152376 has landed. Should be able to more or less implement this bug as originally described now.

Comment 8

[srinivas]

Attachment: Please review the PR

amendments  made for suggested indentation changes.

Comment 9

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/67269b66e12b7699ad9f43ad9593cfb1925d106c
Bug 1151337 Perfherder graphs should use OptionCollectionModel

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8592949 [details] [review]
Please review the PR

Took a few iterations but this looks good now!

Comment 11

[William Lachance (:wlach)]

This is now fixed!

Comment 12

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/87325652ad280628050e97029ccaa9ea585b60b2
Bug 1151337 Perfherder graphs should use OptionCollectionModel