Closed Bug 1647458 Opened 6 years ago Closed 5 years ago

Bulk action handling / Todo list

Categories

(Tree Management :: Push Health, enhancement)

Product:
Tree Management
Component:
Push Health
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: camd, Assigned: suyash546, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Check list UX - Early Mockup
72.29 KB, image/png
Details
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/6579
47 bytes, text/x-github-pull-request
Details | Review
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/6703
47 bytes, text/x-github-pull-request
Details | Review

We need a UI to match the mockup, but will also need back-end work for this.

We will need a new table to track ignored test lines per task.
This table will have fields like:

job: ForeignKey to Job table
resolution: Possibly an enum of "ignored", "flagged", "regression"
test: This can be the text of the test name
user: ForeignKey to the user table
date: date of this action

Need to investigate if we need more than this.

Assignee: nobody → suyash546
Blocks: 1642786
Mentor: cdawson

This is going just above me for now xD
Could you please explain a little more so that I can do proper research on this bug.

Flags: needinfo?(cdawson)

camd, can you link to the mockup and/or add to the user scenario here. It would be great to understand what we are building to think about more than just fields in a DB

Sure! Let's not work on this feature yet. The design/feature is in the final stages of being finalized. We have a mockup, and I'll attach a screenshot of that. Our UX Consultant Victoria is going to do some polish on the design this week, but here is the general idea:

The user will check one or more items in the list and then click either Retrigger or Mark as Investigated.

  • Retrigger will retrigger just the checked items, which all happens within the browser.
  • Mark as Investigated is where the back-end/API work will be. This will mark that specific test on that specific task as investigated. Then the UI will reflect that test as a strikethrough in the list.

Later, we will add filtering so you can hide the investigated tests.

When a test is in the investigated state, and the user checks it, they should be able to mark it as Uninvestigated (the original state for a test)

Flags: needinfo?(cdawson)

Also, you can see the current UI for Push Health by changing to the try repo and clicking the Push Health status link on any push header. Here's a link to one:

https://treeherder.mozilla.org/pushhealth.html?repo=try&revision=0a73ae8292a7e5937b0694330159260f6a1833d9

thanks for the data. In a future world if we auto retriggered jobs, would that change the way we think? This new work flow is a lot more straighforward, I just want to make sure that we don't discount solving the problem in other ways. Possibly in a future time if we did this the need for marking tests into different categories might need to exist.

Lastly thinking about scalability-

  • how will the UI look when there are 100 jobs (10 original retriggered 9 times each) failing with the same error?
  • how will the UI handle 300 unique failures?

Of course this bug is about the backend, knowing how we handle these cases could help shape the backend, maybe we need more data, maybe fields should be named differently. I think we have a good place to start, and implementing something is a good start either as a prototype, version 1, or what ends up being a final state.

tldr; "we'll burn that bridge when we get there" :)

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #6)

thanks for the data. In a future world if we auto retriggered jobs, would that change the way we think? This new work flow is a lot more straighforward, I just want to make sure that we don't discount solving the problem in other ways. Possibly in a future time if we did this the need for marking tests into different categories might need to exist.

I think we should probably have the expectation that this UI will evolve with mozci implementation, and later if we do something like auto-re-triggers. Ideally, we'll continue working closely with users to make sure major new features are designed with their workflow in mind and are intuitive to use. But for now, I think it makes sense to keep things simple based on the changes we've come up with based on our initial research and feedback we've gathered during the iterative design phase.

Lastly thinking about scalability-

  • how will the UI look when there are 100 jobs (10 original retriggered 9 times each) failing with the same error?
  • how will the UI handle 300 unique failures?

In terms of how failures are displayed, it's not really different from how Push Health is currently. The tasks/jobs will likely need to wrap to the next row if there are a huge amount of them that were re-triggered or re-run. I suspect you're missing the context of how users get to this feature (per the mock-up). More details to come in the presentation Tuesday.

Of course this bug is about the backend, knowing how we handle these cases could help shape the backend, maybe we need more data, maybe fields should be named differently. I think we have a good place to start, and implementing something is a good start either as a prototype, version 1, or what ends up being a final state.

The intention behind the to-do list is an investigative tool for users to mark a failure as investigated or not. This is really just for them, so anything auto-triggered shouldn't have anything to do with this table - at least as far as we've discussed it. It might evolve over time. But I think it makes the most sense to keep it simple rather than try to anticipate what the needs might be in the future. Just my two cents :)

here is an example of a lot of failures:
https://treeherder.mozilla.org/pushhealth.html?repo=try&revision=7828c362aadc6179d99176e3518f052b38bc4390

these are common and can be found just about any day. expand the first failure and you will see many instances failing. I assume it will work in the new UI design.

My understanding of users wanting a todo list is so they can figure out what they looked at and what they haven't- the end goal to ensure they look at all the regressions. The goal of push health is to solve this automatically for end users, most end users do not use the retrigger button which is designed to be a single click action to gather more data and have the tool make a decision for them. If end users were using the tool as intended, would there be a need for a todo list? I would like to know what users are doing to investigate manually and would need to mark stuff as "done" or "investigated".

I am just beginning to start on this bug.
What I mostly understood from this was to create a new models.py file in the push health directory
A new model with field
Job (foreign key to treeherder.model.models)
resolution - (enum of ignored, flagged and regression )- although , i am not sure what should be the default value
User - foreign key to the built in django model User(imported from django.contrib.auth.models)
date - the default date when the data will be added
test -
I am unclear with what is the test and where do we get that.
I also had a confusion not directly a hindrance to the bug. When we are ingesting data from taskcluster, how will these tables get field automatically.
Will we set up some system to do the same?
It would also be great if you can tell me whether I am understanding the Bug correctly now and ready for coding task

Flags: needinfo?(sclements)
Flags: needinfo?(jmaher)
Flags: needinfo?(cdawson)

(In reply to Suyash Salampuria from comment #9)

I am just beginning to start on this bug.
What I mostly understood from this was to create a new models.py file in the push health directory

Unless Cam disagrees, I think you could use the existing treeherder/models/model.py instead of creating a new file.

A new model with field
Job (foreign key to treeherder.model.models)
resolution - (enum of ignored, flagged and regression )- although , i am not sure what should be the default value

I think this could be called action - with "investigated" or "uninvestigated" choices. I don't think we need a default value.

User - foreign key to the built in django model User(imported from django.contrib.auth.models)
date - the default date when the data will be added
test -
I am unclear with what is the test and where do we get that.

Cam has a better idea of the underlying logic of this app, so he can help you with this.

I also had a confusion not directly a hindrance to the bug. When we are ingesting data from taskcluster, how will these tables get field automatically.
Will we set up some system to do the same?
It would also be great if you can tell me whether I am understanding the Bug correctly now and ready for coding task

This is the big picture of how it will be used:

  • When a user visits their particular push (likely only once a user clicks on that particular failure line), we call an API to load any tests a user may have marked as "investigated" during a previous session
  • A user continues or starts to investigate a push, marking tests as "investigated" via the checkbox or "uninvestigated" if they made a mistake.
  • The marking mechanism will call an API to update this table (we'll likely want to make sure a user is only doing this for their own pushes)

So this table doesn't need to be populated with data from ingestion. In addition to creating this table, you'll need to create an API (with both read and create or update operations) and the UI functionality.

Flags: needinfo?(sclements)
Flags: needinfo?(jmaher)

Just for reference, we will use the term "job" and "task" interchangeably here.
There are bulk actions the users can make

Current actions are:

  1. Retrigger
  2. Mark as Investigated
  3. Mark as Uninvestigated (if in an "investigated" state from #2 above)

The states of "investigated" and "uninvestigated" will require persistence to the database. We have an existing mechanism a little like this called "classifying" that is done by the Sheriffs. But this is done on a per-job basis. However, each job will have several tests that it executes

Let's create a new Model:

TextLogErrorState
    id
    text_log_error_id  ForeignKey to text_log_error
    state              investigated, uninvestigated (these could be extra failure_classification_ids, if we want
    comment            Not sure if we want this or not
    author

We have a TextLogErrorMetadata and TextLogErrorMatch that will be removed in a PR once I get around to that (or maybe Sarah's PRs will remove some of that)

(In reply to Sarah Clements [:sclements] from comment #10)

(In reply to Suyash Salampuria from comment #9)

I am just beginning to start on this bug.
What I mostly understood from this was to create a new models.py file in the push health directory

Unless Cam disagrees, I think you could use the existing treeherder/models/model.py instead of creating a new file.

Yes. Let's use the existing models.py file.

Flags: needinfo?(cdawson)

Alternative to my proposed new model, we could just add the state, comment and author fields to the text_log_error schema itself.

hrm... that table is quite large and there's multiple lines associated with any job. I need to land my backfill pr before this is changed. I'm not quite seeing how marking a failure line will show us which test suites associated with a failure line are investigated or not :) But Cam has a better understanding of how that data is associated/use in the UI.

We don't have a feature to add comments, so perhaps we should hold off on adding that field for now.

(In reply to Sarah Clements [:sclements] from comment #14)

hrm... that table is quite large and there's multiple lines associated with any job. I need to land my backfill pr before this is changed. I'm not quite seeing how marking a failure line will show us which test suites associated with a failure line are investigated or not :) But Cam has a better understanding of how that data is associated/use in the UI.

Shoot, right. My bad. OK. Let me think about this a little further... I think this is where I may need to do the work in https://bugzilla.mozilla.org/show_bug.cgi?id=1636506 first. So that we can have the association to the test.

In order to implement the investigated/uninvestigated marking, we will need me to finish work on Bug 1636506. But that won't block all the work on this feature. I am working on this now.

Suyash: I recommend you implement the Front-end work on the checkboxes with retriggering as shown in the mockup. Then we can add in the marking of investigated in a separate PR.

I was working on the frontend today
I was looking at the props failure of the TestFailure.jsx component.
it was something like this
{"testName":"gfx/tests/reftest/1435143.html == gfx/tests/reftest/1435143-ref.html","action":"test","jobName":"test-windows10-64/opt-reftest-e10s-1","jobSymbol":"R1","jobGroup":"Reftests","jobGroupSymbol":"R","platform":"windows10-64","config":"opt","key":"tgfxtestsreftest1435143htmlgfxtestsreftest1435143refhtmloptwindows1064testwindows1064optrefteste10s1Reftests","jobKey":"optwindows10-64test-windows10-64/opt-reftest-e10s-1Reftests","inProgressJobs":[],"failJobs":[{"id":308210327,"machine_platform_id":207,"option_collection_hash":"102210fe594ee9b33d82058545b1ed14f4c8206e","job_type_id":116251,"job_group_id":450,"result":"testfailed","state":"completed","failure_classification_id":1,"push_id":741251,"start_time":"2020-07-01T15:41:26","job_type_name":"test-windows10-64/opt-reftest-e10s-1","job_type_symbol":"R1","platform":"windows10-64","task_id":"Eann3JhkT0-uVnIeeoEEqg","run_id":0,"resultStatus":"testfailed","searchStr":"Windows 10 x64 test-windows10-64/opt-reftest-e10s-1 undefined(R1)","duration":null,"hoverText":"test-windows10-64/opt-reftest-e10s-1 - testfailed - NaN min"}],"passJobs":[],"passInFailedJobs":[],"logLines":[{"action":"test_result","line_number":44389,"test":"gfx/tests/reftest/1435143.html == gfx/tests/reftest/1435143-ref.html","subtest":"image comparison, max difference: 1, number of differing pixels: 4","status":"FAIL","expected":"PASS"}],"suggestedClassification":"New Failure","confidence":0,"tier":1,"failedInParent":false,"passFailRatio":0}

Sorry for copy paste exactly
I was looking for a unique item in this so that I can mark the test investigated. Although, I don't exactly know the model structure we are going to implement here for this. SO it might be early to ask this
But currently, we are rendering using the props failure(which comes after some arrangement from metrics in the Health.js) and the job name is displayed. I am unable to find any id or unique in this failure so that it can be updated on the database.
Although, I remember we were adding the parameter id to the new component we were adding

Suyash: Each of those items has a key and a jobKey. I think you would want to use the key and append the Push revision SHA to it. I'm not sure how long the keys in LocalStorage can be, so you may have to experiment with that.

This has been completed.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: