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: add event bus backend #246

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Oct 25, 2023

Description

This PR creates a backend to send tracking logs to the even bus to be processed from an internal consumer

Testing instructions

  • Install openedx-events feat: add signal for event-tracking emission openedx-events#230
  • Install event-tracking from this branch
  • Configure the Redis event-bus
  • Emit a tracking log by interacting as a learner with the course
  • Verify the event is correctly sent to the event bus with an output like:
  • Enable the following setting: SEND_TRACKING_EVENT_EMITTED_SIGNAL
  • The list of allowed events is controlled by this setting EVENT_BUS_TRACKING_LOGS and it's set automatically by ERB.
edx_event_bus_redis.internal.producer] [user 11] [ip 172.19.0.1] producer.py:86 - Message delivered to Redis event bus: topic=dev-analytics, message_id=08044dac-741d-11ee-8695-0242ac13000f, signal=<OpenEdxPublicSignal: org.openedx.analytics.event_tracking.emitted.v1>, redis_msg_id=b'1698337853247-0'

@openedx-webhooks
Copy link

openedx-webhooks commented Oct 25, 2023

Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

requirements/base.in Outdated Show resolved Hide resolved
@Ian2012 Ian2012 marked this pull request as ready for review October 26, 2023 21:30
@Ian2012 Ian2012 force-pushed the cag/add-openedx-event-signal branch 5 times, most recently from 84c4c13 to 28ec6c1 Compare October 30, 2023 17:19
@Ian2012 Ian2012 force-pushed the cag/add-openedx-event-signal branch from ee39f40 to da73352 Compare November 7, 2023 21:02
Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Looks good at first blush overall. I'd like to get the openedx-events PR over the line and try this for real once that's merged.

eventtracking/config.py Outdated Show resolved Hide resolved
eventtracking/config.py Outdated Show resolved Hide resolved
eventtracking/config.py Outdated Show resolved Hide resolved
eventtracking/backends/event_bus.py Outdated Show resolved Hide resolved
eventtracking/backends/event_bus.py Outdated Show resolved Hide resolved
@bmtcril
Copy link
Contributor

bmtcril commented Nov 8, 2023

@UsamaSadiq would you or someone else on ArbiBOM like to review this?

@UsamaSadiq
Copy link
Member

Hi @bmtcril arch-bom is primarily handling the event bus related changes so I believe Rebecca or Tim would be the perfect reviewers for this.

@mphilbrick211
Copy link

Hi @Ian2012! Is this ready for review?

@bmtcril
Copy link
Contributor

bmtcril commented Nov 14, 2023

I'm planning on doing a deep dive review on this tomorrow, fwiw

@Ian2012
Copy link
Contributor Author

Ian2012 commented Nov 14, 2023

@mphilbrick211 yes, waiting on Brian review

@bmtcril
Copy link
Contributor

bmtcril commented Nov 16, 2023

I’ve tried rebuilding the openedx image like 20 times to get this running, had to hack the Dockerfile to explicitly uninstall the old version in order to get your branch to install. I updated my settings to be:

SEND_TRACKING_EVENT_EMITTED_SIGNAL = True
EVENT_TRACKING_BACKENDS["xapi"]["ENGINE"] = "eventtracking.backends.event_bus.EventBusRoutingBackend"
EVENT_TRACKING_BACKENDS["xapi"]["OPTIONS"]["backends"]["xapi"]["ENGINE"] = "eventtracking.backends.event_bus.EventBusRoutingBackend"

But now LMS won’t start:

