1533002 - Add proper error handling in ReplicatesGraph

Status: RESOLVED FIXED

(Tree Management :: Perfherder, enhancement, P3)

Description

[Ionuț Goldan [:igoldan]]

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.

Comment 1

[parshva.barbhaya]

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.

Comment 2

[Ionuț Goldan [:igoldan]]

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

Comment 3

[parshva.barbhaya]

Hey [e-mail],
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?

Comment 4

[Sarah Clements [:sclements]]

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.

Comment 5

[Ionuț Goldan [:igoldan]]

(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.

Comment 6

[Ionuț Goldan [:igoldan]]

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?

Comment 7

[Sarah Clements [: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?

Comment 8

[Ionuț Goldan [:igoldan]]

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.

Comment 9

[Sarah Clements [:sclements]]

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