Perfherder graphs should use OptionCollectionModel

RESOLVED FIXED

Status

Tree Management
Perfherder
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wlach, Assigned: vykuntamsrinivas, Mentored)

Tracking

Details

(Whiteboard: [good first bug][lang=js])

User Story

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

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 4

3 years ago
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.
Flags: needinfo?(wlachance)
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...
Flags: needinfo?(wlachance)
Depends on: 1152376
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8592949 [details] [review]
Please review the PR

amendments  made for suggested indentation changes.
Attachment #8592949 - Flags: review?(wlachance)
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: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.