1123814 - Get desktop notifications when a build finishes or gets busted

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Frontend, defect, P5)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I have treeherder open in an app tab, as lots of other do too likely. I am eagerly waiting for a build to complete and would like to know first thing.

I think it would be nice to use the web notifications API to show messages when builds have completed. This should be a feature that either needs a user-global on/off switch, or the ability to notify for specific builds previously selected by the user.

If we go for build-specific notifications, then an option to always be notified if a build fails would be nice, optionally more fine grained for only bustages or also oranges.

Comment 1

[Ed Morley [:emorley]]

Yeah I agree this would be awesome. I don't know how soon we'll be able to work on this (470 open bugs at the moment, many of which infra/reliability/fixing missing features compared to TBPL) but if anyone wanted to experiment with this in the meantime we'd definitely take the PRs! (or even just UX brainstorming).

Comment 3

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:notifications > mozilla:master

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8771728 [details] [review]
[treeherder] KWierso:notifications > mozilla:master

Here's a WIP. I'd like some feedback before I get too into this.
At the moment, this adds a new button to each push's actionbutton menu, to add this push to a watchlist. 

When you add an item to this watchlist, an interval is started up.

When the interval gets called, it checks the percentComplete value for each of the revisions in the watchlist. If the percentComplete is 100, a notification is issued, and that revision is removed from the watchlist.

I've only tested that this works on an already finished push, so I'm hoping it works for a push that makes the transition from incomplete to 100% finished.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Let it run on a still running push on inbound and it successfully notified me when the push transitioned to 100% percentComplete. There were still 4 or 5 running PGO builds when the transition happened, visible on Treeherder and BuildAPI. Not sure why they aren't contributing to the percentComplete value.

I also retriggered a job on completed try push and I saw the new running job in Treeherder, but the push stayed at 100% completed, despite the new running job.

I'll try to see if there's a bug already on file for these things, maybe I'll file one if I can't find one.

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 8771728 [details] [review]
[treeherder] KWierso:notifications > mozilla:master

Cameron, I don't suppose you could take a look at this? I think you'll be be better reviewer of this than I :-)

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8771728 [details] [review]
[treeherder] KWierso:notifications > mozilla:master

This looks really cool.  Nice work!

Comment 9

[Ed Morley [:emorley]]

I think for this we need to decide whether to:
a) Have it be opt-in per push
b) Have it be always on when viewing a single push
c) Have it default to on when viewing a single push, but allow users to disable it for that push
d) Have a global toggle for "show notifications when {viewing a single push, viewing a single push on try, ...}" and no per-push controls

Once we know the answer to that it should be easier to make progress here.

Thoughts?

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'd really like to see this, any chance I could get some guidance on how to fix it? It seems the latest suggestion was to have desktop notifications always run on single revision pages? 

Note that to enable desktop notifications there needs to be a button somewhere that the user can click to enable notifications, I'd suggest making this a toggle button that persists state. If you enable desktop notifications, you'd get them on all single-revision pages until you disabled them. Having a button that is not push specific I'm not seeing a good place to put this, the filter bar is already pretty full. Any suggestions?

Next, the current patch uses polling, which we are already doing in places. It seems to me that I could instead listen for thEvents.jobsLoaded, which would hopefully give me a list of updated jobs only.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

There is also a notifications icon in TH now, I guess I could potentially hook this up to receiving desktop notifications for each such notification, and add a notification on new jobs completing to that. I'm not sure what the scope of the notifications pane in TH is supposed to be though.

Comment 12

[Cameron Dawson [:camd]]

Hi Philipp--  Sounds great that you're up for working on this!

Looking at Ed's list, my personal preference is for option "a".  You could have a toggle button on PushHeader.jsx (the header for each push) where you enable it for a specific push.  Put a button called "watch" between the % complete and "View Tests" button.  

I would be annoyed by option "b" and "c", personally.  :)  But option "d" would be ok, too.  If you don't put it on PushHeader.jsx, then I would vote for putting the toggle button on the top-level (thGlobalTopNavPanel), next to the notification bell button.  

