ref: Refactor upload task to accomodate multiple report types #190

scott-codecov · 2023-11-22T19:34:51Z

This is a proposal for how we could potentially handle the ingest of multiple types of upload. It assumes we're reusing the existing CommitReport model and adding a new report_type column to that table. Existing behavior should be unchanged (for report_type = "coverage", the default) and allows us to reuse much of the ancillary support behavior that the upload task currently provides (fetching commit info, all the locking around processing, etc.).

When report_type = "coverage" we create our regular chain of tasks (upload processor -> finisher -> notify) but we would likely trigger other (new) tasks to handle the processing and notification related to other types of ingests.

I noticed that we were passing around repoid + commitid + report_type + report_code a lot so I also refactored that into a new class called UploadArgs which encapsulates these (together with the arguments that are passed to the task via Redis).

The existing ReportService has some generic methods (like creating/retrieving uploads) but we'd want to create different adapters for each report type. I left some comments in the code about specific interfaces we're currently relying on.

codecov-staging · 2023-11-22T19:39:24Z

Codecov Report

Merging #190 (4a810e6) into main (e187e61) will decrease coverage by 0.02%.
The diff coverage is 96.26%.

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   98.34%   98.33%   -0.02%     
==========================================
  Files         351      351              
  Lines       27721    27763      +42     
==========================================
+ Hits        27262    27300      +38     
- Misses        459      463       +4

Flag	Coverage Δ
integration	`98.33% <96.26%> (-0.02%)`	⬇️
latest-uploader-overall	`98.33% <96.26%> (-0.02%)`	⬇️
unit	`98.33% <96.26%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`96.76% <95.34%> (-0.03%)`	⬇️
OutsideTasks	`98.13% <100.00%> (ø)`

Files	Coverage Δ
services/report/__init__.py	`97.92% <100.00%> (ø)`
tasks/tests/unit/test_upload_task.py	`99.45% <100.00%> (+<0.01%)`	⬆️
tasks/upload.py	`97.60% <95.29%> (-1.47%)`	⬇️

codecov-qa · 2023-11-22T19:39:33Z

Codecov Report

Merging #190 (0fe87c6) into main (c7bb826) will decrease coverage by 0.02%.
The diff coverage is 95.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   98.33%   98.32%   -0.02%     
==========================================
  Files         358      358              
  Lines       28869    28923      +54     
==========================================
+ Hits        28389    28438      +49     
- Misses        480      485       +5

Flag	Coverage Δ
integration	`98.32% <95.93%> (-0.02%)`	⬇️
latest-uploader-overall	`98.32% <95.93%> (-0.02%)`	⬇️
unit	`98.32% <95.93%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`96.68% <95.09%> (-0.04%)`	⬇️
OutsideTasks	`98.09% <100.00%> (+<0.01%)`	⬆️

Files	Coverage Δ
database/enums.py	`100.00% <100.00%> (ø)`
database/models/reports.py	`99.20% <100.00%> (+<0.01%)`	⬆️
services/report/__init__.py	`97.93% <100.00%> (+<0.01%)`	⬆️
tasks/backfill_commit_data_to_storage.py	`97.29% <100.00%> (+0.03%)`	⬆️
tasks/save_report_results.py	`98.75% <100.00%> (+0.01%)`	⬆️
tasks/tests/unit/test_upload_task.py	`99.45% <100.00%> (+<0.01%)`	⬆️
tasks/upload.py	`97.25% <94.56%> (-1.83%)`	⬇️

codecov · 2023-11-22T19:40:31Z

Codecov Report

Merging #190 (0fe87c6) into main (c7bb826) will decrease coverage by 0.02%.
The diff coverage is 95.96%.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   98.30%   98.29%   -0.02%     
==========================================
  Files         389      389              
  Lines       29565    29620      +55     
==========================================
+ Hits        29064    29114      +50     
- Misses        501      506       +5

Flag	Coverage Δ
integration	`98.32% <95.93%> (-0.02%)`	⬇️
latest-uploader-overall	`98.32% <95.93%> (-0.02%)`	⬇️
unit	`98.32% <95.93%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`96.57% <95.14%> (-0.03%)`	⬇️
OutsideTasks	`98.09% <100.00%> (+<0.01%)`	⬆️

Files	Coverage Δ
database/enums.py	`100.00% <100.00%> (ø)`
database/models/reports.py	`99.20% <100.00%> (+<0.01%)`	⬆️
services/report/__init__.py Critical	`97.93% <100.00%> (+<0.01%)`	⬆️
tasks/backfill_commit_data_to_storage.py	`97.29% <100.00%> (+0.03%)`	⬆️
tasks/save_report_results.py	`98.75% <100.00%> (+0.01%)`	⬆️
tasks/tests/unit/test_upload_task.py	`99.45% <100.00%> (+<0.01%)`	⬆️
tasks/upload.py Critical	`97.27% <94.62%> (-1.81%)`	⬇️

Related Entrypoints
run/app.tasks.upload.Upload

giovanni-guidini · 2023-11-24T12:44:40Z

I like this idea in general. I think that if we can re-use the entry point of uploads processing it's gonna be a positive thing.
I think in the future there are other optimizations we might try to make, mostly in terms of reducing the span of the code that runs within a lock, but these should be done incrementally.

The refactor itself was very good. Special thanks to the helpful TODO comments.
I like how logic that was all over the place is isolated in a single class (UploadArgs), although (and this is nit) I don't like the name. There's much logic in UploadArgs, that sounds to me like a data container. I'd call it either UploadContext or UploadContextManager, and you may or may not choose to have an UploadArgs data container in there. Like I said, nit stuff.

scott-codecov · 2023-11-27T19:17:07Z

Thanks for taking a look @giovanni-guidini. I agree that UploadContext is a better name.

* main: Only trigger AI PR review if pull is open Add log line when triggering AI PR review task fix: use original PR base to compute patch coverage (#199) Prepare release 23.12.4 Update workflows chore(deps): Update codecov-shared dependency (#194) Prep the terrain for reports with label compression. (#188) allow staging deploy when pushing to staging (#192)

codecov-public-qa · 2023-12-06T15:35:25Z

Codecov Report

Merging #190 (0fe87c6) into main (c7bb826) will decrease coverage by 0.02%.
The diff coverage is 95.93%.

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   98.33%   98.32%   -0.02%     
==========================================
  Files         358      358              
  Lines       28869    28923      +54     
==========================================
+ Hits        28389    28438      +49     
- Misses        480      485       +5

Flag	Coverage Δ
integration	`98.32% <95.93%> (-0.02%)`	⬇️
latest-uploader-overall	`98.32% <95.93%> (-0.02%)`	⬇️
unit	`98.32% <95.93%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`96.68% <95.09%> (-0.04%)`	⬇️
OutsideTasks	`98.09% <100.00%> (+<0.01%)`	⬆️

Files	Coverage Δ
database/enums.py	`100.00% <100.00%> (ø)`
database/models/reports.py	`99.20% <100.00%> (+<0.01%)`	⬆️
services/report/__init__.py	`97.93% <100.00%> (+<0.01%)`	⬆️
tasks/backfill_commit_data_to_storage.py	`97.29% <100.00%> (+0.03%)`	⬆️
tasks/save_report_results.py	`98.75% <100.00%> (+0.01%)`	⬆️
tasks/tests/unit/test_upload_task.py	`99.45% <100.00%> (+<0.01%)`	⬆️
tasks/upload.py	`97.25% <94.56%> (-1.83%)`	⬇️

scott-codecov · 2023-12-06T16:58:21Z

This is ready for a proper review now. After some discussion we decided that this was indeed the path forward that we're going to take for handling other types of uploads. I made the migration to add the new report_type column (codecov/codecov-api#287) and values of null or "coverage" are interpreted currently to mean coverage reports. It may be worth backfilling existing reports with "coverage" so we can remove the nullability on that column.

