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 signal for event-tracking emission #230

Merged
merged 8 commits into from
Nov 14, 2023

Conversation

Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Jun 2, 2023

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:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

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)

@openedx-webhooks
Copy link

openedx-webhooks commented Jun 2, 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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 2, 2023
@Ian2012 Ian2012 force-pushed the cag/add-openedx-event-signal branch 4 times, most recently from 5eb5b1a to 81f9bbe Compare June 5, 2023 16:36
@Ian2012 Ian2012 marked this pull request as ready for review June 5, 2023 16:47
@Ian2012 Ian2012 requested a review from a team as a code owner June 5, 2023 16:47
@Ian2012 Ian2012 force-pushed the cag/add-openedx-event-signal branch from 81f9bbe to f37a213 Compare June 5, 2023 16:50
@mariajgrimaldi
Copy link
Member

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 :)

@bmtcril
Copy link
Contributor

bmtcril commented Jun 6, 2023

This is also blocked on #224 and its implementation

@Ian2012
Copy link
Contributor Author

Ian2012 commented Jun 6, 2023

@bmtcril why is it blocked by that PR?

I was thinking that this feature flag would prevent performance issues on large installations:

https://github.com/openedx/event-routing-backends/pull/305/files#diff-475f912f3206aed7a6cbb4ac6a1954040e433ff426ce93f798c2700558e34d88R14

Or am I missing something?

@mariajgrimaldi
Copy link
Member

Hi! is this ready for our review? If it's not, then please convert it to draft :)! thanks

@bmtcril
Copy link
Contributor

bmtcril commented Jun 12, 2023

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

@mariajgrimaldi mariajgrimaldi marked this pull request as draft June 13, 2023 15:50
@mphilbrick211
Copy link

Hi @bmtcril and @mariajgrimaldi - just checking in on this :)

@bmtcril
Copy link
Contributor

bmtcril commented Aug 24, 2023

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.

Comment on lines +29 to +30
data = attr.ib(type=str)
context = attr.ib(type=str)
Copy link
Contributor Author

@Ian2012 Ian2012 Oct 26, 2023

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

@Ian2012 Ian2012 marked this pull request as ready for review October 26, 2023 16:51
@Ian2012
Copy link
Contributor Author

Ian2012 commented Oct 26, 2023

@bmtcril @mariajgrimaldi this is ready for review now

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.

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.

@mariajgrimaldi
Copy link
Member

I'll be reviewing this. Thanks!

@mariajgrimaldi mariajgrimaldi self-assigned this Oct 27, 2023
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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!

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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

@mphilbrick211
Copy link

Hi @mariajgrimaldi! Are you able to merge this?

@mphilbrick211
Copy link

Hi @mariajgrimaldi! Are you able to merge this?

@bmtcril - is this something you can merge if Maria is unable?

@mariajgrimaldi
Copy link
Member

@mphilbrick211: I was waiting for Cristhian to update the init file. I'll be merging this now.

@mariajgrimaldi mariajgrimaldi merged commit 39f1f67 into openedx:main Nov 14, 2023
8 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.

@mphilbrick211
Copy link

Sorry about that, @mariajgrimaldi - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants