Thank you for helping out with Treeherder! You can find us on IRC at irc://irc.mozilla.org/treeherder Here's some links to help get you started. Project page: https://wiki.mozilla.org/Auto-tools/Projects/Treeherder Repo locations and links to set up a development version of the software: https://wiki.mozilla.org/Auto-tools/Projects/Treeherder#Getting_Started Interacting with us: https://wiki.mozilla.org/Auto-tools/Projects/Treeherder#Contributing A-Team general reference, coding style guides: http://ateam-bootcamp.readthedocs.org
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.
Hi wlach, I would like to work on this bug. I know angular js a bit.
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).
Assignee: nobody → vykuntamsrinivas
Summary: Perfherder should use OptionCollectionModel → Perfherder graphs should use OptionCollectionModel
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
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.
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...
Filed bug 1152376 with a PR. This bug should be easier once that lands. :)
Ok bug 1152376 has landed. Should be able to more or less implement this bug as originally described now.
Created attachment 8592949 [details] [review] Please review the PR amendments made for suggested indentation changes.
Attachment #8592949 - Flags: review?(wlachance)
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 on attachment 8592949 [details] [review] Please review the PR Took a few iterations but this looks good now!
Attachment #8592949 - Flags: review?(wlachance) → review+
This is now fixed!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/87325652ad280628050e97029ccaa9ea585b60b2 Bug 1151337 Perfherder graphs should use OptionCollectionModel
You need to log in before you can comment on or make changes to this bug.