1468172 - Introduce nudge functionality for perf alerts

Status: RESOLVED FIXED

(Tree Management :: Perfherder, enhancement, P2)

Comment 1

[GitHub Bugzilla PR Linker]

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

Comment 2

[Ionuț Goldan [:igoldan]]

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

This is still work in progress, but at least it outlines the main approach for nudging alerts.
I'll remove all console logs once this is ready to land. Will also add unit tests for BE.

Comment 3

[Ionuț Goldan [:igoldan]]

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

I'm interested about the Javascript changes. Would you have added this new functionality in another way, more React-friendly?

Comment 4

[Cameron Dawson [:camd]]

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

My comment from the PR:

I think this is fine for now.  As we move to ``React`` I want to convert models like this to be JavaScript ``class``es that use ``fetch``.  But unless you're up for converting this whole model to a JS class now, we can add it this way, and then convert the whole thing when we get to it.  ``findPushIdNeighbour`` *could* be a static function in a perf helper, if it is going to be used in lots of odd places.  But from the look of this, it seems appropriate here.  ``nudgeAlert`` seems correct here.  So I think your approach is fine and we'll convert this whole model later.

Comment 5

[Ionuț Goldan [:igoldan]]

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

Could you give a final look over this feature?

Comment 6

[Ionuț Goldan [:igoldan]]

I will drop all logs right before landing this PR.

Comment 7

[William Lachance (:wlach)]

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

Left feedback in PR

Comment 8

[Ionuț Goldan [:igoldan]]

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

I've addressed the review requests you made.

Comment 9

[Ed Morley [:emorley]]

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

Happy with the changes to address the general comments I left before. I'll leave it to Will to review the perfherder-specific parts :-)

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/a0c1b607debad384642b9cfb258d5da9e3c4bba6
Bug 1468172 - Introduce nudge functionality for perf alerts (#3671)

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

:igoldan, can you verify this works and is useful- I want to make sure this is fully resolved

Comment 12

[Ionuț Goldan [:igoldan]]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #11)
> :igoldan, can you verify this works and is useful- I want to make sure this
> is fully resolved

This bug does enable the nudge feature. Unfortunately, the re-drawing was broken prior to adding it.
Basically, when you're nudging in either direction, the graph drawing brakes, just like when you're manually creating a new alert; but on the BE side everything goes ok, unaffected. So a page refresh should bring the correct graph back again.

I may have filed a separate bug for that already, just need to check.