feat: use event producer config #33269

rgraber · 2023-09-15T17:44:17Z

Left as draft because this should not be merged until Arch BOM is ready to test on stage.

Original PR description from @navinkarkera. This PR has all the changes of his PR here but events are disabled by default and the latest release of the openedx-events library is used. Got into some git troubles, hence the new PR.

Description

Replace openedx_event signal handlers with new producer config to push to event bus.

Supporting information

Depends on feat: enable producing to event bus via settings openedx-events#249

Testing instructions

Setup master devstack.
Get lms and cms up using make {lms,cms}-up
Go to cms shell using make cms-shell
Run pip install -r requirements/edx/development.txt to install new openedx_events version.
Exit from cms-shell and see cms logs using make cms-logs.
Go to studio and delete an xblock or publish a section/subsection.

This should push the corresponding event to event bus and print below message:

edx.devstack.cms  | 2023-07-28 13:43:57,201 INFO 515 [edx_event_bus_redis.internal.producer] [user 3] [ip 172.18.0.1] producer.py:82 - Message delivered to Redis event bus: topic=dev-content-authoring-xblock-lifecycle, message_id=cccfef82-2d4c-11ee-9c7a-0242ac12000c, signal=<OpenEdxPublicSignal: org.openedx.content_authoring.xblock.published.v1>, redis_msg_id=b'1690551837201-0'

Play with the EVENT_BUS_PRODUCER_CONFIG by toggling the flag and triggering the event.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Replace openedx_event signal handlers with new producer config to push to event bus

robrap

Thank you.

robrap · 2023-09-15T19:36:29Z

cms/envs/devstack.py

-#   in the LMS and CMS.
-# .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/31813'
-FEATURES['ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS'] = True
-FEATURES['ENABLE_SEND_ENROLLMENT_EVENTS_OVER_BUS'] = True


What about other references to this? https://github.com/search?q=repo%3Aopenedx%2Fedx-platform%20ENABLE_SEND_ENROLLMENT_EVENTS_OVER_BUS&type=code ?

Can you search for all four of these settings and ensure you got all occurrences in the repo?

robrap · 2023-09-15T19:41:46Z

lms/djangoapps/certificates/signals.py

-    """
-    Publish `CERTIFICATE_CREATED` events to the event bus.
-    """
-    if SEND_CERTIFICATE_CREATED_SIGNAL.is_enabled():


Maybe you could share the private PR for edx-internal settings so we can cross-reference that the old settings are being replaced by the new, and that they match values (enabled vs disabled)?

Share it where? I don't think it makes sense to link it on a public PR.

If you don't want to add private links here, maybe we just add more details to the deployment plan document?

robrap · 2023-09-15T19:45:15Z

lms/djangoapps/certificates/tests/test_signals.py

@@ -440,78 +433,3 @@ def test_verified_to_audit(self):
        ) as mock_allowlist_task:
            self.verified_enrollment.change_mode('audit')
            mock_allowlist_task.assert_not_called()
-
-
-class CertificateEventBusTests(ModuleStoreTestCase):


[non-blocking question] I can't think of anything, but I'm wondering how we might ensure that people would know that a signal is in use by the event bus, and that it can't just be deleted or refactored? Maybe I should create a separate issue for this?

robrap · 2023-09-15T19:47:13Z

lms/envs/common.py

+# .. setting_default: {}
+# .. setting_description: Dictionary of event_types mapped to lists of dictionaries containing topic related configuration.
+#    Each topic configuration dictionary contains
+#    * a topic/stream name called `topic` where the event will be pushed to.


Nit: I think * topic: a topic/stream ... would be a format that is quicker to read.

robrap · 2023-09-15T19:49:38Z

lms/envs/common.py

+# .. setting_description: Dictionary of event_types mapped to lists of dictionaries containing topic related configuration.
+#    Each topic configuration dictionary contains
+#    * a topic/stream name called `topic` where the event will be pushed to.
+#    * a flag called `enabled` denoting whether the event will be published to the topic.


Can we use toggle instead of flag?

Can we mention that each toggle should be annotated following: https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html?

robrap · 2023-09-15T19:50:14Z

lms/envs/common.py

