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

Handle TA upload error #998

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft

Handle TA upload error #998

wants to merge 34 commits into from

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Jan 9, 2025

No description provided.

these 2 new tasks are meant to replace the test results processor and
test results finisher tasks

the difference between the tasks is that the new tasks:
- use the new parse_raw_upload function provided by the test results
  parser library
- instead of writing to the database in the processor, the finisher
  takes care of writing to the database
- the processor writes the results of its parsing and the finisher pulls
  from that
the upload task will check the new ta tasks feature flag to determine
whether to use the newly introduced ta processor and ta finisher tasks

we also call the new ta processor and ta finisher tasks via a chord
since we removed the concurrent db writes from the processors
also update test results parser version
we don't need to chunk them anymore and each upload can get its own
processing task
i want to introduce the new finished state in the test results upload
pipeline

to do so safely i'm adding the new v2_processed and v2_finished states

the reason for this is that the meaning of processed and v2_processed
are different

processed means that test instances are in the db and persisted
but in the new pipeline v2_finished has that meaning and v2_processed
just means that the intermediate results are in redis for now
this commit changes the TA upload states to have the v2_persisted
state which represents the test run data being persisted to the db
and the v2_finished state representing that the upload was taken into
account when making the latest commment on the PR

this also removes the v2_failed state since i want an upload to both
be able to have valid test runs and have some failed parsing, the
v2_failed state made it seem like it was either processed or failed
when it could be both

the failures related to an upload will instead be represented by upload
errors
this is an abstraction to help us use bigquery for test analytics data

this abstraction allows us to write data to bigquery using the Storage
Write API and query it using SQL statements as strings, it also handles
creating the connection to bigquery for us

the use protocol buffers is a consequence of using the Storage Write API

feat: create TADriver abstraction

this commit creates the TADriver abstraction which (for now) exposes a
write_testruns method, which will take care of persisting a list of
testruns to some specific storage

this commit implements this interface for Postgres and for Bigquery

the idea is that we will start by persisting testruns to both, then
eventually we add a method to the interface for retrieving the
aggregates at which point it will be safe for our cloud offering to
switch over to only bigquery for storage of TA testruns.
remove msgpack
add pytest-insta
update test-results-parser
update type hints usage from test_results_parser
don't update upload state in pg.py
gist of it is we're keeping the new upload states but we're reverting
the msgpack changes

we're also using the ta_storage utilities for persisting data in the
processor now

and we're using pytest-insta for snapshots in the tests
we need to update the test results tasks' use of the test results
parser library
Copy link

github-actions bot commented Jan 9, 2025

This PR includes changes to shared. Please review them here: codecov/shared@609e56d...d7d0096

@codecov-notifications
Copy link

Codecov Report

Attention: Patch coverage is 95.26854% with 37 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/ta_finisher.py 86.70% 21 Missing ⚠️
tasks/ta_processor.py 92.68% 6 Missing ⚠️
services/test_results.py 88.09% 5 Missing ⚠️
services/comparison/__init__.py 80.00% 2 Missing ⚠️
tasks/test_results_processor.py 92.85% 2 Missing ⚠️
tasks/notify.py 95.23% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link

github-actions bot commented Jan 9, 2025

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 95.26854% with 37 lines in your changes missing coverage. Please review.

Project coverage is 97.72%. Comparing base (78ce991) to head (b96909a).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/ta_finisher.py 86.70% 21 Missing ⚠️
tasks/ta_processor.py 92.68% 6 Missing ⚠️
services/test_results.py 88.09% 5 Missing ⚠️
services/comparison/__init__.py 80.00% 2 Missing ⚠️
tasks/test_results_processor.py 92.85% 2 Missing ⚠️
tasks/notify.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
- Coverage   97.74%   97.72%   -0.03%     
==========================================
  Files         454      452       -2     
  Lines       36368    36768     +400     
==========================================
+ Hits        35548    35930     +382     
- Misses        820      838      +18     
Flag Coverage Δ
integration 42.49% <62.40%> (+0.47%) ⬆️
unit 90.20% <65.98%> (-0.27%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

⚠️ Impact Analysis from Codecov is deprecated and will be sunset on Jan 31 2025. See more

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.

1 participant