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

add metric for overall report size #583

Merged
merged 1 commit into from
Aug 2, 2024
Merged

add metric for overall report size #583

merged 1 commit into from
Aug 2, 2024

Conversation

matt-codecov
Copy link
Contributor

@matt-codecov matt-codecov commented Aug 2, 2024

arpad added one for the individual coverage files inside of an upload in #574. this PR does the same for the overall upload

an upload contains one or more coverage files, but also path fixes and the output of git ls-files. so it can be much larger than just the sum of its coverage files. also those coverage files can be b64-encoded which affects the size too

(the .gitignore changes help me use Sapling with this repo and aren't important)

@codecov-staging
Copy link

codecov-staging bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
+ Coverage   97.50%   97.53%   +0.03%     
==========================================
  Files         414      421       +7     
  Lines       35002    35244     +242     
==========================================
+ Hits        34127    34374     +247     
+ Misses        875      870       -5     
Flag Coverage Δ
integration 97.53% <100.00%> (+0.03%) ⬆️
latest-uploader-overall 97.53% <100.00%> (+0.03%) ⬆️
unit 97.53% <100.00%> (+0.03%) ⬆️

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

Components Coverage Δ
NonTestCode 94.64% <99.07%> (+0.08%) ⬆️
OutsideTasks 97.75% <98.90%> (+0.03%) ⬆️
Files Coverage Δ
services/report/__init__.py 92.30% <100.00%> (+0.08%) ⬆️
services/report/prometheus_metrics.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.57%. Comparing base (f236938) to head (47f8960).

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

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #583   +/-   ##
=======================================
  Coverage   97.57%   97.57%           
=======================================
  Files         455      456    +1     
  Lines       36440    36450   +10     
=======================================
+ Hits        35556    35566   +10     
  Misses        884      884           
Flag Coverage Δ
integration 97.53% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 97.53% <100.00%> (+<0.01%) ⬆️
unit 97.53% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.72% <100.00%> (+<0.01%) ⬆️
OutsideTasks 97.75% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/report/__init__.py Critical 92.32% <100.00%> (+0.08%) ⬆️
services/report/prometheus_metrics.py 100.00% <100.00%> (ø)

This change has been scanned for critical changes. Learn more

@codecov-qa
Copy link

codecov-qa bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (f236938) to head (47f8960).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #583   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files         420      421    +1     
  Lines       35234    35244   +10     
=======================================
+ Hits        34364    34374   +10     
  Misses        870      870           
Flag Coverage Δ
integration 97.53% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 97.53% <100.00%> (+<0.01%) ⬆️
unit 97.53% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.64% <100.00%> (+<0.01%) ⬆️
OutsideTasks 97.75% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/report/__init__.py 92.30% <100.00%> (+0.08%) ⬆️
services/report/prometheus_metrics.py 100.00% <100.00%> (ø)

Copy link

codecov-public-qa bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (f236938) to head (47f8960).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #583   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files         420      421    +1     
  Lines       35234    35244   +10     
=======================================
+ Hits        34364    34374   +10     
  Misses        870      870           
Flag Coverage Δ
integration 97.53% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 97.53% <100.00%> (+<0.01%) ⬆️
unit 97.53% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.64% <100.00%> (+<0.01%) ⬆️
OutsideTasks 97.75% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
services/report/__init__.py 92.30% <100.00%> (+0.08%) ⬆️
services/report/prometheus_metrics.py 100.00% <100.00%> (ø)

@matt-codecov
Copy link
Contributor Author

matt-codecov commented Aug 2, 2024

the tests fail with

ValueError: Duplicated timeseries in CollectorRegistry: {'worker_services_report_raw_upload_size_bucket', 'worker_services_report_raw_upload_size', 'worker_services_report_raw_upload_size_sum', 'worker_services_report_raw_upload_size_count', 'worker_services_report_raw_upload_size_created'}

that does not repro locally...

the metrics are both defined as globals. if they are created twice in prometheus's registry, does that mean the services.report module is getting imported twice?

@matt-codecov matt-codecov requested a review from a team August 2, 2024 01:00
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, though you should remove all the print statements before merging. I believe those are just temporary to debug some import issue?

services/report/__init__.py Outdated Show resolved Hide resolved
@matt-codecov
Copy link
Contributor Author

yes the prints are attempting to diagnose the test failure since i can't reproduce it locally. wondering if it's an issue with an image and if i just try again 12h later if it'll work lol

@matt-codecov
Copy link
Contributor Author

matt-codecov commented Aug 2, 2024

after much guesswork i appeased prometheus by declaring the metrics in a submodule instead of in services/reports/__init__.py. no idea why they were getting double-initialized in __init__.py, and why it only happened in CI. possibly a pytest quirk https://stackoverflow.com/questions/69095406/does-pytest-import-modules-twice/73639070#73639070

@matt-codecov matt-codecov dismissed Swatinem’s stale review August 2, 2024 19:27

Feedback was about print statements that were in there while debugging CI-only failures. They're gone

@matt-codecov matt-codecov added this pull request to the merge queue Aug 2, 2024
Merged via the queue into main with commit 8d8882d Aug 2, 2024
25 of 26 checks passed
@matt-codecov matt-codecov deleted the pr583 branch August 2, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants