fix(tasks): Add exception handling and logging for errors#759
Conversation
| reindex_users.delay(user_ids) | ||
| except Exception as e: | ||
| db.session.rollback() | ||
| current_app.logger.exception(e) |
There was a problem hiding this comment.
I think this will also bubble up the exception to sentry, please make sure thats not the case
There was a problem hiding this comment.
But for this case, we do need the exception to get to sentry or not? Or will we go everyday and check the job logs for any errors?
There was a problem hiding this comment.
we need to know that the job has failed. We don't need to know this from sentry necessarily, we have now email notifications on the finished tasks... My doubt there is that exceptions might be raised elsewhere incorrectly. Have you discussed with Alex or Nico on what is expected from their side regarding how the exceptions from tasks are handled? should they be in sentry? I think all the jobs should have consistent behaviour on this, so we need to align with the rest.
There was a problem hiding this comment.
Form what I understand, Sentry exceptions should come up on the things we can fix. And for the spamming of the sentry for other jobs like ORCID, etc, I had a discussion with Pablo, and they seem to be aligned on that as well. That most of the sentry logs for the tasks that are basically 'spam' is due to the fact that we cannot do anything for them. For eg. if the full name is missing in the ORCID job.
^^ This mentioned above is a separate underlying issue in invenio-vocabularies and a potential fix there, not here.
So for this case, on the task level, if there is a duplicate account, we will see the sentry notification and go fix the DB, so we can keep it as an exception. But if we want to rely on email notifications, then yeah we can change it to warning. What do you think?
There was a problem hiding this comment.
Discussed IRL, we will keep this as exception for the reason above
Next step is to fix the DB and re-run the task
| user_ids = users_sync(identities=dict(since=since)) | ||
| reindex_users.delay(user_ids) | ||
| except Exception as e: | ||
| db.session.rollback() |
There was a problem hiding this comment.
won't this rollback all the updates for all the users? can we rollback only one user update?
There was a problem hiding this comment.
We have the DB commit() inside the functions here in invenio-cern-sync, so it will only rollback the errored user
github-project-automation
bot
moved this from In review 🔍
to To release 🤖
in Sprint Q2 2026
2 participants
No description provided.