3169 move notifications presteps to one task #989
Conversation
…ications-presteps-to-one-task
…ications-presteps-to-one-task
| empty_upload=empty_upload, | ||
| **kwargs, | ||
| ) | ||
| # TODO: What should happen when there's a lock error? I think this should change to some retrying mechanism |
There was a problem hiding this comment.
I'm thinking when 2 finishers are done (presumably with different results), one picks up this lock and the other will wait. I suspect the 2nd one should be able to pick up this task
| ), | ||
| ) | ||
| UploadFlow.log(UploadFlow.NOTIF_LOCK_ERROR) | ||
| return { |
There was a problem hiding this comment.
TODO for me: I should change this to ShouldCallNotifyResponse
| assert commit, "Commit not found in database." | ||
| commit_yaml = UserYaml(commit_yaml) | ||
|
|
||
| log_extra_dict = { |
There was a problem hiding this comment.
I pass around this log bw functions, but there might be a better strategy to do so, perhaps leveraging the context 👀
There was a problem hiding this comment.
repoid and commitid are already on the log context if that's been re-enabled
there are no examples of this, but in principle if you have task-specific fields you would like to log you can subclass LogContext, add your own fields, override as_dict(), and call set_log_context(my_ctx) at the start of your task. i think because of dataclass quirks all of your fields must have default values (or you need to rework LogContext to be kw_only=True)
if something truly makes sense on every log across the board you can just add it to the log context itself but i don't think we should cram too much in there. commit_yaml probably doesn't belong there for example; we're never going to search by it and it isn't going to change so we only really need that on one log message
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| "countdown": countdown, | ||
| "max_retries": max_retries, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
We should add a UploadFlow log for "reattempting notification" or something
❌ 24 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| # Defines all the logic to consider a notification | ||
| should_notify_check_functions = [ | ||
| partial( | ||
| self.upload_processing_checks, processing_results=processing_results |
There was a problem hiding this comment.
These types of notifications could in theory happen before we even grab the lock, but leaving them here for now to be consistent with where the rest of the checks get done.
I also don't think the check order matters here, just as long as we make them, but I could be wrong
There was a problem hiding this comment.
i also assume order doesn't matter here but i would not be shocked if it does :P
don't have to do it now, but it might be a good idea to make each of these functions async and then asyncio.gather() or asyncio.as_completed() them to run them concurrently. could speed up notifications a good amount
…fications-presteps-to-one-task
| # This is currently not used, but my idea here was to provide it and log this message in switch statements. | ||
| # Logging could become a bit less flexible but there's less logging altogether. Thoughts? | ||
| message: str |
There was a problem hiding this comment.
how would message be different from reason?
| message: str | ||
|
|
||
|
|
||
| # TODO: Ask: does this need a throws = (SoftTimeLimitExceeded,)? |
There was a problem hiding this comment.
never noticed this throws= thing before. thiago added one to the notify task in 46c9bc5, looks like it's to tell celery that a certain uncaught exception is not necessarily an error...? interesting
i would say it should be considered an error if this task times out. if it happens a lot we can potentially async-ify the conditions and perform them concurrently, we're not totally helpless
| assert commit, "Commit not found in database." | ||
| commit_yaml = UserYaml(commit_yaml) | ||
|
|
||
| log_extra_dict = { |
There was a problem hiding this comment.
repoid and commitid are already on the log context if that's been re-enabled
there are no examples of this, but in principle if you have task-specific fields you would like to log you can subclass LogContext, add your own fields, override as_dict(), and call set_log_context(my_ctx) at the start of your task. i think because of dataclass quirks all of your fields must have default values (or you need to rework LogContext to be kw_only=True)
if something truly makes sense on every log across the board you can just add it to the log context itself but i don't think we should cram too much in there. commit_yaml probably doesn't belong there for example; we're never going to search by it and it isn't going to change so we only really need that on one log message
| self.app.tasks[pulls_task_name].apply_async( | ||
| kwargs=dict( | ||
| repoid=repoid, | ||
| pullid=pull.pullid, | ||
| should_send_notifications=False, | ||
| ) | ||
| ) | ||
| compared_to = pull.get_comparedto_commit() | ||
| if compared_to: | ||
| comparison = get_or_create_comparison( | ||
| db_session, compared_to, commit | ||
| ) | ||
| db_session.commit() | ||
| self.app.tasks[ | ||
| compute_comparison_task_name | ||
| ].apply_async( | ||
| kwargs=dict(comparison_id=comparison.id) | ||
| ) |
There was a problem hiding this comment.
these side-effects don't sound notification-related. if they need to happen, they probably belong here
in fact, i wonder if there is a bug here. currently we kick off the notification and then kick off these side-effects. the notification does not wait for the PR to be updated or for the comparison to be computed, it'll just go. does that mean it will have stale information? will it duplicate the work that these tasks are doing asynchronously? we might need to change this to a chain/chord that will kick the notification off after the pull/compare tasks finish
| # ASK: this installation/bot related logic impedes notification if unavailable, but it | ||
| # seems correct for it to be part of the notifier task, thoughts? |
There was a problem hiding this comment.
i think the git provider app/token stuff is a tricky gray area
if you imagine a generic notification system, there are roughly three stages:
- deciding whether to post this kind of notification based on business logic
- i would say app/token stuff doesn't belong in this stage
- computing the underlying data for this notification
- some data (e.g. patch coverage) may require credentials to compute. so the coverage product for example has to decide whether to totally fail without credentials or just skip patch coverage
- sending the Slack template to Slack, the PR Comment template to the PR Comment, etc
- the Slack notification doesn't care if we don't have git provider credentials, but the PR comment does
ideally, missing credentials would only skip the things that definitely require credentials and the rest of the system would continue working. and when credentials are missing, that's a misconfiguration on the part of the user and we should alert them to it in the UI but otherwise not treat it as an error
i think your NO task is implementing the first stage of the generic notification system, where app/token stuff shouldn't be a factor. the NotifyTask represents the second and third stages, and cleaning that up to work like i described is a separate matter
| # Defines all the logic to consider a notification | ||
| should_notify_check_functions = [ | ||
| partial( | ||
| self.upload_processing_checks, processing_results=processing_results |
There was a problem hiding this comment.
i also assume order doesn't matter here but i would not be shocked if it does :P
don't have to do it now, but it might be a good idea to make each of these functions async and then asyncio.gather() or asyncio.as_completed() them to run them concurrently. could speed up notifications a good amount
| # This is technically a different repository_service method than the one used in the notify task. I | ||
| # replaced it because the other method is a) deeply intertwined with the notify file and b) it seems | ||
| # to be extra specific just for the notification aspect of it, so this should suffice for other checks. | ||
| # Please look corroborate this isn't a red flag. |
There was a problem hiding this comment.
is this a note to yourself or to reviewers?
| # Some check on CI skipping from some regex? Idk what this is | ||
| if regexp_ci_skip.search(commit.message or ""): |
There was a problem hiding this comment.
So, this PR stems from us having many notification conditions spread out in different tasks (finisher, notify) and this is my take for it. I decided to create a task, notification_orchestrator, that's meant to be a centralized place for doing checks that determine whether we'd notify or not, rather than doing it when we're already in the notify task (and expecting to notify). The idea is for any codecov product to tap into this orchestrator if they desire to do so, although this first pass is targeted mostly for the coverage case.
I would recommend solely looking at
notification_orchestrator.pyand giving feedback there. The changes inupload_finisher.pyandnotify.pytask are mostly deletions - these are the changes that now belong in the orchestrator task. I have not yet tested this orchestrator task fully, so this is moreso a pass about it capturing the logic well and if it seems a reasonable/readable approach for notifying. I'm easy with renaming and whatnot, just let me know what you think :)I'm happy for this PR to just be adding the task, and in a future PR connecting it to the finisher/notify task (I'd modify this PR to revert the changes in those two files as well).
I left some inline comments as questions and I'll leave some more as PR comments. Please let me know about this when you get a chance. After the 1st pass, I'll work on the tests as well.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.