OEP-50 | initial draft of the hooks extension framework #184
Conversation
|
Thanks for the pull request, @felipemontoya! I've created OSPR-5608 to keep track of it in JIRA. As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
openedx-webhooks
added
core committer
open-source-contribution
felipemontoya
changed the title
|
Hello all, |
openedx-webhooks
added
engineering review
and removed
waiting on author
There was a problem hiding this comment.
Thanks @felipemontoya, I am excited to see this.
We had proposed a much simpler and lighter weight mechanism with some similar ideas in openedx/edx-django-utils#64 ; I'm mentioning it because it's like a "filter" hook that can be easily declared for small functions. If there's some way that these hooks will cover that same use case, it would be nice. (However, our use for that was something quite insignificant: changing the algorithm for selecting XBlock icons. I'm not sure if hooks are used for things that small?)
|
very exciting 👍 |
|
@bradenmacdonald thanks a lot for your comment on the lightweight mechanism created in openedx/edx-django-utils#64. We studied it during this last weeks to see how we could better design what we are proposing. We ended up drawing more inspiration from the social-core pipelines for their flexibility. I don't know what the limit should be for the size or complexity of the hooks framework. For now, I started the proposal with what I though was a comprehensive list of locations that covers the entire lifecycle of a user registering, login in, enrolling, interacting with a course, getting a certificate and loging out. Later we added hooks for the course publication lifecycle. In the future we could add more hooks when the system gains traction and we could also make the exposed triggers somewhat configurable. We have in mind to have a big FEATURE SettingFlag that switches the system on/off. Later on this could be more granular. |
felipemontoya
force-pushed
the
fmo/hooks-extension-framework
branch
from
9b316e0 to
7347384
Compare
|
@nizarmah, @bradenmacdonald. You are both candidates for arbiter of this PR. Given that Braden might also want to be a reviewer I was thinking of writing Nizar in. Do you agree with this? Also, this has been as a draft since February and the first working document started getting feedback since August last year. The latest round of implementation discussion in happening in discuss. The OEP itself has been mostly quiet for a while. Would you agree to give a review period of about a month more, to put a review end date by the 23.04.2021? |
|
@feanil I take from openedx/wg-coordination#5 that you are reviewing this from edX side. Would agree with this date for the OEP as well? Or am I being too aggressive with the timing here? We have a proposed implementation that already incorporates much of the feedback and we want to move forward with making a PR for that as well. We'll post the link once is up. |
|
Here's the draft PR where we're implementing the necessary tooling for the Hooks Extension Framework Please feel free to leave any comments or feedback! |
I think Nizar would be a great choice! I'm also available if you need a backup :) |
|
Thanks @felipemontoya and @bradenmacdonald for the opportunity 😃
I need to schedule some time |
|
I added PR https://github.com/edx/edx-platform/pull/27157 with the first view of how the documentation would look. |
| actions called. | ||
|
|
||
| Although the first document contains a list of the proposed hooks, we now | ||
| proposed that the documentation for this is kept in-code at https://github.com/edx/edx-platform/tree/master/docs/guides. |
There was a problem hiding this comment.
[kudos] I appreciate having the reference doc for these standard APIs be co-located with the codebase that implements those APIs. This also allows other microservices to support such APIs in the future within their own codebase. Though yes, we will focus on enabling this for edx-platform for now.
| must consider that: | ||
|
|
||
| * Most actions will be implemented in plugins. | ||
| * The order of execution for multiple actions/filters listening on a trigger |
There was a problem hiding this comment.
[clarification] Where and how is the order of execution defined?
There was a problem hiding this comment.
With the separation of django signals for events this would be defined via a config dict in django settings. I left the specifics of the format out since they are more of a implementation detail.
| via their results. In this case, the actions called at the time will be called | ||
| filters. | ||
| * Triggers that will not allow the application flow to be altered by the | ||
| actions called. |
There was a problem hiding this comment.
[puzzled] This definition of filters versus actions doesn't match what is written in the Wordpress docs.
Filters do not alter the application flow. Instead, they alter the data that is used within the application flow.
Actions halt the application flow, execute the action, and then return back to the normal flow.
What am I missing in my understanding of this proposal?
There was a problem hiding this comment.
This comment made me clarify a lot of the wording. First I removed "trigger" and "action" from the OEP since it was making it even more complicated. I settled on events and filters (both are kinds of hooks).
I also removed the specific link to the wordpress docs because this proposal is not to copy what Wordpress does, but to be inspired by it.
I clarified the events vs filters part at: https://github.com/edx/open-edx-proposals/pull/184/files#diff-f28146adea40eb552912482a8b24a19107252beba3e25295413bcd5ef8927628R71-R78
| ====================== | ||
|
|
||
| The only extension point that will be affected by this OEP is the | ||
| REGISTRATION_EXTENSION_FORM. This extension point should still be supported |
There was a problem hiding this comment.
[clarification] Is the proposal to reimplement the current extensibility provided by REGISTRATION_EXTENSION_FORM to instead use this new framework?
There was a problem hiding this comment.
I made it clear now. Our suggestion is to keep the current feature until it is clear that it can be implemented using hooks and a migration path has been documented
|
|
||
| * Django Signals was initially considered as the primary mechanism to connect | ||
| triggers and actions, but was ultimately discarded due to the lack of control | ||
| of the execution order when more than one action is listening for a trigger. |
There was a problem hiding this comment.
[seeking more information] Django Signals are the standard Django web framework capability for this type of extension. My initial inclination would be to leverage this capability that already exists in our framework, rather than building and maintaining our own.
What use cases do we have that require a pre-defined execution order?
Also, see my question above since it's not clear how this proposal will satisfy this requirement?
There was a problem hiding this comment.
Now that django signals are back as the mechanism for supporting event hooks, the strict execution order is only there because filters need a way to 'add' or 'reduce' the result of all the functions that were registered.
| .. _two types of hooks: https://developer.wordpress.org/plugins/hooks/#actions-vs-filters | ||
|
|
||
|
|
||
| Performance Considerations |
There was a problem hiding this comment.
[request] Can we provide some basis for debugging and troubleshooting performance and functional errors? For example, can we include how one might support distributed tracing so we can track (1) how long each hook's implementation takes and (2) reliable traces/logs through multiple hooks.
There was a problem hiding this comment.
Added a paragraph at the end of this section about that.
| act on it, and return a modified version for the rest of the core-code to use in | ||
| the next steps of execution. We intend to make the two kinds of hooks | ||
| interchangeable such that developers are not limited by the framework but | ||
| instead given maximum flexibility on its usage to attain their extension goals. |
There was a problem hiding this comment.
[strengthening this proposal] It would be great if you could include a few Use Cases (feature-extension examples) for each of the 2 types of hooks. That would bring to light what each hook could enable for the platform.
There was a problem hiding this comment.
Added a few that I think illustrate what I have in mind.
https://github.com/edx/open-edx-proposals/pull/184/files#diff-f28146adea40eb552912482a8b24a19107252beba3e25295413bcd5ef8927628R196-R216
|
Hello again @felipemontoya! I finally had a chance to go into depth regarding the discussions on discourse, and the proposal and the comments raised.
I think that would be fair or a week after, until 30.04.2021, since some of the discussions on discourse have been getting to a resolution, meanwhile, the ones regarding signals/broadcasts and async are still going on, though. I do have some comments I was considering posting, but I noticed that the proposal hasn't been updated since February 7; therefore, I was thinking of postponing my comments slightly until you have had the chance to update it based on the discourse discussion. |
felipemontoya
force-pushed
the
fmo/hooks-extension-framework
branch
from
8aefce2 to
0478bec
Compare
|
I updated the oep with the latest feedback. In summary:
|
|
@nizarmah @nasthagiri any comments on the current status of this OEP? Are we ready to merge this latest version? |
There was a problem hiding this comment.
@felipemontoya sorry for the late reply. I have a small comment about a possible typo, but as the arbiter of this OEP, I noticed you've addressed the discussion on the forum and the comments on the pull request in the final changes of this OEP.
I'll keep an eye for @nasthagiri's comments and make sure things are addressed and provide my approval again on the pull request.
Thanks for the nice work with the document and the helpful forum updates about the discussions!
|
|
||
| Since the creation of the plugin framework different initiatives in the | ||
| community have demonstrated success by adopting this pattern. This OEP extends | ||
| the range of possible extensions that the patter can hold sustainably. |
There was a problem hiding this comment.
I'm not sure if patter was intended here, or if it's a typo.
There was a problem hiding this comment.
Good eyes. It is indeed a typo. I'll fix it quick
| open edX plugins. Initially, this proposal is meant as a way for community | ||
| members to extend the platform, but it could even become a way for the | ||
| installation at edx.org to implement some of its particular business rules in a | ||
| way that is sufficiently separated from the Open edX core. |
There was a problem hiding this comment.
[+1] Yes, completely agree. Once this framework is in place, it would be good to follow-up with technical refactorings of edx.org specific business logic.
| current receivers of an event. | ||
|
|
||
|
|
||
| Use Cases |
There was a problem hiding this comment.
[kudos] Excellent examples of use cases. Thx!
| common case is to be passed data and return the same of somewhat changed, but | ||
| similar data. Other common case would be to receive data and raise an exception | ||
| in accordance with the definition of the hook. Most likely to completely halt | ||
| the process that would happen after the hook. |
There was a problem hiding this comment.
[appreciation] Nice explanation of the difference between events and filters.
| ======================== | ||
|
|
||
| The reference implementation must be completed before this OEP is given "Final" | ||
| status. It is not necessary for the "accepted" status. |
There was a problem hiding this comment.
[request] hmm... Can we replace "Final" with "Accepted" and "accepted" with "Provisional"?
The reference implementation must be completed before this OEP is given "Accepted" status. It is not necessary for the "Provisional" status.
Rationale: We don't have any OEPs in the "Final" state and should probably update OEP-1 to exclude that state altogether. "Accepted" is synonymous with the community accepting and adopting the proposal. We did introduce a new state called "Provisional", which means the OEP was reviewed and tentatively accepted while the work is being implemented and piloted.
| When possible, hooks should be compatible with the Async Event Messaging | ||
| described in OEP-41. | ||
| In terms of error handling, the implementation should allow the configuration of | ||
| silent failures letting operators know the source of the error. |
There was a problem hiding this comment.
[+1] If I understand this statement correctly, the implementation will have the means to debug issues (logging statements, etc) to help operators understand the root cause of performance and functional issues caused by hooks.
There was a problem hiding this comment.
Yes, but cautiously. By this I mean that we definitely need to put some tooling in place in the implementation to allow debugging and help operators know what is going on. However we also don't want to make the library implementing this bloated by too many extra tools and scaffolding.
felipemontoya
force-pushed
the
fmo/hooks-extension-framework
branch
from
0478bec to
13c0f55
Compare
|
Thanks a lot @nasthagiri and @nizarmah. I updated the two suggestions you made and went ahead to make the status be Provisional. Would agree to merge this in this Provisional state so that we can focus on the discussions at the libraries about naming and ergonomics? I'd like to merge so that this starts to appear at https://open-edx-proposals.readthedocs.io/en/latest/ |
|
Yes, go for it. I had approved the PR and we had kept it open for comments already. |
|
@felipemontoya 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
OEP-50 | initial draft of the hooks extension framework
This PR converts the previous proposal held in a google doc to a formal OEP