Skip to content
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.

Sign up for GitHub

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

feat: use event producer config #33269

Closed
wants to merge 9 commits into from
Closed

feat: use event producer config #33269

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b46d149
feat: use event producer config
navinkarkera Jul 28, 2023
b0f157f
fixup!: disable events by default
Sep 15, 2023
e92bd61
Merge branch 'master' into rsgraber/use-producer-config-disable
Sep 15, 2023
8f6b642
fixup!: actually bump version
Sep 15, 2023
9c0cf67
fixup!: rm one more handler
Sep 18, 2023
2f96f8a
fixup!: annotate better
Sep 18, 2023
982b224
fixup!: rm old handler tests
Sep 18, 2023
bf8224f
fixup!: delete more
Sep 18, 2023
19363a4
Merge branch 'master' into rsgraber/use-producer-config-disable
Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 1 addition & 61 deletions cms/djangoapps/contentstore/signals/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,7 @@
from edx_toggles.toggles import SettingToggle
from opaque_keys.edx.keys import CourseKey
from openedx_events.content_authoring.data import CourseCatalogData, CourseScheduleData
from openedx_events.content_authoring.signals import (
COURSE_CATALOG_INFO_CHANGED,
XBLOCK_DELETED,
XBLOCK_DUPLICATED,
XBLOCK_PUBLISHED,
)
from openedx_events.event_bus import get_producer
from openedx_events.content_authoring.signals import COURSE_CATALOG_INFO_CHANGED
from pytz import UTC