Traceback (most recent call last):
  File "lms/wsgi.py", line 19, in <module>
    startup.run()
  File "/openedx/edx-platform/./lms/startup.py", line 20, in run
    django.setup()
  File "/openedx/venv/lib/python3.8/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/openedx/venv/lib/python3.8/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/apps.py", line 23, in ready
    override_default_tracker()
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 175, in override_default_tracker
    tracker.register_tracker(DjangoTracker())
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 29, in __init__
    backends = self.create_backends_from_settings(backends_settings_name)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 57, in create_backends_from_settings
    backends = self._instantiate_objects(config)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 112, in _instantiate_objects
    result[key] = self._instantiate_objects(value)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 108, in _instantiate_objects
    result = self._instantiate_from_dict(node)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 143, in _instantiate_from_dict
    options = self._instantiate_objects(options)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 112, in _instantiate_objects
    result[key] = self._instantiate_objects(value)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 112, in _instantiate_objects
    result[key] = self._instantiate_objects(value)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 108, in _instantiate_objects
    result = self._instantiate_from_dict(node)
  File "/openedx/venv/lib/python3.8/site-packages/eventtracking/django/django_tracker.py", line 144, in _instantiate_from_dict
    return cls(**options)
TypeError: __init__() got an unexpected keyword argument 'backend_name'
unable to load app 0 (mountpoint='') (callable not found or import error)

That's as far as I've gotten, any ideas?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Nov 16, 2023

@bmtcril Pop the backend_name from the options:
EVENT_TRACKING_BACKENDS["xapi"]["OPTIONS"].pop("backend_name")

In development, I'm using this Makefile to "hot install" this as an editable requirement:

build:
	tutor dev exec lms bash -c "cd /openedx/requirements && pip install -r private.txt"
	tutor dev exec cms bash -c "cd /openedx/requirements && pip install -r private.txt"
	tutor dev exec lms-worker bash -c "cd /openedx/requirements && pip install -r private.txt"
	tutor dev exec cms-worker bash -c "cd /openedx/requirements && pip install -r private.txt"
	tutor dev restart cms lms lms-worker cms-worker
	tutor dev logs -f --tail 10  lms cms

Just make sure to clone this branch and add the requirement to private.txt as -e ./event-tracking/ and make sure to do the same for openedx-events and event-routing-backends

@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Nov 29, 2023
@Ian2012 Ian2012 force-pushed the cag/add-openedx-event-signal branch 2 times, most recently from cc8dc35 to 12f0c81 Compare November 30, 2023 20:35
@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 6, 2024

We can fix that by simply receiving all arguments in **kwargs. But I guess this is no place to discuss this matter

eventtracking/config.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Ok, I think we can probably revisit when the other work lands. Last thing, can you add docs for the new backend?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 12, 2024

@bmtcril @timmc-edx This one is ready for review

requirements/base.in Outdated Show resolved Hide resolved
@timmc-edx
Copy link
Contributor

Looks all good from an Event Bus perspective.

feat: implement event bus backend

feat: create signal to send tracking log events to event bus

refactor: sent event direclty to event bus
chore: add constraint for openedx-events
fix: only generate metadata

fix: assert for subset ignoring runtime generated only metadata

chore: quality fixes

chore: improve docstring

fix: convert timestamp str to datetime object

test: add test for str datetime

fix: rename tracking log event emitted

fix: rename tracking log event emitted config

fix: rename tracking event

chore: handle comments

feat: add settings for allowed events in the event bus

chore: use opt_in for tracking logs event bus toggle

fix: serialize tracking log data and context dates as logger

chore: use send_event instead of custom metadata

test: update unit test

docs: add event bus routing documentation
Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

LGTM, triple checking to see if there's anyone else who needs to review

@bmtcril
Copy link
Contributor

bmtcril commented Feb 13, 2024

@Ian2012 doesn't seem like there are any other required reviewers, can you bump the version and I'll merge this?

@bmtcril bmtcril self-requested a review February 13, 2024 16:37
@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 13, 2024

@bmtcril @timmc-edx Sorry for the change at the last minute. I've migrated the handler from event-routing-backends here, as this code is scoped to this repository. Do you have any comments on that one?

If there aren't any comments you can merge already

@bmtcril bmtcril merged commit 12bd7bd into openedx:master Feb 14, 2024
6 checks passed
@openedx-webhooks
Copy link

@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants