-
Notifications
You must be signed in to change notification settings - Fork 20
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
docs: Update Celery code owner ADR with failed attempt #345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion for clarity.
Also GitHub isn't letting me comment on this line, but we should update the line "* New celery tasks will need to add the same decorator. See "Rejected Alternatives" for a potential alternative." to reflect that we no longer have a good alternative
@@ -29,8 +29,8 @@ Consequences | |||
(Rejected) Alternatives | |||
----------------------- | |||
|
|||
An untested potential alternative to the ``@set_code_owner_attribute`` decorator is to try celery's `task_prerun signal`_ in an IDA, which would also ensure all future celery tasks are automatically handled. Although this is a potentially superior solution, it was missed at the time of implementation. We will not be switching to this solution at this time given we have a working solution and other priorities, but it is a potentially viable solution if the need arises. | |||
One hoped-for alternative we discovered after the initial ADR was to try setting the code owner in celery's `task_prerun signal`_ in an IDA, which would also ensure all future celery tasks are automatically handled. We `trialed the task_prerun approach <https://github.com/openedx/edx-platform/pull/33180>`_ but discovered that no attribute was set on the Celery tasks. This seems to be because New Relic instruments the task function itself with transaction-start and transaction-end calls, so any signals that are run before or after the task execution occur outside the scope of the transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer to say something like
One hoped-for alternative we discovered after the initial ADR was to try setting the code owner in celery's `task_prerun signal`_ in an IDA, which would also ensure all future celery tasks are automatically handled. We `trialed the task_prerun approach <https://github.com/openedx/edx-platform/pull/33180>`_ but discovered that no attribute was set on the Celery tasks. This seems to be because New Relic instruments the task function itself with transaction-start and transaction-end calls, so any signals that are run before or after the task execution occur outside the scope of the transaction. | |
Setting the code owner in celery's `task_prerun signal`_ in an IDA | |
This would theoretically also ensure all future celery tasks in the IDA are automatically handled. We `trialed the task_prerun approach <https://github.com/openedx/edx-platform/pull/33180>`_ but discovered that no attribute was set on the Celery tasks. This seems to be because New Relic instruments the task function itself with transaction-start and transaction-end calls, so any signals that are run before or after the task execution occur outside the scope of the transaction. |
And then add a change log or something to mention that this alternative was updated after the ADR was written. This way the actual alternative that was rejected is less buried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, add a subheading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of. Just state what the alternative is first before going into the history and the reasons for rejecting it. Doesn't have to be as a header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ready for re-review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.