from cms.djangoapps.contentstore.courseware_index import (
Expand Down Expand Up @@ -159,60 +153,6 @@ def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=
transaction.on_commit(lambda: emit_catalog_info_changed_signal(course_key))


@receiver(COURSE_CATALOG_INFO_CHANGED)
def listen_for_course_catalog_info_changed(sender, signal, **kwargs):
"""
Publish COURSE_CATALOG_INFO_CHANGED signals onto the event bus.
"""
get_producer().send(
signal=COURSE_CATALOG_INFO_CHANGED, topic='course-catalog-info-changed',
event_key_field='catalog_info.course_key', event_data={'catalog_info': kwargs['catalog_info']},
event_metadata=kwargs['metadata'],
)


@receiver(XBLOCK_PUBLISHED)
def listen_for_xblock_published(sender, signal, **kwargs):
"""
Publish XBLOCK_PUBLISHED signals onto the event bus.
"""
if settings.FEATURES.get("ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS"):
topic = getattr(settings, "EVENT_BUS_XBLOCK_LIFECYCLE_TOPIC", "course-authoring-xblock-lifecycle")
get_producer().send(
signal=XBLOCK_PUBLISHED, topic=topic,
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)


@receiver(XBLOCK_DELETED)
def listen_for_xblock_deleted(sender, signal, **kwargs):
"""
Publish XBLOCK_DELETED signals onto the event bus.
"""
if settings.FEATURES.get("ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS"):
topic = getattr(settings, "EVENT_BUS_XBLOCK_LIFECYCLE_TOPIC", "course-authoring-xblock-lifecycle")
get_producer().send(
signal=XBLOCK_DELETED, topic=topic,
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)


@receiver(XBLOCK_DUPLICATED)
def listen_for_xblock_duplicated(sender, signal, **kwargs):
"""
Publish XBLOCK_DUPLICATED signals onto the event bus.
"""
if settings.FEATURES.get("ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS"):
topic = getattr(settings, "EVENT_BUS_XBLOCK_LIFECYCLE_TOPIC", "course-authoring-xblock-lifecycle")
get_producer().send(
signal=XBLOCK_DUPLICATED, topic=topic,
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)


@receiver(SignalHandler.course_deleted)
def listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=unused-argument
"""
Expand Down
28 changes: 28 additions & 0 deletions cms/envs/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,6 +1791,9 @@

# Blockstore
'blockstore.apps.bundles',

# Openedx events
'openedx_events',
]


Expand Down Expand Up @@ -2768,6 +2771,31 @@
DISCUSSIONS_INCONTEXT_FEEDBACK_URL = ''
DISCUSSIONS_INCONTEXT_LEARNMORE_URL = ''

######################## Event bus producer config ########################

# .. setting_name: EVENT_BUS_PRODUCER_CONFIG
# .. 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.
# * a flag called `enabled` denoting whether the event will be published to the topic.
# * `event_key_field` which is a period-delimited string path to event data field to use as event key.
# 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.content_authoring.course.catalog_info.changed.v1': [
{'topic': 'course-catalog-info-changed', 'event_key_field': 'catalog_info.course_key', 'enabled': False},
],
'org.openedx.content_authoring.xblock.published.v1': [
{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': False},
],
'org.openedx.content_authoring.xblock.deleted.v1': [
{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': False},
],
'org.openedx.content_authoring.xblock.duplicated.v1': [
{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': False},
],
}
#### django-simple-history##
# disable indexing on date field its coming django-simple-history.
SIMPLE_HISTORY_DATE_INDEX = False
13 changes: 0 additions & 13 deletions cms/envs/devstack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,23 +301,10 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing
CREDENTIALS_PUBLIC_SERVICE_URL = 'http://localhost:18150'

#################### Event bus backend ########################
# .. toggle_name: FEATURES['ENABLE_SEND_XBLOCK_EVENTS_OVER_BUS']
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: Temporary configuration which enables sending xblock events over the event bus.
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: 2023-02-21
# .. toggle_warning: For consistency in user experience, keep the value in sync with the setting of the same name
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What about other references to this? https://github.com/search?q=repo%3Aopenedx%2Fedx-platform%20ENABLE_SEND_ENROLLMENT_EVENTS_OVER_BUS&type=code ?
  2. Can you search for all four of these settings and ensure you got all occurrences in the repo?

EVENT_BUS_PRODUCER = 'edx_event_bus_redis.create_producer'
EVENT_BUS_REDIS_CONNECTION_URL = 'redis://:[email protected]:6379/'
EVENT_BUS_TOPIC_PREFIX = 'dev'
EVENT_BUS_CONSUMER = 'edx_event_bus_redis.RedisEventConsumer'
EVENT_BUS_XBLOCK_LIFECYCLE_TOPIC = 'course-authoring-xblock-lifecycle'
EVENT_BUS_ENROLLMENT_LIFECYCLE_TOPIC = 'course-authoring-enrollment-lifecycle'

################# New settings must go ABOVE this line #################
########################################################################
Expand Down
18 changes: 0 additions & 18 deletions lms/djangoapps/certificates/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@

from django.db.models.signals import post_save
from django.dispatch import receiver
from openedx_events.event_bus import get_producer

from common.djangoapps.course_modes import api as modes_api
from common.djangoapps.student.models import CourseEnrollment
from common.djangoapps.student.signals import ENROLLMENT_TRACK_UPDATED
from lms.djangoapps.certificates.config import SEND_CERTIFICATE_CREATED_SIGNAL
from lms.djangoapps.certificates.generation_handler import (
CertificateGenerationNotAllowed,
generate_allowlist_certificate_task,
Expand All @@ -32,7 +30,6 @@
COURSE_GRADE_NOW_PASSED,
LEARNER_NOW_VERIFIED
)
from openedx_events.learning.signals import CERTIFICATE_CREATED

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -159,18 +156,3 @@ def _listen_for_enrollment_mode_change(sender, user, course_key, mode, **kwargs)
course_key,
)
return False


@receiver(CERTIFICATE_CREATED)
def listen_for_certificate_created_event(sender, signal, **kwargs):
"""
Publish `CERTIFICATE_CREATED` events to the event bus.
"""
if SEND_CERTIFICATE_CREATED_SIGNAL.is_enabled():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

get_producer().send(
signal=CERTIFICATE_CREATED,
topic='learning-certificate-lifecycle',
event_key_field='certificate.course.course_key',
event_data={'certificate': kwargs['certificate']},
event_metadata=kwargs['metadata']
)
82 changes: 0 additions & 82 deletions lms/djangoapps/certificates/tests/test_signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@
and disabling for instructor-paced courses.
"""

from datetime import datetime, timezone
from unittest import mock
from uuid import uuid4

import ddt
from django.test.utils import override_settings
from edx_toggles.toggles.testutils import override_waffle_flag, override_waffle_switch
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
Expand All @@ -21,14 +18,10 @@
CertificateGenerationConfiguration,
GeneratedCertificate
)
from lms.djangoapps.certificates.signals import listen_for_certificate_created_event
from lms.djangoapps.certificates.tests.factories import CertificateAllowlistFactory, GeneratedCertificateFactory
from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory
from lms.djangoapps.grades.tests.utils import mock_passing_grade
from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification
from openedx_events.data import EventsMetadata
from openedx_events.learning.signals import CERTIFICATE_CREATED
from openedx_events.learning.data import CourseData, UserData, UserPersonalData, CertificateData


class SelfGeneratedCertsSignalTest(ModuleStoreTestCase):
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

"""
Tests for Certificate events that interact with the event bus.
"""
def setUp(self):
super().setUp()
self.user = UserFactory.create()
self.name = f'{self.user.first_name} {self.user.last_name}'
self.course = CourseFactory.create(self_paced=True)
self.enrollment = CourseEnrollmentFactory(
user=self.user,
course_id=self.course.id,
is_active=True,
mode='verified',
)

@override_settings(SEND_CERTIFICATE_CREATED_SIGNAL=False)
@mock.patch('lms.djangoapps.certificates.signals.get_producer', autospec=True)
def test_event_disabled(self, mock_producer):
"""
Test to verify that we do not push `CERTIFICATE_CREATED` events to the event bus if the
`SEND_CERTIFICATE_CREATED_SIGNAL` setting is disabled.
"""
listen_for_certificate_created_event(None, CERTIFICATE_CREATED)
mock_producer.assert_not_called()

