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

feat: add TestResultsFlow for measuring time to notification #439

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joseph-sentry
Copy link
Contributor

No description provided.

Copy link

sentry-io bot commented May 8, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: tasks/upload.py

Function Unhandled Issue
run_impl NoConfiguredAppsAvailable app.tasks.upload.Upload
Event Count: 171

Did you find this useful? React with a 👍 or 👎

@codecov-qa
Copy link

codecov-qa bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.34%. Comparing base (966d2e8) to head (20ec9ae).

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #439   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files         399      399           
  Lines       33607    33630   +23     
=======================================
+ Hits        32715    32738   +23     
  Misses        892      892           
Flag Coverage Δ
integration 97.34% <95.83%> (+<0.01%) ⬆️
latest-uploader-overall 97.34% <95.83%> (+<0.01%) ⬆️
unit 97.34% <95.83%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.59% <95.83%> (+<0.01%) ⬆️
OutsideTasks 97.50% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/checkpoint_logger/flows.py 100.00% <100.00%> (ø)
tasks/test_results_finisher.py 95.23% <100.00%> (+0.23%) ⬆️
tasks/tests/unit/test_upload_task.py 99.49% <ø> (ø)
tasks/upload.py 91.06% <85.71%> (-0.14%) ⬇️

... and 1 file with indirect coverage changes

@codecov-notifications
Copy link

codecov-notifications bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #439   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files         399      399           
  Lines       33607    33630   +23     
=======================================
+ Hits        32715    32738   +23     
  Misses        892      892           
Flag Coverage Δ
integration 97.34% <95.83%> (+<0.01%) ⬆️
latest-uploader-overall 97.34% <95.83%> (+<0.01%) ⬆️
unit 97.34% <95.83%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.59% <95.83%> (+<0.01%) ⬆️
OutsideTasks 97.50% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/checkpoint_logger/flows.py 100.00% <100.00%> (ø)
tasks/test_results_finisher.py 95.23% <100.00%> (+0.23%) ⬆️
tasks/tests/unit/test_upload_task.py 99.49% <ø> (ø)
tasks/upload.py 91.06% <85.71%> (-0.14%) ⬇️

... and 1 file with indirect coverage changes

Copy link

codecov-public-qa bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.34%. Comparing base (966d2e8) to head (20ec9ae).

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #439   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files         399      399           
  Lines       33607    33630   +23     
=======================================
+ Hits        32715    32738   +23     
  Misses        892      892           
Flag Coverage Δ
integration 97.34% <95.83%> (+<0.01%) ⬆️
latest-uploader-overall 97.34% <95.83%> (+<0.01%) ⬆️
unit 97.34% <95.83%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.59% <95.83%> (+<0.01%) ⬆️
OutsideTasks 97.50% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/checkpoint_logger/flows.py 100.00% <100.00%> (ø)
tasks/test_results_finisher.py 95.23% <100.00%> (+0.23%) ⬆️
tasks/tests/unit/test_upload_task.py 99.49% <ø> (ø)
tasks/upload.py 91.06% <85.71%> (-0.14%) ⬇️

... and 1 file with indirect coverage changes

Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.37%. Comparing base (966d2e8) to head (20ec9ae).

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     #439   +/-   ##
=======================================
  Coverage   97.37%   97.37%           
=======================================
  Files         430      430           
  Lines       34297    34320   +23     
=======================================
+ Hits        33396    33419   +23     
  Misses        901      901           
Flag Coverage Δ
integration 97.34% <95.83%> (+<0.01%) ⬆️
latest-uploader-overall 97.34% <95.83%> (+<0.01%) ⬆️
unit 97.34% <95.83%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 94.61% <95.83%> (+<0.01%) ⬆️
OutsideTasks 97.50% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/checkpoint_logger/flows.py 100.00% <100.00%> (ø)
tasks/test_results_finisher.py 95.91% <100.00%> (+0.20%) ⬆️
tasks/tests/unit/test_upload_task.py 99.50% <ø> (ø)
tasks/upload.py Critical 91.11% <85.71%> (-0.14%) ⬇️

... and 1 file with indirect coverage changes

This change has been scanned for critical changes. Learn more

@joseph-sentry joseph-sentry requested a review from a team May 13, 2024 19:38
Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

It might be interesting to add another checkpoint in the beginning of TestResultsFinisherTask.process_impl_within_lock like TEST_RESULTS_PROCESSING_BEGIN.

THe subflow TEST_RESULTS_BEGIN --> TEST_RESULTS_FINISHER_BEGIN would give us a rough estimate of processing time. (I think that the finisher task only runs after all the processor tasks finish, and that we don't current have a metric for that)

What do you think?

But in general lgtm 👍
Very exciting to see more Flows

@success_events("TEST_RESULTS_BEGIN")
@subflows(
("notification_latency", "TEST_RESULTS_BEGIN", "TEST_RESULTS_NOTIFY"),
("notification_latency", "TEST_RESULTS_BEGIN", "FLAKE_DETECTION_NOTIFY"),
Copy link
Contributor

Choose a reason for hiding this comment

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

should the subflows have the same name?

Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
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.

None yet

2 participants