Open Bug 1600004 Opened 2 years ago Updated 6 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.

Is this bug still active and relevant to the codebase?
I would love to solve it as my first contribution to the repository.

Flags: needinfo?(igoldan)

(In reply to jamesbarboza720 from comment #10)

Is this bug still active and relevant to the codebase?

Yes, it's still relevant to the codebase.

Flags: needinfo?(igoldan)

(In reply to Ionuț Goldan [:igoldan] from comment #2)

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

Hello, Ionuț :)
Thanks for the detail.
When I look at this issue, The example URL is broken. So, Can I check this issue through the URL[0]?

Also, Is your intention to add a prev_push_revision to the button as follows?

ex) prev_push_revision:b0e87848f181 <-(current revision)

I'm talking to you because I want to know more clear.
Thanks :)

[0]
https://treeherder.mozilla.org/perfherder/alerts?id=29510&hideDwnToInv=0

Flags: needinfo?(igoldan)

Hello, I am an outreachy applicant. Can I work on this bug?

(In reply to Myeongjun Go from comment #12)

[...]
Hello, Ionuț :)
Thanks for the detail.
When I look at this issue, The example URL is broken. So, Can I check this issue through the URL[0]?

Sure thing.

Also, Is your intention to add a prev_push_revision to the button as follows?

ex) prev_push_revision:b0e87848f181 <-(current revision)

I'm talking to you because I want to know more clear.
Thanks :)

Yes, I'd like the button' s text to include both previous push & the current push.

[0]
https://treeherder.mozilla.org/perfherder/alerts?id=29510&hideDwnToInv=0

Flags: needinfo?(igoldan)
You need to log in before you can comment on or make changes to this bug.