One request I'd have would be to implement this in React (using ng-react, if necessary).  I'm in the midst of converting everything from Angular to React and don't want to be adding more Angular code that will need converting.  :)

Our notifications (thNotify) happen only on the one page, and only on-screen.  That notification (bell) icon gives you a history of those for when they've disappeared too quickly.  So probably not what you're looking for.  It sounds like perhaps something much like what ircclound does, play a sound and pop up a growl-like notification box would work well.

Yes, the thEvents.jobsLoaded event is sent every time new jobs are loaded into treeherder via our polling and they are just the jobs that changed.  But you may not need to listen for that event yourself.  You could tap into what PushHeader.jsx does when it determines whether to set the % done to complete.  Though, you won't want to notify EVERY time we render in a complete state.  Only when we CHANGE to complete, so perhaps listening for that event IS the right way.  PushHeader.jsx (or a sub-component of it) is likely a good place for that logic.  :) 

Note: ``push.job_counts`` only have pending, running, completed counts.  Not # passed or failed.  That's probably good for the first iteration of this.  If we wanted to notify with counts in a later PR, we could father those counts, too.  Hopefully after I've converted the resultset_store.js file to React.  :)

Please let me know if you have more questions.  Thanks so much for looking into this!

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

One thing I would like to see is a way to toggle notifications per push state change, or per job state change. The latter might not make sense for a full push on m-c, but if you are doing a try run with just a few platforms and you want to be notified when the first build completes (most notably, when it fails), it would be very helpful.

I've managed to do this by making the watch button a three state toggle button with the labels "Watch", "Notifying (per-push)", "Notifying (per-job)". The latter two states have a border to make it look selected.

In my current patch I am not persisting the watch state over a reload. This of course means if you switch to a different repo tab it will navigate to a different URL and not notify. I'm not sure in what other situations TH decides to reload the page, if at all.

I'm happy to add persisting, but before I do I thought I'd ask your opinion. If you think it is worth adding, I'd need to create a model somewhere that would allow saving and querying for watched pushes which I'd pass on to the react component. With the react/angular mix I'm not quite sure where the best place for that would be that doesn't add more angular code.

Comment 14

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3373

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I noticed that the eslint rules don't check for indent. I made sure my patch has correct indent, but I almost missed that. Maybe worth adding in another bug?

Comment 16

[Cameron Dawson [:camd]]

Yeah, we want to enable that, but our codebase is partially in 4-space and all the new code is in 2.  As we migrate from AngularJS to React, we're changing that.  We may get to a point where we do a big 4-to-2 migration and enable that lint rule, but probably not right away.

Comment 17

[Cameron Dawson [:camd]]

FWIW: We currently do a reload on any of these url parameter changes:
            'repo',
            'revision',
            'author',
            'fromchange',
            'tochange',
            'startdate',
            'enddate',
            'nojobs'

However, my end-goal is to never have to fully reload the page.  At worst, it'd load new data if you set a param here that is outside the currently loaded data.  But we're not there yet.

For now, I think not persisting this is totally fine.  YAGNI principle and all that...  If folks decide we need that, we could store it in localstorage, perhaps.  We might need a "clear all watches" action in that drop-down menu if we do that.  Or store a timestamp with them for expiration.  We'd have to think more bout that...  :)  But in that case, we'd just check LS on page-load and if we have a matching repo/commit then watch it.  Admittedly, I'm spit-balling here...  :)

Comment 18

[Cameron Dawson [:camd]]

While testing this out, I immediately found myself wanting to hide/collapse the non-watched pushes.  Just mentioning in case we want to do that sometime in the future.  Maybe it's just me...  :)

Comment 19

[Cameron Dawson [:camd]]

Comment on attachment 8962197 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3373

Clearing review till my comments are addressed.  Thanks!  :)

Comment 20

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/7cf1013f09d3cb83405eed9d6b25182447b23517
Bug 1123814 - Get desktop notifications when pushes or jobs complete (#3373)