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.

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!: new event bus config format #272

Merged
merged 24 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
67 changes: 67 additions & 0 deletions docs/decisions/0013-new-event-bus-producer-config.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
13. Change event producer config settings
#########################################

Status
******

**Accepted**

Context
*******

In a previous ADR, we set the structure for the event bus publishing configuration to a dictionary like the following:

.. code-block:: python

{ 'org.openedx.content_authoring.xblock.published.v1': [
{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': True},
{'topic': 'content-authoring-xblock-published', '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': True},
],
}

While attempting to implement this for edx-platform, we came across some problems with using this structure. In particular, it results in ambiguity
because maintainers can accidentally add something like
``{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': True}`` and
``{'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key', 'enabled': False}`` to the same event_type.
Also, as written, this configuration requires maintainers to copy over the entire configuration to override it, which is non-obvious
to people who may only be trying to enable/disable a single event while keeping everything else the same. Moreover, it's also non-obvious
that enabling/disabling an existing event/topic pair requires reaching into the structure, searching for the dictionary with the correct topic, and modifying
the existing object, which is awkward.

This ADR aims to propose a new structure that will provide greater flexibility in using this configuration.

Decision
********

The new EVENT_BUS_PRODUCER_CONFIG will have the following configuration format:

.. code-block:: python

# .. setting_name: EVENT_BUS_PRODUCER_CONFIG
# .. setting_default: {}
# .. setting_description: Dictionary of event_types to dictionaries for topic related configuration.
# Each topic configuration dictionary uses the topic as a key and contains a flag called `enabled`
# denoting whether the event will be and `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.xblock.published.v1': {
'content-authoring-xblock-lifecycle': {'event_key_field': 'xblock_info.usage_key', 'enabled': False}
robrap marked this conversation as resolved.
Show resolved Hide resolved
'content-authoring-xblock-published': {'event_key_field': 'xblock_info.usage_key', 'enabled': True}
},
'org.openedx.content_authoring.xblock.deleted.v1': {
'content-authoring-xblock-lifecycle': {'event_key_field': 'xblock_info.usage_key', 'enabled': True},
},
}

In edx-platform, it will be added to the KEYS_WITH_MERGED_VALUES list to allow partial additions/overrides.
robrap marked this conversation as resolved.
Show resolved Hide resolved

Consequences
************

* As long as the implementing IDA has a concept of KEYS_WITH_MERGED_VALUES (more complex configurations that can be modified via code in settings), maintainers can add existing topics to new event_types without having to recreate the whole dictionary
* There is no ambiguity about whether an event/topic pair is enabled or disabled
1 change: 1 addition & 0 deletions docs/decisions/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ Architectural Decision Records (ADRs)
0010-multiple-event-types-per-topic
0011-depending-on-multiple-event-bus-implementations
0012-producing-to-event-bus-via-settings
0013-new-event-bus-producer-config
49 changes: 34 additions & 15 deletions openedx_events/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@ def general_signal_handler(sender, signal, **kwargs): # pylint: disable=unused-
"""
Signal handler for publishing events to configured event bus.
"""
configurations = getattr(settings, "EVENT_BUS_PRODUCER_CONFIG", {}).get(signal.event_type, ())
configurations = getattr(settings, "EVENT_BUS_PRODUCER_CONFIG", {}).get(signal.event_type, {})
rgraber marked this conversation as resolved.
Show resolved Hide resolved
event_data = {key: kwargs.get(key) for key in signal.init_data}
for configuration in configurations:
if configuration["enabled"]:

for topic in configurations.keys():
if configurations[topic]["enabled"]:
rgraber marked this conversation as resolved.
Show resolved Hide resolved
get_producer().send(
signal=signal,
topic=configuration["topic"],
event_key_field=configuration["event_key_field"],
topic=topic,
event_key_field=configurations[topic]["event_key_field"],
event_data=event_data,
event_metadata=kwargs["metadata"],
)
Expand All @@ -34,47 +35,65 @@ class OpenedxEventsConfig(AppConfig):

name = "openedx_events"

def _get_validated_signal_config(self, event_type, configurations):
def _get_validated_signal_config(self, event_type, configuration):
"""
Validate signal configuration format.

Example expected signal configuration:
{
"topic_a": { "event_key_field": "my.key.field", "enabled": True },
"topic_b": { "event_key_field": "my.key.field", "enabled": False }
}

Raises:
ProducerConfigurationError: If configuration is not valid.
"""
if not isinstance(configurations, list) and not isinstance(configurations, tuple):
if not isinstance(configuration, dict) and not isinstance(configuration, dict):
robrap marked this conversation as resolved.
Show resolved Hide resolved
raise ProducerConfigurationError(
event_type=event_type,
message="Configuration for event_types should be a list or a tuple of dictionaries"
message="Configuration for event_types should be a dict"
)
try:
signal = OpenEdxPublicSignal.get_signal_by_type(event_type)
except KeyError as exc:
raise ProducerConfigurationError(message=f"No OpenEdxPublicSignal of type: '{event_type}'.") from exc
for configuration in configurations:
if not isinstance(configuration, dict):
for _, topic_configuration in configuration.items():
print(f"{topic_configuration=}")
if not isinstance(topic_configuration, dict):
raise ProducerConfigurationError(
event_type=event_type,
message="One of the configuration object is not a dictionary"
message="One of the configuration objects is not a dictionary"
Copy link
Contributor

Choose a reason for hiding this comment

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

Imagine everything is working fine, and someone adds a new bad config detail. Do we want that to break all working publishing? Seems scary. Something to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm going to punt on this since it requires a little more thought than I want to put it while trying to land this but it will be a fast follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[punt]

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to fail early rather than allowing a back config detail? This error will stop the application from starting.

)
expected_keys = {"topic": str, "event_key_field": str, "enabled": bool}
expected_keys = {"event_key_field": str, "enabled": bool}
for expected_key, expected_type in expected_keys.items():
if expected_key not in configuration:
if expected_key not in topic_configuration.keys():
raise ProducerConfigurationError(
event_type=event_type,
message=f"One of the configuration object is missing '{expected_key}' key."
)
if not isinstance(configuration[expected_key], expected_type):
if not isinstance(topic_configuration[expected_key], expected_type):
raise ProducerConfigurationError(
event_type=event_type,
message=(f"Expected type: {expected_type} for '{expected_key}', "
f"found: {type(configuration[expected_key])}")
f"found: {type(topic_configuration[expected_key])}")
)
return signal

def ready(self):
"""
Read `EVENT_BUS_PRODUCER_CONFIG` setting and connects appropriate handlers to the events based on it.

Example expected configuration:
{
"org.openedx.content_authoring.xblock.deleted.v1" : {
"topic_a": { "event_key_field": "xblock_info.usage_key", "enabled": True },
"topic_b": { "event_key_field": "xblock_info.usage_key", "enabled": False }
},
"org.openedx.content_authoring.course.catalog_info.changed.v1" : {
"topic_c": {"event_key_field": "course_info.course_key", "enabled": True }
}
}

Raises:
ProducerConfigurationError: If `EVENT_BUS_PRODUCER_CONFIG` is not valid.
"""
Expand Down
19 changes: 11 additions & 8 deletions openedx_events/tests/test_producer_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,34 @@ def test_configuration_is_validated(self):
with pytest.raises(ProducerConfigurationError, match="should be a dictionary"):
apps.get_app_config("openedx_events").ready()

with override_settings(EVENT_BUS_PRODUCER_CONFIG={"invalid.event.type": []}):
with override_settings(EVENT_BUS_PRODUCER_CONFIG={"invalid.event.type": {}}):
with pytest.raises(ProducerConfigurationError, match="No OpenEdxPublicSignal of type"):
apps.get_app_config("openedx_events").ready()

with override_settings(EVENT_BUS_PRODUCER_CONFIG={"org.openedx.content_authoring.xblock.deleted.v1": ""}):
with pytest.raises(ProducerConfigurationError, match="should be a list or a tuple"):
with pytest.raises(ProducerConfigurationError, match="should be a dict"):
apps.get_app_config("openedx_events").ready()

with override_settings(EVENT_BUS_PRODUCER_CONFIG={"org.openedx.content_authoring.xblock.deleted.v1": [""]}):
with pytest.raises(ProducerConfigurationError, match="object is not a dictionary"):
with override_settings(EVENT_BUS_PRODUCER_CONFIG={"org.openedx.content_authoring.xblock.deleted.v1":
{"topic": ""}}):
with pytest.raises(ProducerConfigurationError, match="One of the configuration objects is not a"
" dictionary"):
apps.get_app_config("openedx_events").ready()

with override_settings(
EVENT_BUS_PRODUCER_CONFIG={
"org.openedx.content_authoring.xblock.deleted.v1": [{"topic": "some", "enabled": True}]
"org.openedx.content_authoring.xblock.deleted.v1": {"some": {"enabled": True}}
rgraber marked this conversation as resolved.
Show resolved Hide resolved
}
):
with pytest.raises(ProducerConfigurationError, match="missing 'event_key_field' key."):
apps.get_app_config("openedx_events").ready()

with override_settings(
EVENT_BUS_PRODUCER_CONFIG={
"org.openedx.content_authoring.xblock.deleted.v1": [
{"topic": "some", "enabled": 1, "event_key_field": "some"}
]
"org.openedx.content_authoring.xblock.deleted.v1":
{
"some": {"enabled": 1, "event_key_field": "some"}
}
}
):
with pytest.raises(
Expand Down
28 changes: 7 additions & 21 deletions test_utils/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,14 @@

SECRET_KEY = "not-so-secret-key"
EVENT_BUS_PRODUCER_CONFIG = {
'org.openedx.content_authoring.xblock.published.v1': (
'org.openedx.content_authoring.xblock.published.v1':
{
'topic': 'content-authoring-xblock-lifecycle',
'event_key_field': 'xblock_info.usage_key',
'enabled': True
'content-authoring-xblock-lifecycle': {'event_key_field': 'xblock_info.usage_key', 'enabled': True},
'content-authoring-all-status': {'event_key_field': 'xblock_info.usage_key', 'enabled': True},
'content-authoring-xblock-published': {'event_key_field': 'xblock_info.usage_key', 'enabled': False}
robrap marked this conversation as resolved.
Show resolved Hide resolved
},
'org.openedx.content_authoring.xblock.deleted.v1':
{
'topic': 'content-authoring-all-status',
'event_key_field': 'xblock_info.usage_key',
'enabled': True
},
{
'topic': 'content-authoring-xblock-published',
'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': True
},
),
'content-authoring-xblock-lifecycle': {'event_key_field': 'xblock_info.usage_key', 'enabled': True},
}
}
Loading