+#    Note: The topic names should not include environment prefix as it will be dynamically added based on
+#    EVENT_BUS_TOPIC_PREFIX setting.
+EVENT_BUS_PRODUCER_CONFIG = {
+    'org.openedx.learning.certificate.created.v1': [


Can we add toggle annotations for each of these following: https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html?

The best I can think of is "temporary" but we also don't have a removal date, which is required. opt_in, maybe? or open_edx?

it seems dependent on openedx/openedx-events#265

Good questions.

Because the toggle is a permanent feature of the config, let's not call it temporary.

I think opt_in works fine. Note that these options should be going away anyway.

We have a strange situation where we will think we might be changing the default after some roll-out period, and this is dependent on the ticket you linked. Maybe just have some comment around this at the top of the config, noting that we need to work this out?

Looking more I think we may still want to keep 'ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS' since it actually gates several events which presumably should be launched or held back as a group. We can use the value of it to set the value in the config map. What do you think?

Beginning to wonder if the entire map should be derived, with different settings for each event, at least in edx-platform. It's not a perfect solution because of the proliferation of settings, but it I think it's better than having some of them be set in the map and some derived from elsewhere. Less risk of someone trying to only enable or disable one event and not realizing they need to recreate the entire rest of the map in settings.

If someone wants to add or change a topic it will still require setting the entire map, which is frustrating. But I think a better-than-before approach would just be to have enabled/disabled extracted as a different setting for each event and then derive the map from that.

Are you concerned with a single topic, or the entire setting? We could also use KEYS_WITH_MERGED_VALUES.

The entire setting. If they wanted to change a single topic, they'd have to copy over the entire config dict and set all the derived settings correctly, which is non obvious. Keys with merged values looks like it may be our friend here.

With keys with merged values, do you feel more comfortable using derived settings on a per-topic basis? Feel free to answer with actual code changes for where you are landing.

rgraber · 2023-10-16T19:43:16Z

replaced by #33458

navinkarkera and others added 4 commits July 28, 2023 15:59

feat: use event producer config

b46d149

Replace openedx_event signal handlers with new producer config to push to event bus

fixup!: disable events by default

b0f157f

Merge branch 'master' into rsgraber/use-producer-config-disable

e92bd61

fixup!: actually bump version

8f6b642

rgraber mentioned this pull request Sep 15, 2023

feat: use event producer config #32874

Closed

rgraber marked this pull request as ready for review September 15, 2023 18:36

rgraber marked this pull request as draft September 15, 2023 18:40

rgraber changed the title ~~feat: start with events disabled~~ feat: use event producer config Sep 15, 2023

robrap reviewed Sep 15, 2023

View reviewed changes

Rebecca Graber and others added 5 commits September 18, 2023 10:15

fixup!: rm one more handler

9c0cf67

fixup!: annotate better

2f96f8a

fixup!: rm old handler tests

982b224

fixup!: delete more

bf8224f

Merge branch 'master' into rsgraber/use-producer-config-disable

19363a4

rgraber mentioned this pull request Sep 28, 2023

feat!: new event bus config format openedx/openedx-events#272

Merged

11 tasks

navinkarkera mentioned this pull request Oct 2, 2023

feat: replace signal handler with event producer config openedx/xblock-skill-tagging#14

Open

10 tasks

rgraber mentioned this pull request Oct 11, 2023

feat: use new event_bus_producer_config #33458

Merged

rgraber closed this Oct 16, 2023

nedbat deleted the rsgraber/use-producer-config-disable branch January 8, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use event producer config #33269

feat: use event producer config #33269

rgraber commented Sep 15, 2023 •

edited

Loading

robrap left a comment

robrap Sep 15, 2023

robrap Sep 15, 2023

rgraber Sep 18, 2023

robrap Oct 5, 2023

robrap Sep 15, 2023

robrap Sep 15, 2023

robrap Sep 15, 2023

robrap Sep 15, 2023

rgraber Sep 18, 2023

rgraber Sep 18, 2023

robrap Sep 18, 2023

rgraber Sep 18, 2023

rgraber Sep 19, 2023

rgraber Sep 19, 2023

robrap Sep 19, 2023

rgraber Sep 19, 2023

robrap Sep 19, 2023

rgraber commented Oct 16, 2023

feat: use event producer config #33269

feat: use event producer config #33269

Conversation

rgraber commented Sep 15, 2023 • edited Loading

Description

Supporting information

Testing instructions

Deadline

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber commented Oct 16, 2023

rgraber commented Sep 15, 2023 •

edited

Loading