-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
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:
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. |
b05d73f
to
eef9798
Compare
84c4c13
to
28ec6c1
Compare
ee39f40
to
da73352
Compare
There was a problem hiding this 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.
@UsamaSadiq would you or someone else on ArbiBOM like to review this? |
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. |
b7dc4f7
to
973cbbd
Compare
Hi @Ian2012! Is this ready for review? |
I'm planning on doing a deep dive review on this tomorrow, fwiw |
@mphilbrick211 yes, waiting on Brian review |
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:
But now LMS won’t start:
That's as far as I've gotten, any ideas? |
@bmtcril Pop the backend_name from the options: 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 |
cc8dc35
to
12f0c81
Compare
We can fix that by simply receiving all arguments in |
There was a problem hiding this 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?
d05324b
to
79e800f
Compare
@bmtcril @timmc-edx This one is ready for review |
79e800f
to
38aca82
Compare
Looks all good from an Event Bus perspective. |
38aca82
to
bd6a08a
Compare
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
bd6a08a
to
fa5b0ac
Compare
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
fa5b0ac
to
7347639
Compare
There was a problem hiding this 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
@Ian2012 doesn't seem like there are any other required reviewers, can you bump the version and I'll merge this? |
b0ce5b9
to
777cc59
Compare
777cc59
to
d04ac95
Compare
@bmtcril @timmc-edx Sorry for the change at the last minute. I've migrated the handler from If there aren't any comments you can merge already |
@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. |
Description
This PR creates a backend to send tracking logs to the even bus to be processed from an internal consumer
Testing instructions
SEND_TRACKING_EVENT_EMITTED_SIGNAL
EVENT_BUS_TRACKING_LOGS
and it's set automatically by ERB.