From cfe6086df8e4cbd004b17062d0ac20554494fff0 Mon Sep 17 00:00:00 2001 From: mariagrimaldi Date: Wed, 12 May 2021 15:21:08 -0400 Subject: [PATCH 1/5] docs: add ADR for Open edX event naming --- docs/decisions/0002-events-naming.rst | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/decisions/0002-events-naming.rst diff --git a/docs/decisions/0002-events-naming.rst b/docs/decisions/0002-events-naming.rst new file mode 100644 index 00000000..cc2d3bcf --- /dev/null +++ b/docs/decisions/0002-events-naming.rst @@ -0,0 +1,58 @@ +2. Open edX events naming +========================= + +Status +------ + +Draft + +Context +------- + +Besides a suitable location, event-type hooks need a form of +identification when adding them to the Open edX platform. By doing this, +recognizing which event to use, or debugging errors will be easier. + +This ADR has the purpose of defining the rules to be followed when +naming an Open edX Event. + +Decisions +--------- + +1. Given that signals don't have an explicit name, the identifier will be +the module path and variable where the signal is stored. + +2. While trying to follow the format defined in the `OEP-41`_: + +``{Reverse DNS}.{Architecture Subdomain}.{Subject}.{Action}.{Major Version}`` + +We encountered two major issues for our use case: + +* Reverse DNS: if we use the namespace ``org.openedx``, then our package would + be ``openedx-events/org/openedx/openedx_events/.../`` which will break + conventions for Python packages in the Open edX ecosystem. To be consistent, + the proposal is to remove ``Reverse DNS`` and use a ``Namespace`` that in our + case would be ``openedx_events``. + +* Major version location: following the same idea, when using ``Major version`` + at the end, the name would be ``/openedx_events/.../action/`` meaning the + signal will be stored in the variable v1, which is not suitable given our + architecture. To be consistent with our design, the proposal is to place + ``Major Version`` before ``Subject``. + +Applying the proposals will result in: + +``{Namespace}.{Architecture Subdomain}.{Major Version}.{Subject}.{Action}`` + +Examples: + +* openedx_filters.learning.v1.course.enrollment.created +* openedx_filters.learning.v1.student.registration.completed +* openedx_filters.learning.v1.session.login.completed + +.. _OEP-41: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#specification + +Consequences +------------ + +* All events defined in this repository must follow the same format. From b15652137590beee8753dee921553c59063a64f4 Mon Sep 17 00:00:00 2001 From: mariagrimaldi Date: Fri, 14 May 2021 15:26:44 -0400 Subject: [PATCH 2/5] fix: addressing feedback comments --- docs/decisions/0002-events-naming.rst | 60 +++++++++++++++++---------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/docs/decisions/0002-events-naming.rst b/docs/decisions/0002-events-naming.rst index cc2d3bcf..34aa4c90 100644 --- a/docs/decisions/0002-events-naming.rst +++ b/docs/decisions/0002-events-naming.rst @@ -19,40 +19,56 @@ naming an Open edX Event. Decisions --------- -1. Given that signals don't have an explicit name, the identifier will be -the module path and variable where the signal is stored. +The following decisions were made after receiving feedback from the Open edX +community on this repository and on other platforms such as discuss. -2. While trying to follow the format defined in the `OEP-41`_: +1. The name will be a ``string`` that follows the `type format`_ defined in +the `OEP-41`_ with a minor change: -``{Reverse DNS}.{Architecture Subdomain}.{Subject}.{Action}.{Major Version}`` +``{Reverse DNS}.{Architecture Subdomain}.{Subject}.{Action}`` -We encountered two major issues for our use case: +Examples: -* Reverse DNS: if we use the namespace ``org.openedx``, then our package would - be ``openedx-events/org/openedx/openedx_events/.../`` which will break - conventions for Python packages in the Open edX ecosystem. To be consistent, - the proposal is to remove ``Reverse DNS`` and use a ``Namespace`` that in our - case would be ``openedx_events``. +* org.openedx.learning.course.enrollment.created +* org.openedx.learning.student.registration.completed +* org.openedx.learning.session.login.completed -* Major version location: following the same idea, when using ``Major version`` - at the end, the name would be ``/openedx_events/.../action/`` meaning the - signal will be stored in the variable v1, which is not suitable given our - architecture. To be consistent with our design, the proposal is to place - ``Major Version`` before ``Subject``. +As can be seen, ``{Major Version}`` was removed from the name, meaning, +events won't be versioned. Instead, the data sent by them will be: -Applying the proposals will result in: +.. code-block:: python -``{Namespace}.{Architecture Subdomain}.{Major Version}.{Subject}.{Action}`` + providing_args={ + "user_data": UserData, + "registration_data": RegistrationData, + "registration_data_v2": RegistrationDataV2, + } -Examples: +2. The name will be sent as metadata for the events to +help with debugging, logging, or any other process related to events. + +For example: + +.. code-block:: python + + # Event definition + STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal( + event_type="org.openedx.learning.student.registration.completed", + providing_args={ + "user_data": UserData, + "registration_data": RegistrationData, + ... + } + ) -* openedx_filters.learning.v1.course.enrollment.created -* openedx_filters.learning.v1.student.registration.completed -* openedx_filters.learning.v1.session.login.completed +3. The definition of the signal does not have a one-to-one mapping to the name +proposed here. The definition will be done in such a way that it's the most +usable and accessible for the developer. +.. _type format: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#id5 .. _OEP-41: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#specification Consequences ------------ -* All events defined in this repository must follow the same format. +* All events must follow the same format for their associated name. From 5dab0acbfe4e69699e8599b8e47ea932fde81d02 Mon Sep 17 00:00:00 2001 From: Felipe Montoya Date: Fri, 28 May 2021 16:37:49 -0500 Subject: [PATCH 3/5] feat: turning all current feedback into adrs --- .../0002-events-naming-and-versioning.rst | 91 +++++++++++++++++++ docs/decisions/0002-events-naming.rst | 74 --------------- docs/decisions/0003-events-payload.rst | 59 ++++++++++++ 3 files changed, 150 insertions(+), 74 deletions(-) create mode 100644 docs/decisions/0002-events-naming-and-versioning.rst delete mode 100644 docs/decisions/0002-events-naming.rst create mode 100644 docs/decisions/0003-events-payload.rst diff --git a/docs/decisions/0002-events-naming-and-versioning.rst b/docs/decisions/0002-events-naming-and-versioning.rst new file mode 100644 index 00000000..99068970 --- /dev/null +++ b/docs/decisions/0002-events-naming-and-versioning.rst @@ -0,0 +1,91 @@ +2. Open edX events naming and versioning +======================================== + +Status +------ + +Accepted + + +Context +------- + +Event type hooks are an important public promise. They are a strong foundation +of a healthy ecosystem of extensions for the Open edX world and have been +recognized as such by the arch team of the Open edX core and also by the community. + +This ADR has the purpose of defining the rules to be followed when naming an +Open edX Event with the intent of covering the use cases of: + +* Open edX core developers wanting to add new events. +* Open edX core developers wanting to deprecate and eventually remove events. +* Open edX extension developers wanting to create and maintain stable + applications, even when the events framework evolves and changes over time. +* Service developers wanting to listen for events using message queues. + +The decisions reached in this ADR are the product of open discussions in the ADR +PR history. + + +Decisions +--------- + +1. The name of an event will be a ``string`` that follows the `type format`_ +defined in the `OEP-41`_: + +``{Reverse DNS}.{Architecture Subdomain}.{Subject}.{Action}.{Major Version}`` + +Examples: + +* org.openedx.learning.course.enrollment.created.v1 +* org.openedx.learning.student.registration.completed.v2 +* org.openedx.learning.session.login.completed.v1 +* org.openedx.learning.session.login.completed.v1 + +2. The signal definitions will be placed written in code in the way that is more +practical and familiar for python developer with emphasis on Django experience. +Definition will be grouped by subdomain along with other data structures in a +way that favors reuse. +There will be a subclass of Django signal that accepts the event name and makes +available for listeners of the emitted events and the frameworks other event +processing queues. Only the Architecture Subdomain part of the event name will +be used in the package name of the signal definition. + +3. The events library will use SemVer 2. The major version will not be tied to +Open edX releases for the time being. We still recognize that Open edX releases +are the logical boundary to remove signals and therefore make breaking changes +such as remove signal definitions. + +4. The Major version of an event will be both part of the `event name` and also +be written to the variable defining the signal. However version 1 (V1) of an +event will remove the _V1 suffix for readability. We also expect to have +relatively few breaking changes to the signal definitions. The minor version of +a signal will be written in the payload as part of the header information +defined in `OEP-41`_. + +5. The events library will be a single library containing the signal definitions, +the simple data structures required and the necessary classes and tools to +support the events framework. The definitions of the signals will be written +with a logical boundary such that if the project ever decides to separate them +in a split library there is no necessary large refactor. + +.. _type format: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#id5 +.. _OEP-41: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#specification + + +Consequences +------------ + +1. There will not be a necessary correspondence between open edX release and +major versions of this library. Also there will not be a need to make a major +release if there is no breaking for consecutive Open edX releases. + +2. Open edX core and in particular edx-platform must emit the signals meant for +public consumption as they are written in this library, changes in edx-platform +that require changes in the public signal will require a backwards compatible +addition to this library or an altogether new signal with support for the old +signal until deprecated and removed. + +3. Changing the arguments passed to an event must always be done in a backwards +compatible way since making it incompatible warrants the use of a new major +version. diff --git a/docs/decisions/0002-events-naming.rst b/docs/decisions/0002-events-naming.rst deleted file mode 100644 index 34aa4c90..00000000 --- a/docs/decisions/0002-events-naming.rst +++ /dev/null @@ -1,74 +0,0 @@ -2. Open edX events naming -========================= - -Status ------- - -Draft - -Context -------- - -Besides a suitable location, event-type hooks need a form of -identification when adding them to the Open edX platform. By doing this, -recognizing which event to use, or debugging errors will be easier. - -This ADR has the purpose of defining the rules to be followed when -naming an Open edX Event. - -Decisions ---------- - -The following decisions were made after receiving feedback from the Open edX -community on this repository and on other platforms such as discuss. - -1. The name will be a ``string`` that follows the `type format`_ defined in -the `OEP-41`_ with a minor change: - -``{Reverse DNS}.{Architecture Subdomain}.{Subject}.{Action}`` - -Examples: - -* org.openedx.learning.course.enrollment.created -* org.openedx.learning.student.registration.completed -* org.openedx.learning.session.login.completed - -As can be seen, ``{Major Version}`` was removed from the name, meaning, -events won't be versioned. Instead, the data sent by them will be: - -.. code-block:: python - - providing_args={ - "user_data": UserData, - "registration_data": RegistrationData, - "registration_data_v2": RegistrationDataV2, - } - -2. The name will be sent as metadata for the events to -help with debugging, logging, or any other process related to events. - -For example: - -.. code-block:: python - - # Event definition - STUDENT_REGISTRATION_COMPLETED = OpenEdxPublicSignal( - event_type="org.openedx.learning.student.registration.completed", - providing_args={ - "user_data": UserData, - "registration_data": RegistrationData, - ... - } - ) - -3. The definition of the signal does not have a one-to-one mapping to the name -proposed here. The definition will be done in such a way that it's the most -usable and accessible for the developer. - -.. _type format: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#id5 -.. _OEP-41: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#specification - -Consequences ------------- - -* All events must follow the same format for their associated name. diff --git a/docs/decisions/0003-events-payload.rst b/docs/decisions/0003-events-payload.rst new file mode 100644 index 00000000..f003c9e0 --- /dev/null +++ b/docs/decisions/0003-events-payload.rst @@ -0,0 +1,59 @@ +3. Open edX events payload conventions +====================================== + +Status +------ + +Accepted + + +Context +------- + +Given their public promise status, event hooks have maintainability as the main +design goal. The contracts we are creating here should be stable enough to +support the growth of the extensions community. That said, things should be +allowed to evolve in a backwards compatible manner. When things inevitable break, +they should break in CI. Which should not require the code of edx-platform to +test integrations. + + +Decisions +--------- + +1. Events will receive header information in line with the `OEP-41 format`_. +However all the fields but `data`, which are also referred as the envelope +information, will be wrapped in a dictionary. The data specific to the event +will be passed as unpacked named arguments so that function signature mismatch +errors can be caught in CI. + +2. The envelope information will be calculated on the fly by the +`OpenEdxPublicSignal` class to keep this information out of the way when writing +the event calling location as well. + +3. The data sent to an event will always use the form of attr objects following +the `OEP-49 data pattern`_ . This data objects will be defined in the events +library and will be scoped to the Architecture Subdomain they are in. Data +objects should use only serializable python primitives and standard types with +the exception of OpaqueKeys. + + +.. _OEP-41 format: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#message-format +.. _OEP-49 data pattern: https://open-edx-proposals.readthedocs.io/en/latest/oep-0049-django-app-patterns.html#id9 + + +Consequences +------------ + +1. Extension developers will be able to test their event listeners without the +need to import any edx-platform code. + +2. Consequence of the versioning ADR together with this one, extension developers +will be able to test their code with different versions of the library and thus +guarantee that their code will not break when upgrading open releases. + +3. The events library will have a dependency on the OpaqueKeys library. + +4. Events defined by this library will not be drop-in replacement of current +edx-platform signals. This means some refactoring will be needed when converting +the platform code over to openedx_events. From 3868b80bbfc68c1689633266825a63b72b33713f Mon Sep 17 00:00:00 2001 From: Felipe Montoya Date: Fri, 4 Jun 2021 15:47:26 -0500 Subject: [PATCH 4/5] docs: added clarifications around message queues and subdomains --- docs/decisions/0002-events-naming-and-versioning.rst | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/decisions/0002-events-naming-and-versioning.rst b/docs/decisions/0002-events-naming-and-versioning.rst index 99068970..abe6d4b7 100644 --- a/docs/decisions/0002-events-naming-and-versioning.rst +++ b/docs/decisions/0002-events-naming-and-versioning.rst @@ -48,7 +48,7 @@ Definition will be grouped by subdomain along with other data structures in a way that favors reuse. There will be a subclass of Django signal that accepts the event name and makes available for listeners of the emitted events and the frameworks other event -processing queues. Only the Architecture Subdomain part of the event name will +processing queues. Only the `Architecture Subdomain`_ part of the event name will be used in the package name of the signal definition. 3. The events library will use SemVer 2. The major version will not be tied to @@ -70,6 +70,7 @@ with a logical boundary such that if the project ever decides to separate them in a split library there is no necessary large refactor. .. _type format: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#id5 +.. _Architecture Subdomain: https://openedx.atlassian.net/wiki/spaces/AC/pages/663224968/edX+DDD+Bounded+Contexts .. _OEP-41: https://open-edx-proposals.readthedocs.io/en/latest/oep-0041-arch-async-server-event-messaging.html#specification @@ -89,3 +90,10 @@ signal until deprecated and removed. 3. Changing the arguments passed to an event must always be done in a backwards compatible way since making it incompatible warrants the use of a new major version. + +4. Since it was decided from the OEP-50 that Django Signals is the underlying +mechanism for messages, service developers wanting to listen for events using +message queues will still require a way to transform the signal-based events +into the message queue technology. This part of the solution is not covered by +this library + From 2981d75a3af63bef3bd8075e60db43adc841e8e2 Mon Sep 17 00:00:00 2001 From: Felipe Montoya Date: Thu, 10 Jun 2021 18:44:48 -0500 Subject: [PATCH 5/5] docs: final round clarifying future of message queues --- docs/decisions/0002-events-naming-and-versioning.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/decisions/0002-events-naming-and-versioning.rst b/docs/decisions/0002-events-naming-and-versioning.rst index abe6d4b7..2a3d69a3 100644 --- a/docs/decisions/0002-events-naming-and-versioning.rst +++ b/docs/decisions/0002-events-naming-and-versioning.rst @@ -40,7 +40,6 @@ Examples: * org.openedx.learning.course.enrollment.created.v1 * org.openedx.learning.student.registration.completed.v2 * org.openedx.learning.session.login.completed.v1 -* org.openedx.learning.session.login.completed.v1 2. The signal definitions will be placed written in code in the way that is more practical and familiar for python developer with emphasis on Django experience. @@ -95,5 +94,6 @@ version. mechanism for messages, service developers wanting to listen for events using message queues will still require a way to transform the signal-based events into the message queue technology. This part of the solution is not covered by -this library - +this library at this time. In the future we may consider adding it here or setup +a separate library for those events. We leave that decision open for now because +many details about the implementation are still unclear.