docs: [FC-0074] add how-to add event bus support to an Open edX Event #428
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mariajgrimaldi! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mariajgrimaldi
changed the title
mariajgrimaldi
force-pushed
the
MJG/event-bus-support-docs
branch
from
9cb3fee to
66e760b
Compare
mariajgrimaldi
force-pushed
the
MJG/event-bus-support-docs
branch
from
9741f2e to
0e21812
Compare
| When adding support is not possible do the following: | ||
|
|
||
| - Add it to the ``KNOWN_UNSERIALIZABLE_SIGNALS`` list in the ``openedx_events/tooling.py`` file so the event bus ignores it. | ||
| - Add a ``warning`` in the event's docstring to inform developers that the event is not compatible with the event bus. |
There was a problem hiding this comment.
It would be good to document why as well
There was a problem hiding this comment.
Agreed! Thank you. d8deec5
|
|
||
| An Open edX Event is compatible with the event bus when its payload can be serialized, sent, and deserialized by other services. The payload, structured as `attrs data classes`_, must align with the event bus schema format which in this case is the :term:`Avro Schema`. This schema is used to serialize and deserialize the :term:`Event Payload` when sending it across services. | ||
|
|
||
| This ensures the event can be sent by the producer and be then re-emitted by the same instance of `OpenEdxPublicSignal`_ on the consumer side. For more information on the event bus schema format, refer to the :doc:`../decisions/0004-external-event-bus-and-django-signal-events` and :doc:`../decisions/0005-external-event-schema-format` decision records. |
There was a problem hiding this comment.
It might be useful to call out that the serialization should ensure that the data not only emits, but is identical. Serializing this way should prevent things like timezone issues on timestamps and precision problems with floating point numbers.
There was a problem hiding this comment.
That'd be a good addition! I added a similar note to the paragraph: 51683fd#diff-67886caf4b3357c606ceb6d3ea25e3839b6056f13b22d48a146431adc0fa829dR31
|
|
||
| This ensures the event can be sent by the producer and be then re-emitted by the same instance of `OpenEdxPublicSignal`_ on the consumer side. For more information on the event bus schema format, refer to the :doc:`../decisions/0004-external-event-bus-and-django-signal-events` and :doc:`../decisions/0005-external-event-schema-format` decision records. | ||
|
|
||
| Here is an example of an :term:`Event Payload` structured as `attrs data classes`_ that align with the event bus schema format: |
There was a problem hiding this comment.
You may want to simplify this example, there is a lot here. If you are trying to show specific functionality like primitive types vs custom serialized types it may be clearer to create fake classes to demonstrate that.
There was a problem hiding this comment.
I dropped the example altogether and added a reference to where to find examples in the repo: 51683fd#diff-67886caf4b3357c606ceb6d3ea25e3839b6056f13b22d48a146431adc0fa829dR53
|
|
||
| username = attr.ib(type=str) | ||
| email = attr.ib(type=str) | ||
| name = attr.ib(type=str, factory=str) |
There was a problem hiding this comment.
Not sure if factory here is confusing matters or something you want to describe
There was a problem hiding this comment.
I dropped the example here: 51683fd#diff-67886caf4b3357c606ceb6d3ea25e3839b6056f13b22d48a146431adc0fa829dR53
|
|
||
| Before sending the event across services, you need to ensure that the :term:`Event Payload` can be serialized and deserialized correctly. The event bus concrete implementations use the :term:`Avro Schema` to serialize and deserialize the :term:`Event Payload` as mentioned in the :doc:`../decisions/0005-external-event-schema-format` decision record. The concrete implementation of the event bus handles the serialization and deserialization with the help of methods implemented by this library. | ||
|
|
||
| .. For example, here's how the Redis event bus handles serialization before sending a message: |
There was a problem hiding this comment.
Looks like your formatting got messed up here
There was a problem hiding this comment.
I commented this section because it was too specific and unrelated to adding event bus support for an event. What do you think?
| .. warning:: | ||
| One of the known limitations of the current Open edX Event Bus is that it does not support dictionaries as data types. If the :term:`Event Payload` contains dictionaries, you may need to refactor the :term:`Event Payload` to use supported data types. When you know the structure of the dictionary, you can create an attrs class that represents the dictionary structure. If not, you can use a str type to represent the dictionary as a string and deserialize it on the consumer side using JSON deserialization. | ||
|
|
||
| If your :term:`Event Payload` contains only supported data types, you can skip this step. |
There was a problem hiding this comment.
This might make more sense if you moved this into the Step 4 section.
There was a problem hiding this comment.
The skipping part is slightly wrong. I meant "if you're using only supported data types, then you can skip implementing a custom serializer". But that sounds a bit redundant, considering L82. I dropped the line altogether: 901e495
I'm not sure why step 4 would be optional. Can you explain more about this?
| Step 4: Generate the Avro Schema | ||
| -------------------------------- | ||
|
|
||
| As mentioned in the previous step, the serialization and deserialization of the :term:`Event Payload` is handled by the concrete event bus implementation with the help of methods implemented in this library. However, although openedx-events does not handles the serialization and deserialization of the :term:`Event Payload` directly, it ensures the payload of new events can be serialized and deserialized correctly by adding checks in the CI/CD pipeline for schema verification. To ensure this, you need to generate the Avro schema for the :term:`Event Payload`: |
There was a problem hiding this comment.
nit: "openedx-events does not handles the" -> "openedx-events does not handle the"
Maybe: "To ensure this, you need to generate the Avro schema" -> "To ensure tests pass, you need to generate an Avro test schema for your new event's :term:Event Payload:"
There was a problem hiding this comment.
Thank you for the suggestion! fbf38cf
|
|
||
| .. code-block:: bash | ||
|
|
||
| python manage.py generate_avro_schemas org.openedx.learning.course.enrollment.changed.v1 |
There was a problem hiding this comment.
Maybe call out instead of using this specific event?
| Step 5: Send the Event Across Services with the Event Bus | ||
| --------------------------------------------------------- | ||
|
|
||
| To validate that you can consume the event emitted by a service through the event bus, you can send the event across services. Here is an example of how you can send the event across services using the Redis event bus implementation following the `setup instructions in a Tutor environment`_. |
There was a problem hiding this comment.
Maybe a note here that any new custom serialized types need to be merged and deployed on both the producer and consumer side before doing this?
There was a problem hiding this comment.
Done: 34a26ff. Thanks for the suggestion!
mariajgrimaldi
force-pushed
the
MJG/event-bus-concepts
branch
from
5777568 to
dc5d29e
Compare
mariajgrimaldi
force-pushed
the
MJG/event-bus-support-docs
branch
3 times, most recently
from
a4205ad to
51683fd
Compare
| Step 1: Does my Event Need Event Bus Support? | ||
| ---------------------------------------------- | ||
|
|
||
| By default, Open edX Events should be compatible with the Open edX Event Bus. However, there are cases when the support might not be possible or needed for a particular event. Here are some scenarios where you might not need to add event bus support: | ||
|
|
||
| - The event is only used within the same application process and cannot be scoped to other services. | ||
| - The :term:`Event Payload` contains data types that are not supported by the event bus, and it is not possible to refactor the :term:`Event Payload` to use supported data types. | ||
|
|
||
| When adding support is not possible do the following: | ||
|
|
||
| - Add it to the ``KNOWN_UNSERIALIZABLE_SIGNALS`` list in the ``openedx_events/tooling.py`` file so the event bus ignores it. | ||
| - Add a ``warning`` in the event's docstring to inform developers that the event is not compatible with the event bus and why. | ||
|
|
||
| If you don't add the event to the ``KNOWN_UNSERIALIZABLE_SIGNALS`` list, the CI/CD pipeline will fail for the missing Avro schema that could not be generated for the :term:`Event Payload`. If you don't add a warning in the event's docstring, developers might try to send the event across services and encounter issues. |
There was a problem hiding this comment.
@bmtcril: At first we thought that all events in the repo should have event bus support by default. So I was going to add support for these events: https://github.com/openedx/openedx-events/blob/main/openedx_events/tooling.py#L20-L32. However, I realized that we would also need to add support for dictionaries (typed and more complex) and/or a rewrite of the data classes, which requires a lot more effort than what we gain with the support since, as far as I understand, most of those events are locally scoped.
Do you think the question, "Does my Event Need Event Bus Support?" is relevant considering what I mentioned, and that should we study each event before compromising on event bus support?
There was a problem hiding this comment.
I'm not sure I follow when you say locally scoped.
Some of the discussion thread events are very likely to be interesting by consumers of the event bus trying to make the discussion experience more reactive.
How much effort could it take to refactor those classes or to implement serialization capabilities for a list/dict with a limited capability of nesting. E.g: lists of primitives currently supported by avro.
Another option would be to say that we send most of the envelope of those discussion events and we keep the content of the discussion out of the serialization. The same we do with other objects such as a django user, we pass name, email and ID and we leave the consumer figure out the rest via API calls or such.
There was a problem hiding this comment.
@felipemontoya: thanks for the reply!
I'm not sure I follow when you say locally scoped.
I meant for interest only within the service where it's sent. But as you said, you can argue that all events can be of interest to consumers. Do you think this section "Does my Event Need Event Bus Support?" is still relevant?
How much effort could it take to refactor those classes or to implement serialization capabilities for a list/dict with a limited capability of nesting. E.g: lists of primitives currently supported by avro.
I'm more concerned about doing it properly. We already have "support" for those limited nesting capabilities, i.e., using attrs classes for fixed dicts or JSON structs as strings when we don't know the content of the dicts beforehand. I already added a note here with the suggestion: https://github.com/openedx/openedx-events/pull/428/files#diff-67886caf4b3357c606ceb6d3ea25e3839b6056f13b22d48a146431adc0fa829dR120. So I don't think adding support for dicts is strictly necessary. I'm sorry I didn't mention this in my previous comment, so it read only that it was too much effort to add support.
But then we have dicts of lists or lists of data attrs which my guess is that is more difficult to serialize, or maybe we hadn't had a strong use case for it, and that's why it's not supported.
That's where I wondered, should an event always have support for the event bus or can we be flexible with that requirement? I totally understand why we wouldn't want the separation between events with/without event bus support, but that's where we currently stand.
Another option would be to say that we send most of the envelope of those discussion events and we keep the content of the discussion out of the serialization.
I was thinking we could create new event versions without the complex serializable sections and considering what I said about fixed/dynamic dicts, so we can rewrite the data and make it suitable for the event bus, but leave the previous versions to be sent within the service. However, that would require sending/maintaining two versions of the event.
There was a problem hiding this comment.
As a result of this conversation, I started testing two approaches to give basic event bus support to data classes with dictionaries (str keys, only primitive types as values).
- Send dicts as JSON structs (str): feat: add event bus support to forum events V2 #434
- Add avro map support to dicts (based on this PR Cristhian opened a few months ago): feat: add support for annotated python dicts as avro map type #433
I was able to generate schemas for both approaches and avro tests seem to be passing, but I haven't tested them with an event bus implementation just yet. Let me know what you think!
There was a problem hiding this comment.
I managed to send forum events through the event bus by using this implementation: #433
| By default, Open edX Events should be compatible with the Open edX Event Bus. However, there are cases when the support might not be possible or needed for a particular event. Here are some scenarios where you might not need to add event bus support: | ||
|
|
||
| - The event is only used within the same application process and cannot be scoped to other services. | ||
| - The :term:`Event Payload` contains data types that are not supported by the event bus, and it is not possible to refactor the :term:`Event Payload` to use supported data types. |
There was a problem hiding this comment.
| - The :term:`Event Payload` contains data types that are not supported by the event bus, and it is not possible to refactor the :term:`Event Payload` to use supported data types. | |
| - The :term:`Event Payload` contains data types that are not supported by the event bus (such as ...), and it is not possible to refactor the :term:`Event Payload` to use supported data types. |
mariajgrimaldi
force-pushed
the
MJG/event-bus-support-docs
branch
from
6ec9ed2 to
bfc647c
Compare
@Ian2012: thanks for the review! Do you mean something like this? I'll be working on improving that document in the next couple of days as well. |
Description
This PR adds a how-to guide for implementing an event with event bus support. It details the requirements for sending and receiving events through the event bus and how to ensure compatibility.