@override_settings(SEND_CERTIFICATE_CREATED_SIGNAL=True)
@mock.patch('lms.djangoapps.certificates.signals.get_producer', autospec=True)
def test_event_enabled(self, mock_producer):
"""
Test to verify that we push `CERTIFICATE_CREATED` events to the event bus if the
`SEND_CERTIFICATE_CREATED_SIGNAL` setting is enabled.
"""
expected_course_data = CourseData(course_key=self.course.id)
expected_user_data = UserData(
pii=UserPersonalData(
username=self.user.username,
email=self.user.email,
name=self.name,
),
id=self.user.id,
is_active=self.user.is_active
)
expected_certificate_data = CertificateData(
user=expected_user_data,
course=expected_course_data,
mode='verified',
grade='',
current_status='downloadable',
download_url='',
name='',
)
event_metadata = EventsMetadata(
event_type=CERTIFICATE_CREATED.event_type,
id=uuid4(),
minorversion=0,
source='openedx/lms/web',
sourcehost='lms.test',
time=datetime.now(timezone.utc)
)

event_kwargs = {
'certificate': expected_certificate_data,
'metadata': event_metadata
}

listen_for_certificate_created_event(None, CERTIFICATE_CREATED, **event_kwargs)
# verify that the data sent to the event bus matches what we expect
data = mock_producer.return_value.send.call_args.kwargs
assert data['signal'].event_type == CERTIFICATE_CREATED.event_type
assert data['event_data']['certificate'] == expected_certificate_data
assert data['topic'] == 'learning-certificate-lifecycle'
assert data['event_key_field'] == 'certificate.course.course_key'
23 changes: 23 additions & 0 deletions lms/envs/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3304,6 +3304,9 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring

# Notifications
'openedx.core.djangoapps.notifications',

# Openedx events
'openedx_events',
]

######################### CSRF #########################################
Expand Down Expand Up @@ -5379,6 +5382,26 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
NOTIFICATIONS_EXPIRY = 60
EXPIRED_NOTIFICATIONS_DELETE_BATCH_SIZE = 10000

######################## Event bus producer config ########################

# .. setting_name: EVENT_BUS_PRODUCER_CONFIG
# .. 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# * a flag called `enabled` denoting whether the event will be published to the topic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can we use toggle instead of flag?
  2. 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?

# * `event_key_field` which is a period-delimited string path to event data field to use as event key.
# 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': [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems dependent on openedx/openedx-events#265

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions.

  1. Because the toggle is a permanent feature of the config, let's not call it temporary.
  2. I think opt_in works fine. Note that these options should be going away anyway.
  3. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{'topic': 'learning-certificate-lifecycle', 'event_key_field': 'certificate.course.course_key',
'enabled': False},
],
'org.openedx.learning.xblock.skill.verified.v1': [
{'topic': 'learning-xblock-skill-verified', 'event_key_field': 'xblock_info.usage_key', 'enabled': False},
]
}
#### django-simple-history##
# disable indexing on date field its coming from django-simple-history.
SIMPLE_HISTORY_DATE_INDEX = False
3 changes: 0 additions & 3 deletions requirements/constraints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,3 @@ libsass==0.10.0

# greater version breaking upgrade builds
click==8.1.6

# openedx-events 8.6.0 introduces publishing via configuration. Ticket to unpin: https://github.com/edx/edx-arch-experiments/issues/381
openedx-events<8.6.0 # Open edX Events from Hooks Extension Framework (OEP-50)
3 changes: 1 addition & 2 deletions requirements/edx/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,9 +767,8 @@ openedx-django-require==2.1.0
# via -r requirements/edx/kernel.in
openedx-django-wiki==2.0.1
# via -r requirements/edx/kernel.in
openedx-events==8.5.0
openedx-events==8.6.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/kernel.in
# edx-event-bus-kafka
# edx-event-bus-redis
Expand Down
3 changes: 1 addition & 2 deletions requirements/edx/development.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1298,9 +1298,8 @@ openedx-django-wiki==2.0.1
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
openedx-events==8.5.0
openedx-events==8.6.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
# edx-event-bus-kafka
Expand Down
3 changes: 1 addition & 2 deletions requirements/edx/doc.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,9 +908,8 @@ openedx-django-require==2.1.0
# via -r requirements/edx/base.txt
openedx-django-wiki==2.0.1
# via -r requirements/edx/base.txt
openedx-events==8.5.0
openedx-events==8.6.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
# edx-event-bus-kafka
# edx-event-bus-redis
Expand Down
3 changes: 1 addition & 2 deletions requirements/edx/testing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,9 +978,8 @@ openedx-django-require==2.1.0
# via -r requirements/edx/base.txt
openedx-django-wiki==2.0.1
# via -r requirements/edx/base.txt
openedx-events==8.5.0
openedx-events==8.6.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
# edx-event-bus-kafka
# edx-event-bus-redis
Expand Down
Loading