Closed Bug 1533002 Opened 1 year ago Closed 29 days ago

Add proper error handling in ReplicatesGraph

Categories

(Tree Management :: Perfherder, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: igoldan, Assigned: sclements, Mentored)

References

Details

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

This is more desirable than simply letting the error boundary catch the result if there's not any data available.

This requires refactoring the PerfSeriesModel to use the getData wrapper the same way PushModel.getList now does. More details about this can be found in bug 1473777.

Priority: -- → P2
Priority: P2 → P3
Assignee: igoldan → nobody
Mentor: igoldan
Keywords: good-first-bug
Whiteboard: [lang=js]

Hello @igoldan

I would love to claim this issue and work on it as my first issue.
I would love to have some referral documentation to help me get started with setting up the environment.

Flags: needinfo?(igoldan)

Cool! These are the Treeherder docs. They mention everything you need to know, including our Github workflow.

Flags: needinfo?(igoldan)
Assignee: nobody → parshva.barbhaya

Hey igoldan@mozilla.org,
I set up the work envirnoment and went through the docs and the thread you mentioned issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1473777)

I understand that I have to refactor the code but where and which files? Could you provide me with some insights?

Hi Parshva,

You'll want to look in the ui/Perfherder/compare/CompareSubtestDistributionView file and replace all uses of fetch .then, with the getData helper (search for it in the codebase and you'll find which helper file its in). There is some context as to why in the bug bug 1473777 Ionut referenced. If you look in other files in the Perfherder directory, you can see how they parse and show an error message and data that's extracted from the getData helper. If you aren't certain once you get it started, you can open a pull request with a WIP (work in progress) tag and ask Ionut to give you feedback.

Please be sure to reference this bug number in your first commit ( such as "Bug 1533002 - Replicates view error handling change", for example) and in the pull request title, so the bot links the patch to this bugzilla bug.

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

[...]
This requires refactoring the PerfSeriesModel to use the getData wrapper the same way PushModel.getList now does. More details about this can be found in bug 1473777.

The refactoring I've mentioned 3 months ago is no longer required, as it seems Sarah already covered that during bug 1603249.

See Also: → 1603249

Looking more into this, I believe Sarah actually addressed this bug entirely including the proper error handling, thanks to bug 1603249 and the associated PR which made these specific changes:

const replicatePromises = perfDatum.job_ids.map(jobId =>
      getReplicateData({ jobId, rootUrl: project.tc_root_url }),
    );

    const localReplicateData = await Promise.all(replicatePromises);
    const errorMessages = processErrors(localReplicateData);

    if (errorMessages.length) {
      replicateData.replicateDataError = true;
      return { replicateData };
    }

Am I correct on this?

Flags: needinfo?(sclements)

My changes were for the ReplicatesGraph but Parshva's pr was adding it to the CompareSubtestDistribution View: https://github.com/mozilla/treeherder/pull/5877

Parshva, will you finish your pr so we can merge it?

Flags: needinfo?(sclements) → needinfo?(parshva.barbhaya)

This bug was filed for ReplicatesGraph component specifically & it didn't target any other component.
I think it'd be better to file a separate bug & address the CompareSubtestDistribution there.

Going to mark this as resolved. Parshva, if you finish your pr let me or Ionut know and we'll file get it merged.

Flags: needinfo?(parshva.barbhaya)
Assignee: parshva.barbhaya → sclements
Status: NEW → RESOLVED
Closed: 29 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.