Content libraries analytics enhancements (SOL-121) - #26
Conversation
There was a problem hiding this comment.
@bradenmacdonald might make sense to extract this self.runtime.publish(...) into a function - it's duplicated here and on lines 210-214 with only one different parameter.
|
👍 so far, but without tests it has no chances to pass upstream review. Some ideas on testing:
|
|
@e-kolpakov Tests are now added. Thanks again for the suggestion. |
dcc309c to
99982cd
Compare
302980e to
c9ae808
Compare
|
In terms of implementation, this looks great so far! I do have a concern about whether to (and how to) represent dependents of the block in question, but I do not want to have that discussion in this pull request. But I was interested to know if the scope of this pull request was going to include the work to instrument problem_check events with the LibraryUsageLocator and Library version guid of the source library of all blocks copied from a content library? (I will update the "analytics considerations" section to make that more explicit.) |
d7cf66b to
241e68d
Compare
241e68d to
7f97429
Compare
5c51cae to
a80ca34
Compare
48b4591 to
05a8716
Compare
a80ca34 to
a514d6d
Compare
|
@brianhw I have now implemented two changes:
Please see the updated description of this PR and let me know what you think! |
|
tagging @stroilova and @lamagnifica |
|
Hi @bradenmacdonald, two things that would be helpful from a doc perspective: would you please provide a list of the existing events that, like problem_check, will now have the added tracking context data, and would it be possible to see an example of the "removed" event as well as the "assigned" event? Thanks! |
|
Thank you very much for the clear description and sample events, @bradenmacdonald ! In general, this information and structure looks good. I think there is some discussion about descendants and the removed sample event in the parallel email thread. For now, there is only one thing that jumped out at me: What do you think about converting the event.added and event.result fields of the newly added events from a list into a dictionary? Perhaps then the three fields could be labeled with the same names as the three corresponding fields in the problem_check context.module dictionary field listed above (usage_key, original_usage_version, original_usage_key) Thanks. |
|
I think it would be helpful to figure out what the edx.librarycontentblock.content events will look like when the content blocks are not leaf nodes but have descendants. In this scenario, it is helpful to know all the descendants that have been "assigned" to the student. I don't think it is important to know about hierarchical relationships among descendants, but I do think it's useful to distinguish the blocks that are explicitly added from the descendant blocks that are implicitly added. This is in part because when we remove deleted blocks, we can't know the descendants that are removed. We have to infer that from finding those descendants that were added back when the deleted block was originally added. Because we would want to know the descendants resulting for an individual block, I propose that we would add the descendants to the entry for the addition. If we follow Olga's suggestion that each addition is output as a dictionary rather than an array, then the dictionary can include a "descendants" key if there are descendants. If there are no descendants, then this can be absent. So something like this: |
|
Thanks @brianhw I'll do that. What about descendants of descendants - should those be listed hierarchically as well? "event": {
"added": [
{"usage_key": "block-v1:BradenX+OVRD_TEST+1+type@splittest+block@c279e825df7fb020cb2c",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@splittest+block@4b0043c672b54878b4600f7b1655435c",
"original_usage_version": "54a2234656c02c0fdaa54f57",
"descendants": [
{"usage_key": "block-v1:BradenX+OVRD_TEST+1+type@problem+block@abcde123456",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@problem+block@b4600f7b1655435c4b0043c672b54878",
"original_usage_version": "54a2234656c02c0fdaa54f57",
"descendants": [
{"usage_key": "block-v1:BradenX+OVRD_TEST+1+type@problem+block@123456abcde",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@problem+block@600f7b1655435c4b0043c672b54878b4",
"original_usage_version": "54a2234656c02c0fdaa54f57"
}
]
},
]
}
],
} |
|
I was suggesting the use of "descendants" to capture all descendants as a On Mon, Jan 5, 2015 at 8:26 PM, Braden MacDonald notifications@github.com
|
|
Thanks @brianhw that sounds good. |
|
@lamagnifica Sure, I'll add an example of the removed event with my next update. As for "a list of the existing events that, like problem_check, will now have the added tracking context data": I'm not actually sure which events will get this data and which won't. It adds the context to the same place and same type of events as https://github.com/edx/edx-platform/pull/2696 . @brianhw do you know? |
a514d6d to
9a9574e
Compare
|
Ok everyone participating in this PR - I have made some changes to include descendant information. Here are some updated examples. First, I made a library with two main blocks - one without children and one with a hierarchy. This is what it looks like: Next, if a student sees a library content block using this library that has recently been edited to go from showing 1 block to showing 2, they will generate this analytics "added" event: {
"username": "honor",
"host": "precise64",
"event_source": "server",
"event_type": "edx.librarycontentblock.content.assigned",
"context": {
"course_user_tags": {},
"user_id": 1,
"org_id": "BradenX",
"course_id": "course-v1:BradenX+TEST+1",
"path": "/courses/course-v1:BradenX+TEST+1/courseware/f61bfa6228e948f59ac1107f6077e00c/1aac970a9128469ea78c8ef1c1d0a965/"
},
"time": "2015-01-06T05:47:09.501831+00:00",
"ip": "10.0.2.2",
"event": {
"added": [
{
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@2a983fb828ee8bf6e34b",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@108c4492010d49c2b3844eebdd99626b",
"original_usage_version": "54ab715f56c02c5a88bb5df9",
"descendants": [
{
"original_usage_version": "54ab716d56c02c5a88bb5dfb",
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@975fbb4d7cde4642e81e",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@d14de4dfc5fc4fc3a2b1e5c08de3c6e8"
},
{
"original_usage_version": "54ab717b56c02c5a88bb5dfd",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@af83d86bd23f3385cb8d",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@120657bdb6714a3096518e4fc020b1a9"
},
{
"original_usage_version": "54ab6fb356c02c591fefcdb9",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@2779f12e43582c7e0789",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@8d79a3991a2d467cb9095bd1fa365e71"
}
]
}
],
"location": "block-v1:BradenX+TEST+1+type@library_content+block@f3ebf2bfd1d047c284deebbff984e4e3",
"result": [
{
"usage_key": "block-v1:BradenX+TEST+1+type@problem+block@c279e825df7fb020cb2c",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@problem+block@4b0043c672b54878b4600f7b1655435c",
"original_usage_version": "54ab718c56c02c5a88bb5dff",
"descendants": []
},
{
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@2a983fb828ee8bf6e34b",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@108c4492010d49c2b3844eebdd99626b",
"original_usage_version": "54ab715f56c02c5a88bb5df9",
"descendants": [
{
"original_usage_version": "54ab716d56c02c5a88bb5dfb",
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@975fbb4d7cde4642e81e",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@d14de4dfc5fc4fc3a2b1e5c08de3c6e8"
},
{
"original_usage_version": "54ab717b56c02c5a88bb5dfd",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@af83d86bd23f3385cb8d",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@120657bdb6714a3096518e4fc020b1a9"
},
{
"original_usage_version": "54ab6fb356c02c591fefcdb9",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@2779f12e43582c7e0789",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@8d79a3991a2d467cb9095bd1fa365e71"
}
]
}
]
},
"agent": "Mozilla/5.0 ...",
"page": "x_module"
}And if I then edit the course and change the "count" back to 1, it will generate this analytics removed event when the student next views the course: {
"username": "honor",
"host": "precise64",
"event_source": "server",
"event_type": "edx.librarycontentblock.content.removed",
"context": {
"course_user_tags": {},
"user_id": 1,
"org_id": "BradenX",
"course_id": "course-v1:BradenX+TEST+1",
"path": "/courses/course-v1:BradenX+TEST+1/courseware/f61bfa6228e948f59ac1107f6077e00c/1aac970a9128469ea78c8ef1c1d0a965/"
},
"time": "2015-01-06T05:36:15.601560+00:00",
"ip": "10.0.2.2",
"event": {
"reason": "overlimit",
"removed": [
{
"original_usage_version": "54ab718c56c02c5a88bb5dff",
"descendants": [],
"usage_key": "block-v1:BradenX+TEST+1+type@problem+block@c279e825df7fb020cb2c",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@problem+block@4b0043c672b54878b4600f7b1655435c"
}
],
"location": "block-v1:BradenX+TEST+1+type@library_content+block@f3ebf2bfd1d047c284deebbff984e4e3",
"result": [
{
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@2a983fb828ee8bf6e34b",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@108c4492010d49c2b3844eebdd99626b",
"original_usage_version": "54ab715f56c02c5a88bb5df9",
"descendants": [
{
"original_usage_version": "54ab716d56c02c5a88bb5dfb",
"usage_key": "block-v1:BradenX+TEST+1+type@vertical+block@975fbb4d7cde4642e81e",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@vertical+block@d14de4dfc5fc4fc3a2b1e5c08de3c6e8"
},
{
"original_usage_version": "54ab717b56c02c5a88bb5dfd",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@af83d86bd23f3385cb8d",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@120657bdb6714a3096518e4fc020b1a9"
},
{
"original_usage_version": "54ab6fb356c02c591fefcdb9",
"usage_key": "block-v1:BradenX+TEST+1+type@html+block@2779f12e43582c7e0789",
"original_usage_key": "lib-block-v1:BradenX+OVRD+type@html+block@8d79a3991a2d467cb9095bd1fa365e71"
}
]
}
]
},
"agent": "Mozilla/5.0 ...",
"page": "x_module"
} |
There was a problem hiding this comment.
Can we also add "result" here too? It's just as useful for removed calls as for added calls. In the example scenario you gave, where the count is reduced, we only get the removed event.
There was a problem hiding this comment.
Done (df0db884ee9698c58183244548054dee2d4c1cb9)
|
Looks good to me! I would just request adding "result" to the "removed" events as well, and adding some additional coverage for descendants in the test. (It's also a long test, so perhaps it can be broken up. It would end up with a lot of setup redundancy, but would make the test cases clearer.) Also, do you have thoughts on the issue listed in the "TODO" section? I assume that to get things like this to work correctly in the general case, it would require batching up the analytics calls, and then triggering the calls only when the save is actually performed -- e.g. on some kind of a signal when the transaction is committed. This is something that we might eventually add into the tracking framework, but it's certainly not there now. (Something like django-transaction-hooks might help with this.) But I assume the difference here that save() itself is never called, rather than that the transaction not being committed. In that case, a signal from the save() to emit the tracking events would address this case. But I don't know where such a signal might come from in this case, as I don't know where the save() call is actually made. Alternatively, is there any point at which we know that these changes were thrown away (and therefore add an additional event to tell us to ignore the previous events)? |
|
@brianhw I will add the result to the other events. For the TODO issue, that's the main reason I included the "result" field - because the most recent result can generally be trusted, whereas the sum of all the "added" events cannot. I think the best fix for it is just to change the LMS so that the blocks actually get to save their state when the student goes to the progress page. |
|
I agree that the "result" field is generally useful. We've found that to be true with other events as well (such as enrollment), where failures are rare but still happen. Making the change to the LMS sounds like it is outside the scope of this PR. Moreover, I would be concerned that analyses are currently built on the assumption that changes due to displaying progress are not persisted to courseware_studentmodule. That way, creation dates and the like can be assumed to be due to actual viewing of a problem, rather than a progress page calculation. |
9a9574e to
04191cf
Compare
|
Ok I just pushed two commits to make those changes you suggested @brianhw . |
3e7a8ab to
9dc86d8
Compare
04191cf to
9457364
Compare
There was a problem hiding this comment.
Looks like this comment needs to be updated.
|
The events and the code look good to me. I would just request adding some additional coverage for descendants in the test. (It's also a long test, so perhaps it can be broken up. It would end up with a lot of setup redundancy, but would make the test cases clearer.) It would also be more convenient if the documentation at the top were updated with the changes made later, so that @lamagnifica has only one place to look. @stroilova Can you confirm that the events look good to you now? Also, I really can't comment on the split implementation details. This probably needs review from someone on platform. Don or Cale or John? |
61591ba to
697adb1
Compare
|
Likely @dmitchell would be good at commenting on this - Don, would you have the time to review this PR? It's smaller than the other ones : ) |
…rited into a course
…tion analytics events.
9457364 to
b6178fd
Compare
|
Thanks @brianhw. I will update the tests as you suggest and then I'm going to open an "official" PR on edx-platform, which will have consolidated documentation. |
|
I've now opened a PR on edx-platform so will close this one. Please refer to the new PR here. |
6 participants
This implements support for analytics of content libraries, built to implement the ideas of Brian Wilson and Don Mitchell described on the Confluence wiki page in the section near the top entitled "Analytics Considerations".
New tracking events:
Two new events will appear in the tracking logs (see example below):
edx.librarycontentblock.content.assigned- indicates that a student has been assigned their subset of blocks. Data includes:locationThe BlockUsageLocator of the LibraryContentModule emitting the eventadded: the BlockUsageLocator and LibraryUsageLocator and version of any newly-assigned blocksresult: the complete set of blocks now assigned to this student. (will always equaladdedat first, but ifmax_countis increased,addedmay be different)edx.librarycontentblock.content.removed- indicates that a previously-assigned block is no longer being shown to this student. This should be very rare. Data includes:locationThe BlockUsageLocator of the LibraryContentModule emitting the eventblocks: the BlockUsageLocator and LibraryUsageLocator and version of the blocks that were removedreason: Eitheroverlimit(max_countwas decreased by the course author) orinvalid(Block was deleted from library or library setting was changed and no longer includes this block)Testing Notes
This builds on #25 which stores data in the modulestore differently than previous work on content libraries. If testing this PR or PR 25, create a new course and keep it separate from other content courses that may already exist on your system. You can share libraries between this and other PRs, and once this is merged there will be no need to worry about keeping anything separate.
To test: As the vagrant user you can run
sudo tail -f /edx/var/log/tracking/tracking.logto watch the tracking log. If you sign in to the LMS as a student and view a course containing a LibraryContent block, the first time that you view the courseware should result in an "edx.librarycontentblock.content.assigned" entry appearing in the logs. This entry should appear as below.Also the Django admin interface at http://localhost:8000/admin/courseware/studentmodule/ is helpful for resetting the student state for your test student user.
Example tracking log entry
{ "username": "honor", "host": "precise64", "event_source": "server", "event_type": "edx.librarycontentblock.content.assigned", "context": { "course_user_tags": {}, "user_id": 1, "org_id": "BradenX", "course_id": "course-v1:BradenX+OVRD_TEST+1", "path": "/courses/course-v1:BradenX+OVRD_TEST+1/courseware/f61bfa6228e948f59ac1107f6077e00c/1aac970a9128469ea78c8ef1c1d0a965/" }, "time": "2015-01-02T22:01:17.252372+00:00", "event": { "added": [ ["block-v1:BradenX+OVRD_TEST+1+type@problem+block@c279e825df7fb020cb2c", "lib-block-v1:BradenX+OVRD+type@problem+block@4b0043c672b54878b4600f7b1655435c", "54a2234656c02c0fdaa54f57"] ], "location": "block-v1:BradenX+OVRD_TEST+1+type@library_content+block@f3ebf2bfd1d047c284deebbff984e4e3", "result": [ ["block-v1:BradenX+OVRD_TEST+1+type@problem+block@c279e825df7fb020cb2c", "lib-block-v1:BradenX+OVRD+type@problem+block@4b0043c672b54878b4600f7b1655435c", "54a2234656c02c0fdaa54f57"] ] }, }This PR also adds tracking context data to some normal events like
problem_check:{ "username": "honor", "event_source": "server", "event_type": "problem_check", "context": { "course_user_tags": {}, "user_id": 1, "org_id": "BradenX", "module": { "usage_key": "block-v1:BradenX+OVRD_TEST+1+type@problem+block@c279e825df7fb020cb2c", "original_usage_version": "54a2234656c02c0fdaa54f57", "display_name": "A Friendly Test Problem", "original_usage_key": "lib-block-v1:BradenX+OVRD+type@problem+block@4b0043c672b54878b4600f7b1655435c" }, "course_id": "course-v1:BradenX+OVRD_TEST+1", "path": "/courses/course-v1:BradenX+OVRD_TEST+1/xblock/block-v1:BradenX+OVRD_TEST+1+type@problem+block@c279e825df7fb020cb2c/handler/xmodule_handler/problem_check" }, "time": "2015-01-02T22:08:54.091389+00:00", "event": { "submission": {}, "success": "incorrect" } }TODO:
assignedevents are emitted, because the XBlock assigns children but isn't able to save its state to the database - so in the future, children will be re-assigned.