-
Notifications
You must be signed in to change notification settings - Fork 21
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 signal for event-tracking emission #230
feat: add signal for event-tracking emission #230
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. |
5eb5b1a
to
81f9bbe
Compare
81f9bbe
to
f37a213
Compare
Please, add a bit more info in the cover letter! Like how to test, the need for the event, etc. So the community knows how important it is :) |
f37a213
to
2982b33
Compare
This is also blocked on #224 and its implementation |
@bmtcril why is it blocked by that PR? I was thinking that this feature flag would prevent performance issues on large installations: Or am I missing something? |
Hi! is this ready for our review? If it's not, then please convert it to draft :)! thanks |
I think we should flip this to draft and round up on the process some more. I'm confused why the producer is in event-routing-backends and not event-tracking, and want to make sure we both understand how it's all supposed to be working together |
Hi @bmtcril and @mariajgrimaldi - just checking in on this :) |
I think this work is punted to Aspects v2 unless someone picks it up first. I can close it for now, but would prefer to leave it in draft if we can. |
2982b33
to
6af425c
Compare
e053881
to
26d8b6b
Compare
data = attr.ib(type=str) | ||
context = attr.ib(type=str) |
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.
As the event bus doesn't support dictionaries we are sending a JSON string version of the context and the extra data. It's expected that the event bus consumer to handle this information
@bmtcril @mariajgrimaldi this is ready for review now |
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.
This makes sense to me. When we get to the epic task of updating tracking logs to formalize them I hope we can revisit the dictionary issue, but I think this will get the job done. I'm not a committer here so I won't thumb it, but lgtm.
I'll be reviewing this. Thanks! |
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.
I left a few comments for you folks to review!
0a293bc
to
2bc6023
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! Thank you folks
Hi @mariajgrimaldi! Are you able to merge this? |
0542beb
to
689ab46
Compare
@bmtcril - is this something you can merge if Maria is unable? |
@mphilbrick211: I was waiting for Cristhian to update the init file. I'll be merging this now. |
@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. |
Sorry about that, @mariajgrimaldi - thank you! |
Description: This PR defines a signal that listens to every event-tracking emission event. It's currently needed to emit events via the event bus to the OARS stack to have real-time events and better performance than the current implementation.
ISSUE: #215
Dependencies: None
Testing instructions: More details in openedx/event-tracking#246
Merge deadline: This is needed for the Aspects project to move the tracking log processing from celery to async processing, more details in this PR openedx/event-routing-backends#361
Reviewers:
Merge checklist:
Post merge:
finished.