Open Bug 1600004 Opened 1 year ago Updated 10 months ago

Replace current push from alert summaries with previous push & current push tuple

Categories

(Tree Management :: Perfherder, enhancement, P2)

enhancement

Tracking

(Not tracked)

People

(Reporter: marauder, Unassigned, Mentored)

References

Details

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

The issue has been observed while sheriffing on build-metrics alerts.

Same changeset, same time but separate alert summaries:
https://treeherder.mozilla.org/perf.html#/alerts?id=24108
https://treeherder.mozilla.org/perf.html#/alerts?id=24061

Another example:
https://treeherder.mozilla.org/perf.html#/alerts?id=24105
https://treeherder.mozilla.org/perf.html#/alerts?id=24044
https://treeherder.mozilla.org/perf.html#/alerts?id=24071

These alerts should be displayed on the same summary.

This is an old behavior. But I agree it's misleading.
I think it's more of an UI issue. When Perfherder creates new alert summaries, it groups them based on repository, framework, current push & previous push (this is the responsible code). Problem is the UI displays only the first 3 characteristics, but not the 4th one.

Thus, the UI is falsely telling the users: "See this summary from repo X & framework Y? Well, it was caused by current push Z" when instead it should say "See this summary from repo X & framework Y? It was caused by this suspect patch, which consists of all changesets from previous push Z1 up to and including current push Z2".

I propose the following fix, with the help of an example. Let's take summary #24226.
Currently, it visually states that the a6043688b1ab push is the culprit one.

We should tweak the UI & replace a6043688b1ab with something like 9e137d4511cd : a6043688b1ab or 9e137d4511cd ... a6043688b1ab or 9e137d4511cd - a6043688b1ab

Whiteboard: [lang=js]

Okay, that makes sense. I agree that would be a nice enhancement. If we have several alerts with overlapping alerts ranges, could we automatically group them together in a single summary? It would mean a wider regression range, but could save the sheriffs a lot of time performing manual reassignments and would reduce our total number of alert summaries.

(In reply to Dave Hunt [:davehunt] [he/him] ⌚BST from comment #3)

Okay, that makes sense. I agree that would be a nice enhancement.

Cool!

If we have several alerts with overlapping alerts ranges, could we automatically group them together in a single summary? It would mean a wider regression range, but could save the sheriffs a lot of time performing manual reassignments and would reduce our total number of alert summaries.

I think it's doable. Could we file a separate bug for this & handle the grouping issue there? I would like if we would 1st address this bug.

Flags: needinfo?(dave.hunt)
Type: defect → enhancement
Priority: -- → P2
Summary: Alerts created at the same time and changeset were displayed on separate summaries → Replace current push from alert summaries with previous push & current push tuple

(In reply to Ionuț Goldan [:igoldan], Performance Sheriff from comment #4)

If we have several alerts with overlapping alerts ranges, could we automatically group them together in a single summary? It would mean a wider regression range, but could save the sheriffs a lot of time performing manual reassignments and would reduce our total number of alert summaries.

I think it's doable. Could we file a separate bug for this & handle the grouping issue there? I would like if we would 1st address this bug.

Sure, I'll leave you to file the bug, but would like it to be a P1 for consideration in Q1 due to the potential time savings for sheriffs.

Flags: needinfo?(dave.hunt)
Duplicate of this bug: 1258845

I would love work on this claim and contribute to it as my first issue.
Can I claim this issue?

Also, I would love to refer dev docs for setting up development environment.

(In reply to parshva.barbhaya from comment #7)

Also, I would love to refer dev docs for setting up development environment.

I see that Ionut has helped you in bug 1533002 comment 2. Perhaps this could be your second issue. :D

Mentor: igoldan

(In reply to Dave Hunt [:davehunt] [he/him] ⌚BST from comment #8)

I see that Ionut has helped you in bug 1533002 comment 2. Perhaps this could be your second issue. :D
I am awaiting his reply in bug 1533002, would love to handle this issue right now but already have 2 assigned to me right now. Will revert back when I am free to take this issue.

You need to log in before you can comment on or make changes to this bug.