-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Video: Add heartbeats to video logging #815
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
Conversation
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.
Please add a comment about what these function do and why they are necessary. To a person who never has anything to do with heartbeats this will be helpful.
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.
Added comments.
|
@nedbat From the point of view of technical execution - I see no further problems with the PR. However, from the point of view of architecture - what do you think? This PR introduces a heartbeat functionality that will trigger logging (every 30 seconds) for every playing video on the edX site. Are you fine with this? @Lyla-Fischer What do you think? |
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 is why you shouldn't try to line up the values in an object: this should have been a one-line change.
|
I don't know much about the logging we do now: I would have thought that video-start, video-end, and navigate-to-page would have been enough to collect the info you want without a heartbeat like this. @cpennington Can you weigh in? |
|
Operationally, this makes me pretty uncomfortable. I would much rather just capture when the user starts a video, stops the video, or leaves the page. What information would the heartbeat capture that logging those events wouldn't? |
|
Can we capture leaving the page? I thought that was like a crash, and that heartbeats was the only way around that. |
|
Yes, we can do that. |
|
http://stackoverflow.com/questions/1704533/intercept-page-exit-event (for instance). Many pages do this to warn you if you have un-saved changes. |
|
sigh okay then. Let's close this pull request, and log a page exit. I'll add a story to JIRA. |
|
https://edx-wiki.atlassian.net/browse/BLD-298 I am kind of suspicious that any arbitrary action can be done on page exit, as that would make it possible to cause an unescapable infinite loop, which seems both bad and exploitable, and I am sure that I am not the first one who thought of this. I leave it to all y'all to figure it out though. I just want the researchers to be able to tell if a student stopped watching a video. |
|
Incidentally, note that the new formulaequationinput type is causing server hits to happen for virtually every keystroke as students type in equations. We're seeing |
…board-course-order Mod course order on dashboard openedx#815
…g-name Fix problem-builder egg name in custom requirements.
Restricted enrolled users reports GDPR
…ntends (#815) This commit made it so frontend services were started as dependencies whenever LMS (or other services) were started. The increased number of Docker containers is causing resource depletion for at least one developer. As a short-term fix, we will revert this commit. This reverts most of commit 54fb57f, although it leaves in place the changes to docs/workflow.rst, as they are still relevant. It also keeps ADR #4, but amends it.
As a researcher, I would like an indication of whether students still have a video playing on their screen so that I have a better idea of whether they are getting the educational benefit of watching the lecture.
Reviewers:
@valera-rozuvan
@nedbat