Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{Codecov upload issue debugging} - Ensure Codecov consistently waits for ALL reports to successfully process before reporting on coverage on UI and PR comment #2702

Open
rohan-at-sentry opened this issue Oct 21, 2024 · 8 comments
Assignees
Labels
Area: Report Processing Issues with report processing Area: Report Upload Issues with pre-ingest report uploading epic this label is used to mark issues as epics in discovery The design, product, and specifications require refinement P0: must do priority 10
Milestone

Comments

@rohan-at-sentry
Copy link

rohan-at-sentry commented Oct 21, 2024

CONTEXT In issue #2400, we confirmed inconsistencies in the reporting experience across the app and PR comments. Specifically:

  • The app’s commit page displays a fully processed report.
  • The pulls page and PR comments only show partial data.

This investigation was conducted by @nora-codecov and @RulaKhaled

User Experience Impact

Users frequently see partial data reports, leading to the perception of incorrect or incomplete information, which undermines trust in our reporting accuracy. Improving user trust and data clarity is a key focus area.

One solution under consideration involves providing feedback as processing occurs (see #2522). However, implementing this depends on resolving blockers related to #2400, particularly around the feasibility of showing full reports and identifying intermediary processing states.

Aside, but related, @spalmurray-codecov has explored a potential solution, which may address some of these blockers (see findings here).

Next Steps

  1. Determine Reporting Capabilities

  2. Evaluate Findings

    • Based on investigation results, the Eng/Design/PM team will assess next steps, such as showing an intermediary state, adjusting configuration defaults, or other potential improvements.
    • Consider a more standardized approach to report processing, potentially deprecating after_n_builds or setting default behavior without configuration. This will depend heavily on investigation outcomes.
  3. Parallel Investigations

kickoff details / rough notes / other

Today we have 2 ways to let customers instruct customers on when to kick off the notification process..

  1. After_n_builds - https://docs.codecov.com/docs/pull-request-comments#after_n_builds
  2. manual_trigger and send-notifications - https://github.com/codecov/codecov-cli#send-notifications

Problem 1 - Not every customer knows to turn this feature on. Turning this feature on allows Codecov to "know" what is the max # of reports to expect per commit, which allows us to better surface that information to users in-case of dropped / missing uploads.

Problem 2 - This applies a "delay" to notifications sent to the PR comment. The problem is that the UI is effectively decoupled from this leading to inconsistencies in how coverage is reported for uploads that fail on the UI and PR Comment.

Path Forward

Solve problems 1 & 2

  • For customers that do not have this turned on, Codecov effectively cannot know if all reports have been processed. This means that project coverage reporting is inherently inaccurate. We should surface messaging on PR comment and UI (on commits tab and PR tab) inviting users to turn this on

    • Copy "Codecov isn't confident that it has received all coverage reports for this commit. See how to fix this (link to docs) "
  • Ensure manual_trigger and after_n_builds impacts both PR comment and Codecov UI

For Q4 - the goal would be to wrap up discovery on this topic


Related problem to solve

Previously in #2400 we found that:

  • in app the commits page awaits full data reporting
  • in app pulls data is (xx)
  • pr comment:
    • 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 issue is particularly with the PR comment experience is the data is showing partially as uploads come in and therefore can sometimes give the user a false understanding.

Solution exploration

We'd like to explore 1) how we can make the reporting experience consistent in awaiting to see the full report and 2) have an intermediary state to clarify to the user processing is occurring.

related ongoing iteration to show processing: #2522

@rohan-at-sentry rohan-at-sentry added Area: Report Processing Issues with report processing Area: Report Upload Issues with pre-ingest report uploading in discovery The design, product, and specifications require refinement labels Oct 21, 2024
@rohan-at-sentry rohan-at-sentry added this to the Q4 '24 milestone Oct 21, 2024
@rohan-at-sentry rohan-at-sentry moved this to Consideration / Planning in Codecov's Roadmap Oct 21, 2024
@katia-sentry katia-sentry added the epic this label is used to mark issues as epics label Oct 22, 2024
@trent-codecov trent-codecov added the P0: must do priority 10 label Nov 5, 2024
@rohan-at-sentry
Copy link
Author

rohan-at-sentry commented Nov 12, 2024

Things to talk about during kickoff

  • Can manual_trigger and after_n_builds be replaced by a single "feature"

    • What makes both things required
  • If either manual_trigger or after_n_builds were the default, what are the performance implications (or other platform considerations) that would give us pause

@codecovdesign
Copy link
Contributor

codecovdesign commented Nov 13, 2024

sync with @adrian-codecov @Adal3n3 @rohan-at-sentry @suejung-sentry @spalmurray-codecov

@codecovdesign to create issues and one more sync to debrief: #2400 and ping katia and trent for capacity planning (adrian: good candidate for code freeze time)
rohan: also, let's keep async updates high lever here in this issue

@codecovdesign
Copy link
Contributor

codecovdesign commented Dec 5, 2024

12/5 sync with @Adal3n3 @adrian-codecov @spalmurray-codecov @suejung-sentry

cc @rohan-at-sentry

  • 1.A Applications: Determine Reporting Capabilities #2918
    • Seujung: there are 3 efforts to look at, did a write up on how – summary here is yes we can call GH to get information about the workflow runs and then could we use this in Codecov UI. Such as when the latest run.
    • when calling GH API we can infer the run ID, but that would be required (do we save that in the table?)
    • per design: ideally would be nice to see the latest run. today, we show the initial run and it's not matching the data from the latest run
      - status: next sprint to look at start/stop or intermediary state
  • 1.B Platform: Determine Reporting Capabilities #2919
    - per our last sync 3.B Investigation #2922 (Latest design) decided to 1) show error with report, and 2) IF no error await full processing THEN show report. (The upload processing UI is continuously blocked by the start/stop processing state.)
    - status: Adrian: the investigation is WIP but working on putting different ideas on paper and getting feedback. but ideally we can know when it starts and have intermediary state – then be able to notify. We will setup another sync next Friday 12/13 with everyone in the YYZ office to have another sync.
    - note: while the check roughly is an indicator/timing, it is going to be separate from the concept of the full report – this is when we would send the report to the comment, not related to the check status
    - summary: focused on what to notify and when to notify – there are varying things that influence these area and it's a bit muddy at the moment
    - When Codecov Checks are running we can show partial reports but the challenge here is how would the user know when the report is done.
  • 3.A Investigation: Estimating the expected number of uploads for a commit #2921

@codecovdesign
Copy link
Contributor

12/13 sync with @Adal3n3 @adrian-codecov @spalmurray-codecov @suejung-sentry

@Adal3n3
Copy link

Adal3n3 commented Dec 17, 2024

Without the end state of upload processing, here is the alternative way to communicate pending uploads. View this issue for more details: #2522 (comment)

@codecov-hooky codecov-hooky bot closed this as completed Dec 19, 2024
@github-project-automation github-project-automation bot moved this from Solution Discovery to Done in Codecov's Roadmap Dec 19, 2024
@rohan-at-sentry
Copy link
Author

rohan-at-sentry commented Dec 20, 2024

@katia-sentry @adrian-codecov @suejung-sentry @Adal3n3 @codecovdesign - I see this is marked as complete. Do we have a proposal for how we want to solve this problem? Or are we still figuring it out and hooky is just being aggresive

@adrian-codecov
Copy link

Mmm I don't think the epic should be closed since there's other things open, I think hooky automatically closed it when Katia closed the app's team to complete. I'll reopen

@rohan-at-sentry
Copy link
Author

bad hooky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Report Processing Issues with report processing Area: Report Upload Issues with pre-ingest report uploading epic this label is used to mark issues as epics in discovery The design, product, and specifications require refinement P0: must do priority 10
Projects
Status: Done
Development

No branches or pull requests

7 participants