feat: use new setting to set ce_source #305

rgraber · 2024-01-24T15:53:23Z

Description: Use a new EVENTS_SERVICE_NAME setting to determine the source in EventsMetadata. Defaults to the old SERVICE_VARIANT setting if not set. SERVICE_VARIANT is usually only set on lms or cms, and has other uses, so it makes sense to have a new variable that all services can use for the events.

I didn't talk about how the EVENTS_SERVICE_NAME will be used to set the client id because that is Kafka specific.

Testing instructions:
pip install -e openedx-events
set EVENTS_SERVICE_NAME in your service settings
fire an event
check that the event source is openedx/<EVENTS_SERVICE_NAME>/web

Reviewers:

tag reviewer
tag reviewer

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

CHANGELOG.rst

robrap · 2024-01-24T17:16:29Z

openedx_events/data.py

+    """
+    Get the value that should be used for the source of an event.
+    """
+    source = getattr(settings, "EVENT_BUS_SERVICE_NAME", None)


I think we need setting annotations here.

Maybe just on the PR, and maybe in the docs, you should explain:

Why we need the new setting, and when we should use it over the SERVICE_VARIANT setting. I imagine you basically wish to deprecate use of SERVICE_VARIANT here?

Is it ok that an "EVENT_BUS" setting will be used for all events, regardless of whether or not they are sent over the event bus? It may be ok, but maybe we should note this.

Not sure if we need a small ADR around this, or if we can just use code comments?

I didn't put settings annotations here because I put them in event-bus-kafka. Not sure what the protocol is when the same setting is used in 2 different places. I can change that.
I'd indeed like to deprecate use of service variant since that only really applies to lms and cms.
I'd be fine with EVENTS_SERVICE_NAME but we do need to decide on one that we'll use both here and in EBK.

I really don't think this needs an ADR

If the setting is only documented once, it should live in this library, since it serves as the interface for the other implementations. One way to make this more explicit would be if you had this library expose a variable like the following, which could be used in all the libraries:

EVENT_BUS_SERVICE_NAME = "EVENT_BUS_SERVICE_NAME"

I agree that SERVICE_VARIANT is probably not the best option.

[inform] @mariajgrimaldi @felipemontoya

@mariajgrimaldi: I don't know if this will notify you twice, but I added the wrong username and had to edit it above. I'm adding this in case you never got a notification.

The PR description explains why we shouldn't use SERVICE_VARIANT. I think it would actually be more confusing to talk about it more in the code comments.

robrap · 2024-01-24T17:31:41Z

openedx_events/tests/test_data.py

+    @ddt.data(
+        ('settings_variant', None, 'openedx/settings_variant/web'),
+        (None, 'my_service', 'openedx/my_service/web'),
+        (None, None, 'openedx//web'),


I wonder if we should do something for this case? Something like the following:

Suggested change

(None, None, 'openedx//web'),

(None, None, 'openedx/unknown_service/web'), # Or `misconfigured_service`?

The setting annotation could detail this so that people know how to interpret whatever shows up.

Co-authored-by: Robert Raposa <[email protected]>

robrap · 2024-01-25T13:44:10Z

openedx_events/data.py

@@ -62,7 +75,8 @@ class EventsMetadata:
    source = attr.ib(
        type=str, default=None,
        converter=attr.converters.default_if_none(
-            attr.Factory(lambda: "openedx/{service}/web".format(service=getattr(settings, "SERVICE_VARIANT", "")))
+            attr.Factory(lambda: "openedx/{service}/web".format(service=get_source_name()) if get_source_name()


NIt:

I'd find this easier to read if on and earlier line you set:

service_name = "unidentified" or _get_service_name(). # see other PR comment for method name change

Consider something like SERVICE-NAME-UNSET in place of unidentified, to give something easy to search for and easy to resolve.

robrap · 2024-01-25T13:44:25Z

openedx_events/data.py

@@ -27,6 +27,19 @@ def _ensure_utc_time(_, attribute, value):
    raise ValueError(f"'{attribute.name}' must have timezone.utc")


+def get_source_name():


Should be get_service_name.

Can we keep this internal-only and call it _get_service_name?

Also note that if this function were private, and just used for this one purpose, it could return SERVICE-NAME-UNSET itself in place of None.

I wanted to call it from event bus kafka

It should still be changed to get_service_name though, right?

It was. I have 2 methods now

rgraber marked this pull request as ready for review January 24, 2024 16:23

rgraber requested a review from a team as a code owner January 24, 2024 16:23

rgraber force-pushed the rsgraber/20240124-fix-source-field branch from 68cc715 to 40ba1bb Compare January 24, 2024 16:24

robrap reviewed Jan 24, 2024

View reviewed changes

Rebecca Graber and others added 12 commits January 24, 2024 13:48

feat: another setting for source in EventsMetadata

9faab6b

fixup!: version

9b7713f

fixup!: default

1c4b2bb

fixup!: unpack

b90d61c

fixup!: oops

5829550

fixup!: quality

db3e34e

fixup!: docdocdoc

fa01834

fixup!: fix name

ba48114

fixup!: fix test

805109b

fixup!: Apply suggestions from code review

34f2300

Co-authored-by: Robert Raposa <[email protected]>

fixup!: keep renaming

76f7ab7

fixup!: fix log

f208269

rgraber force-pushed the rsgraber/20240124-fix-source-field branch from ee7516c to f208269 Compare January 24, 2024 18:48

fixup!: SSSS

847ff73

rgraber requested a review from robrap January 24, 2024 19:17

robrap reviewed Jan 25, 2024

View reviewed changes

Rebecca Graber added 5 commits January 25, 2024 08:55

fixup!: comments from review

ac30fb1

fixup!: fix test

9ae0dd8

fixup!: quality

2ac9491

fixup!: docstrings

8dd3bb6

fixup!: documentation + tweaks

3b7dac4

robrap approved these changes Jan 25, 2024

View reviewed changes

fixup!: documentation

9a1a445

rgraber merged commit 0a7e448 into main Jan 25, 2024
6 checks passed

rgraber deleted the rsgraber/20240124-fix-source-field branch January 25, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use new setting to set ce_source #305

feat: use new setting to set ce_source #305

rgraber commented Jan 24, 2024 •

edited

Loading

robrap Jan 24, 2024

rgraber Jan 24, 2024

rgraber Jan 24, 2024

robrap Jan 24, 2024

robrap Jan 24, 2024 •

edited

Loading

robrap Jan 24, 2024

rgraber Jan 24, 2024

robrap Jan 24, 2024

robrap Jan 25, 2024

robrap Jan 25, 2024

robrap Jan 25, 2024

rgraber Jan 25, 2024

robrap Jan 25, 2024

rgraber Jan 25, 2024

	(None, None, 'openedx//web'),
	(None, None, 'openedx/unknown_service/web'), # Or `misconfigured_service`?

		@@ -27,6 +27,19 @@ def _ensure_utc_time(_, attribute, value):
		raise ValueError(f"'{attribute.name}' must have timezone.utc")


		def get_source_name():

feat: use new setting to set ce_source #305

feat: use new setting to set ce_source #305

Conversation

rgraber commented Jan 24, 2024 • edited Loading

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

robrap Jan 24, 2024 • edited Loading

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 Jan 24, 2024 •

edited

Loading

robrap Jan 24, 2024 •

edited

Loading