There's TODOs where the other types of ingest can be handled. @joseph-sentry and I can plug in our adapters for test results and bundle analysis there but I figured it'd be good to get this merged and tested to make sure the existing behavior is not changed for coverage uploads.

joseph-sentry

approving this, but I think in the future we should separate the checkpoint logger upload flows for each report type

scott-codecov · 2023-12-11T15:02:38Z

approving this, but I think in the future we should separate the checkpoint logger upload flows for each report type

I could just begin the existing checkpoint flow only for report_type="coverage" right now and then we could eventually have a mapping of report_type -> checkpoint flow once we figure out all the task steps for each report type. WDYT?

* main: Change default upper section from "header" to "condensed_header" (#209) Add logs for some path fixes for each raw upload processing (#135) Create CleanLabelIndexTask (#205) fix: check if labels before popping (#208)

ref: Refactor upload task to accomodate multiple report types

4a810e6

scott-codecov marked this pull request as draft November 22, 2023 19:34

scott-codecov mentioned this pull request Dec 4, 2023

feat: Add report_type column to commit reports codecov/codecov-api#287

Merged

scott-codecov added 2 commits December 6, 2023 09:58

Handle new commit_report.report_type column

5b41794

Merge branch 'main' into scott/upload-report-type

8e65bcc

scott-codecov marked this pull request as ready for review December 6, 2023 16:54

scott-codecov requested review from giovanni-guidini, joseph-sentry and matt-codecov December 6, 2023 16:55

scott-codecov mentioned this pull request Dec 8, 2023

feat: Bundle analysis upload endpoint codecov/codecov-api#299

Merged

joseph-sentry approved these changes Dec 8, 2023

View reviewed changes

joseph-sentry mentioned this pull request Dec 8, 2023

feat: add models and task for failed test ingestion #197

Merged

scott-codecov added 3 commits December 12, 2023 13:10

Merge branch 'main' into scott/upload-report-type

57c2fed

* main: Change default upper section from "header" to "condensed_header" (#209) Add logs for some path fixes for each raw upload processing (#135) Create CleanLabelIndexTask (#205) fix: check if labels before popping (#208)

Merge branch 'main' into scott/upload-report-type

2474b76

Merge branch 'main' into scott/upload-report-type

0fe87c6

scott-codecov merged commit b71cc41 into main Dec 13, 2023
17 of 26 checks passed

scott-codecov deleted the scott/upload-report-type branch December 13, 2023 19:12

scott-codecov mentioned this pull request Dec 14, 2023

fix: Add some missing report type filtering #214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Refactor upload task to accomodate multiple report types #190

ref: Refactor upload task to accomodate multiple report types #190

scott-codecov commented Nov 22, 2023

codecov-staging bot commented Nov 22, 2023 •

edited

Loading

codecov-qa bot commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading

giovanni-guidini commented Nov 24, 2023

scott-codecov commented Nov 27, 2023

codecov-public-qa bot commented Dec 6, 2023 •

edited

Loading

scott-codecov commented Dec 6, 2023

joseph-sentry left a comment

scott-codecov commented Dec 11, 2023 •

edited

Loading

ref: Refactor upload task to accomodate multiple report types #190

ref: Refactor upload task to accomodate multiple report types #190

Conversation

scott-codecov commented Nov 22, 2023

codecov-staging bot commented Nov 22, 2023 • edited Loading

Codecov Report

codecov-qa bot commented Nov 22, 2023 • edited Loading

Codecov Report

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

giovanni-guidini commented Nov 24, 2023

scott-codecov commented Nov 27, 2023

codecov-public-qa bot commented Dec 6, 2023 • edited Loading

Codecov Report

scott-codecov commented Dec 6, 2023

joseph-sentry left a comment

Choose a reason for hiding this comment

scott-codecov commented Dec 11, 2023 • edited Loading

codecov-staging bot commented Nov 22, 2023 •

edited

Loading

codecov-qa bot commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading

codecov-public-qa bot commented Dec 6, 2023 •

edited

Loading

scott-codecov commented Dec 11, 2023 •

edited

Loading