openedx
feat: deprecate `track_function` and `publish` in ModuleSystem [BD-13]
Description
As a part of the ModuleSystem removal, we want to get rid of its remaining attributes:
track_function,publish.
Supporting information
Sandbox instance: https://pr30046.sandbox.opencraft.hosting/
Use [email protected]:edx to log in.
Testing instructions
- We have replaced the remaining usages of
track_functionwithpublish, so it makes sense to check these two together. - Log into the sandbox instance (private link) and check tracking logs for the next actions.
- Check that the grading is working. You can use this XBlock in the LMS and Preview. Check that the Studio view is working correctly too.
- Check that the "Show answer" button emits events.
- Check that the completion events are working (the completion is already enabled on the sandbox).
Other information
Private-ref: BB-5518
Thanks for the pull request, @Agrendalath!
When this pull request is ready, tag your edX technical lead.
@Agrendalath Just checking the status, as this has been open as a draft for a while.
@Agrendalath I tried to test this out on the sandbox instance. But I got an error in the chemical equations problem:
@pkulkark, it was not related to the PR. It's an old sandbox, so I manually deleted all keys matching default:1:safe_exec.* from the cache. safe_exec is working correctly now.
@ormsbee I don't think this touches any of the answer stuff we track for insights
@ashultz0: Really? I thought this was how all those events got to the tracking logs in the end...?
Maybe I'm missing some of the context because I'm not familiar with this code.
Insights basically only looks at answers (and only our older types of answers), it doesn't engage with any of the completion stuff that seems to be what is being changed here.
Possibly the learner view looked at completion but 1) I don't think it did and 2) I don't have to research it because we are burning it to the ground with community OK anyway.
@ormsbee, thank you for reviewing this. I separated the static_url deprecation to #30992.
@ashultz0, this service also handles events like problem_check (analytics seem to be using it here), showanswer (listed here), etc. This PR should not alter events in any way, but this service will be passing them to the track_function, so it introduces some risk.
thanks @Agrendalath
problem_check is the big one for the answers view so we can just validate that they are still happening as expected afterwards
@pkulkark,
P.S @Agrendalath I assume you'll be rebasing with latest master once the review is done?
Of course. Each of these BD-13 will have a conflict with the master branch once the other BD-13 PR is merged.
@ormsbee, what are the next steps for this PR? Are we waiting for more feedback before moving forward?
@schenedx, @jristau1984: This was the tracking log related code refactoring PR we discussed today.
@ormsbee We found the tracking logs are sent to Segment app on edX side. Because of that, we can quickly verify this PR on stage and prod. Once you feel like this PR is ready, please let me know so we can merge, deploy and test with our people on the look out
@ormsbee OK, So I made a mistake conceptually. I thought the logs sent to segment app is the same as tracking logs.
This is not the case. Tracking logs are log files written into the disk, and ansible has a role to copy those logs into S3 bucket edx-all-tracking-logs. See reference here.
Segment logs are sent to the segment app directly and segment will store those logs inside of S3 into a different S3 bucket. What I don't know is if these two logs are going to be logging different events, or same events or sometimes different sometimes same.
Let's assume the worst that they are logging different events, I believe this logging is affecting pure tracking logs. In order to see the effect of this change, we'd have to monitor the prod instance tracking log writes unto EC2 disk. That's an SRE thing.
I will help get SRE attention onto this PR.
@e0d I asked @ormsbee to help me validate if the code in this change would touch segment tracking events as well. That said, I also wanted to get 2U SRE to be helping me with testing this change on the stage environment to ensure tracking logs are still created. Cannot validate that with the current access I have.
@e0d I am planning to merge this PR and validate on stage Monday, Nov 14th. 2022
@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.
EdX Release Notice: This PR has been deployed to the production environment.