-
Notifications
You must be signed in to change notification settings - Fork 18
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: allow to route events in sync #361
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. |
d6b3fa6
to
d80db69
Compare
e2fe19a
to
1756e91
Compare
350ebc2
to
e142f24
Compare
I can imagine some people that would like to still use the celery backend instead of the event bus. Before the settings were connected to the Async one, and I've changed it to the EventBus. How can we automagically update our own settings to define when to use one or the other? |
b138dde
to
6b9530b
Compare
@Ian2012 if you don't want to process EventBus events asnchronously you can use this engine |
We are implementing a new backend for these reasons:
I'm in the process of refactoring the EventRouter so that there are two classes to send events. One for using celery and another one for sending events in the same thread. @bmtcril The current implementation will process and load events to the backends in the same thread but I'm not sure if that's what we really want. I see some advantages in performing everything in the same thread and defining an "IDA" in Aspects, but then we will lose the retry capabilities and we will have to perform more operator tasks related to the event bus. |
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.
Still trying to wrap my head around it all, but here are some initial comments
d43da89
to
a76dc9c
Compare
@bmtcril added new test cases and updated the docs |
cd8ccb0
to
e73201c
Compare
@bmtcril added batching capabilities. Hopefully, we can implement a management command to trigger the consumption of events every N seconds, but I'm worried about race conditions unless we only send events every N seconds and not every X event. I'm using a Redis list to save the events to be sent to the backends (xAPI, Caliper) then those are processed and sent in batch to the backends |
5fd42a5
to
3e724fd
Compare
cb0476d
to
14df49d
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.
@Ian2012 Thanks for making these changes. I have a question could you please answer that?
f6c89f8
to
3a21781
Compare
3a21781
to
9d5e704
Compare
@ziafazal @bmtcril This is finally ready for review, it's mainly a refactor however we should release and tag this to allow people to configure the event bus backend. Would you agree to not change the default settings here but to provide a flag in I'm thinking on |
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 👍
@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 handler to process tracking log from the event bus.
Testing instructions
EVENT_TRACKING_BACKENDS
settings to use the engineeventtracking.backends.event_bus.EventBusRoutingBackend
and update the caliper and xapi backends to use:event_routing_backends.backends.sync_events_router.SyncEventsRouter
tutor dev exec lms ./manage.py lms consume_events -t analytics -g event_routing_backends --extra '{"consumer_name": "aspects"}'