Improve Behavior and Consistency of Report Processing Logic Across PR comment and App (Commit and PR)

codecovdesign commented 2 months ago

Problem to Solve

When report processing fails on a pull request, the failure is currently only visible in the commits section. Users may not be aware that they need to check commits for this information, leading to confusion and missed insights.

Proposed Solution

Update Codecov’s PR comment messaging to display a clear indication of report processing failures directly on the PR page.

areas for investigation:

initial investigation:
- why is there an error mismatch here in commit vs PR?
- today, do we show full or partial data on the pr comment, PR app and commit app
Next: what is our intended/desired behavior?
- is it worthwhile to show coverage information IF any related commits contains reports that are not received?
- do we want to show partial data or awaiting full data (intended)?
  - IF partial show some copy/indication of such
- should afternbuilds consider as default? tradeoff is long wait
- suggestion: show the processing/pending state and then outline error

codecovdesign commented 2 months ago

raised at audit, marking as bug, as pointed out per the user expectation is unexpected

codecovdesign commented 2 months ago

sync with Eli on 8/29

we need to better understand the logic/engineering here. why can't we report an intermediary state or display upload count and should we push harder indicate how many uploads are processing x or y (example)? Overall, there is a lot to do but we'll probably need to expand our capability and contextualize the data for the user. route moving forward for exploration: 1) identify intermediate state, 2) default doesn't show coverage, 3) can opt-in to partial data
Kyle todo setup a kickoff

codecovdesign commented 2 months ago

related issue under review here: https://github.com/codecov/engineering-team/issues/2442 this is related since the contextualized state there when incomplete information is show for a different but related scenario. if we'd like to show partial information, we could follow this pattern.

codecovdesign commented 1 month ago

related: https://github.com/codecov/engineering-team/issues/1245 in this case situation that would be helpful if we knew the amount of uploads processing

codecovdesign commented 1 month ago

sync on 9/10

step 1, @RulaKhaled (in app commit/pr, related: https://github.com/codecov/engineering-team/issues/2238) and @nora-codecov (pr comment), key question: today, do we show full or partial data on the pr comment, PR app and commit app
- do we have this processing state in the DB / could relate to project shelter (could there be a way to leverage this)
- other can we identify the amount of uploads that are being processed? then we can inform/contextualize to the user example 12 of 23 processing
step 2, once we better understand the system logic, review with wider team, then we can explore explore: i) identify intermediate state (help inform: https://github.com/codecov/engineering-team/issues/2220), ii) default doesn't show coverage (identify the default state to show full - wait for dat), iii) can opt-in to partial data (user opt-in flow to partial data)

RulaKhaled commented 1 month ago

Upload Error Behavior on the Commit Page:

What happens?

We retrieve the uploads from commitData?.commit?.uploads.
We check if there is at least one upload with an error state.
If an error is found, we display an error message by grouping the uploads under their respective unique providers.
For each error, we show the build code, CI URL, and the timestamp of when the upload was created.

How does it work?

We loop through the upload states.
If the upload state is set to error, we group the uploads by provider, where the key is the provider and the value is the associated uploads.

These are the expected upload states:

export const UploadStateEnum = {
  error: 'ERROR',
  uploaded: 'UPLOADED',
  processed: 'PROCESSED',
  complete: 'COMPLETE',
  started: 'STARTED',
} as const;

TLDR: On the commit page, if any upload has an error state, we display the error message immediately. We skip checking for other states once an error is detected, focusing solely on the error state for that upload.

RulaKhaled commented 1 month ago

To answer the question:

Will this page update if the CI build is triggered for the same commit?

No, the page will not update because we still account for the existing upload error. As long as the error persists, the error state will remain displayed. If the user submits a different commit with no upload errors, it will be processed as usual. The issue here is not related to the "processing" state but rather how the application handles errors.

We should consider what behavior is expected if there is one processing error. Should we display partial data in such cases? Currently, if part of the data is not successful, no data is shown at all.

Lmk if you have any specific questions!

Adal3n3 commented 1 month ago

According to @thomasrockhu-codecov, unfortunately we can not support the re-run all jobs to show the latest job in the UI. I think we can use "history" or "Version" at the list of commits and list of pull pages to indicate the data is from the initial run so if you rerun the commit/job, you will be looking at the recorded history of the commit. (Related to issue #2220)

Maybe we can reuse the page header and description for this message. Wdyt of this solution @codecovdesign?

Update from sync 9/24: we consider not show this in the commits page but focus on the individual commit details page.

codecovdesign commented 1 month ago

We should consider what behavior is expected if there is one processing error. Should we display partial data in such cases? Currently, if part of the data is not successful, no data is shown at all.

@Adal3n3 from some of the ongoing work, is there an initial iteration that will help with this scenario? (indicating this has processing issue occurred)

also added a sync for next week we can talk then too

codecovdesign commented 1 month ago

sync on 9/24 with @RulaKhaled @Adal3n3 @nora-codecov

rola: we didn't look at pull page too much (see here how it displayed on pulls page) only displays initial run
re: the issue around re-rerun, suggest focus on showing the processing issues on the commit page respectively. instead of x on commit landing indicate fail uploads
outstanding question do we want to show partial data? (today on the commit page we don't show it)
- on pulls page it appears we show the data
- ideas on whether to show partial or not:
  - it's not useful and confusing (intermediary state could be to show patch and contextualize the data)
  - opt-in by default to show partial data (patch only display until full data) – customer could opt out and prefer to
last questions
- what is our logic/behavior for BOTH the pr comment and app pulls page when the head commit has errors

nora-codecov commented 1 month ago

Tom helped me set up a test branch with an erroneous upload, so we can see and test conditions

on codecov

the pr on gh

current experience

pr comment does not notify user that there are failed uploads
pr comment displays patch and project coverage
there is no indication anywhere on the pr that there was an issue with uploads
similarly, in codecov.io there's very little indication of upload failure
it's unclear that 1 failed upload invalidates all past and any future uploads on the commit

The thing that's confusing me is that the notification (pr comment) we want exists, but it's not being returned as the pr comment when there's an upload error, and I don't know why. Here's what I found:

an upload triggers the UploadTask which kicks off the UploadProcessorTask, which chains together the reports to process, and inserts UploadFinisherTask as the last item on the processing chain.
UploadFinisherTask looks at the reports that were just processed in the chain, and if any are successful = False, it changes the result and triggers NotifyErrorTask. Here's that decision making code
NotifyErrorTask (code here) should be posting a comment with the message "❗️ We couldn't process [{self.failed_upload}] out of [{self.total_upload}] uploads. Codecov cannot generate a coverage report with partially processed data. Please review the upload errors on the commit page." which seems to be the behavior we are looking for.
but as we have experienced, when uploads fail the user doesn't get that message as the pr comment, and I don't know why. I'm going to look into it... UPDATE: it's off by default, you have to opt-in by adding it to your codecov.yml, going to add to test pr.

can we tell that we are processing uploads or the number we are processing, and surface that to the user

do not know, will investigate
i feel like there must be several signals that we are mid-process. Whether we could send live numbers on report count back to the user, not sure.

RulaKhaled commented 1 month ago

what is our logic/behavior for BOTH the pr comment and app pulls page when the head commit has errors

Confirmed that we do show partial data, we don't account for the errored upload

nora-codecov commented 1 month ago

added notify: notify_error: true and got the pr comment!

you can see it here

nora-codecov commented 1 month ago

Joey actually did this work at the time but never finalized and merged the prs, which would make this the default behavior for new repos going forward: https://github.com/codecov/shared/pull/320 and https://github.com/codecov/worker/pull/601

nora-codecov commented 1 month ago

can we tell that we are processing uploads or the number we are processing, and surface that to the user

for sure not through the pr comment, so this would be a gazebo-only feature
@RulaKhaled would be the better judge of this, but I think displaying numbers of processing reports/uploads would be more of an event-driven feature, right now we don't have any event-driven setup in gazebo (cmiiw), so that might be a big lift for this feature, to build a listener to display this. We could also do quick and dirty build where gazebo is just hitting backend every x seconds asking for these numbers - I'm going to leave this with apps team, they would know better than me 🙃 but I do think this should be taken into consideration, since it will shape how it's built
about Shelter: shelter won't help in terms of uploads, all report processing will still happen in the current tasks in Worker. However, Shelter is set up to be an event driven tool, so maybe in the future we could build a reporting system on shelter that would work for notifying the user about upload processing status.
There's definitely a way to do this
- CommitReport table - there's 1 row per commit per type (type can be coverage, bundle analysis, test report - think about whether we want to separate those or separate by flags. Also carried forward uploads, would those be displayed differently?)
- the CommitReport can have any number of ReportSession/ReportsUploads. This is created at the beginning of the upload process, so if an upload has been attempted/initiated, there is one of these in the database.
- The Upload box/list that we currently have on the commit page in gazebo, looking at that endpoint, I think this would give what you want. the uploads (ReportSessions) have a state field, you could display by state. In fact, right now I don't see any filters, so I'm not sure why it's not returning and displaying processing reports - theoretically it should.

I think this answers all the investigation questions, let me know if you need anything else ☂

codecovdesign commented 1 month ago

now that we confirmed the behavior states, next is to the question of what we want to do (to show partial or not to show)

eli: initial thinking here is around our confidence of patch coverage; project coverage is another story bc it's difficult to know what's happening until reports are completed
- scenario 1: show partial coverage and contextualize to the user processing is occurring
  - question here is why to show to the user since we know it's, exception here is IF we can fully know the diff/patch then we could conditionally show that information (related: https://github.com/codecov/engineering-team/issues/2442). this issue is that certain reports determine the patch and while one reports one another may report another, and it's still the same project coverage. Unless all patch related lines have been covered through processing
- scenario 2: wait to show full coverage and contextualize the processing state
  - unless 1 is resolved with patch, this seems like the direction to align with
  - could then have opt-in to partial

Next step, we'll have a debrief sync just to discuss these finding and close out the issue for planning

From group review with @Adal3n3 @aj-codecov @eliatcodecov @nora-codecov

scenario 2 is the direction that makes sense, since only the fully processed data is the source of truth – why continue to show partial since we know it's incorrect and/or could cause additional ux confusion
- IF we can show patch data with confidence, as the related lines are processed. example if one report shows that 1 line has gone from uncovered to covered. only time we can show the intermediate coverage number is the moment the patch is 100%
- we do have a record of initiated uploads but that doesn't report in upcoming; we can't know what done is
implement manual trigger in setup (we don't want as default experience, as it may be needed )
- recommended feature to suggest this update (example condition to trigger suggestion: repo that consistently uploading different # of reports) or using after_n_build

update to move forward with:

show full coverage report, eng issue to identify how we'd perform this
- ~~interim ToDo, design what are empty states while processing occurring~~ ✅ will be resolved here: https://github.com/codecov/engineering-team/issues/2220
- kyle todo talk with trent
show in PR comment when there is a processing failure, ongoing work to make processing status shown as default behavior to new repos
- ToDo: polish copy for future iterations (https://github.com/codecov/engineering-team/issues/2400#issuecomment-2374793754)
- additionally surface related issue on applications side

codecov / engineering-team