Performance and test improvement for LearnerMetricsViewSet #260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnbaldwin
changed the title
* Added prefetch_related to the User model in LearnerMetricsViewSet to query the related "CourseEnrollment" records * Updated test coverage to include users, courses, and enrollments for two sites in order to ensure we are retrieving only the data we want and avoid "site bleed". For this, created an initial generalized fixture in "tests/conftest.py". This initial fixture contains User, CourseOverview, CourseEnrollment, Organization, and Site records. If the version of "organizations" is our fork (supports multi-site) then OrganizationCourse and UserOrganizationMapping records are also filled This commit lays the groundwork for additional performance imporovement to reduce the number of queries needed in the serializers used for LearnerMetricsViewSet
johnbaldwin
force-pushed
the
john/learner-metrics-performance
branch
from
a8490dd to
1a60f0b
Compare
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
==========================================
+ Coverage 91.87% 91.95% +0.07%
==========================================
Files 41 41
Lines 2142 2150 +8
==========================================
+ Hits 1968 1977 +9
+ Misses 174 173 -1
Continue to review full report at Codecov.
|
* Added django-environ to devsite settings.py in order to allow setting multisite versus standalone mode in the devsite/devsite/.env file * Updated the requirements files to add `django-environ`
johnbaldwin
force-pushed
the
john/learner-metrics-performance
branch
from
54ea79c to
09f5b9c
Compare
We were doing two queries before. The code now does one query when LCGM "most_recent_for_learner_course" is called. We may still run into performance issues because this means we need to do an individual query per learner and can't get a bunch of learners most recent LCGM records in a single call without either restructuring or augmenting out data. One solution to enabling bulk retrieval is to create an augmented course data table that has a 1:1 to CourseEnrollment that has the most recent grade data or links to the most recent LCGM record
* Now LearnerMetricsViewSet only lists users who have an enrollment. Users who have registerd but not enrolled are not listed * Improved query performance, adding prefetching on users course enrollments
We have a ListSerializer for the learner metrics serialzier, but it is not yet used. How this works now is that we do the query optimization in the view `get_queryset` method. Then the results are processed in the serializer
|
@johnbaldwin is this ready for review? I see that you're still pushing new big changes. |
Removed the `to_representation` method from LearnerMetricsListSerializer because we don't need it
|
Looks like codecov got stuck again |
There were missing tests. We checked for each learner, but didn't check for each learner's enrollments. What was happening with the `prefetch_related` is that it can't filter just the enrollments we want in the related table (or at least I haven't found a way to do this yet) so although we filtered on the learners, we weren't able to filter on the learner enrollments. So now we need to filter the enrollments for each learner against the course id list. Now, we don't strictly need to do this. The front end will filter out unwanted courses. However, when the query params say "I want these courses", then it is good form to just return those courses. If we run into performance issues, then we can return all courses to reduce the query count and come up with an improved solution
|
@OmarIthawi This should be ready for review now. Not sure why codecov is getting stuck again |
|
@OmarIthawi This is ready for review, please. I merged in the Ginkgo fix. |
OmarIthawi
approved these changes
Sep 28, 2020
figures/serializers.py
Outdated
| @@ -876,14 +868,14 @@ def get_enrollments(self, user): | |||
| Rely on the caller (the view) to filter users and prefetch related | |||
| """ | |||
|
|
|||
| user_enrollments = user.courseenrollment_set.all() | |||
| # user_enrollments = user.courseenrollment_set.all() | |||
There was a problem hiding this comment.
Suggested change
| # user_enrollments = user.courseenrollment_set.all() |
There was a problem hiding this comment.
I'll remove the dead code. Thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://appsembler.atlassian.net/browse/RED-1261
django-environto be able to set multi-tenant mode in thedevsite/devsite/.envfile. This is the default locationdjango-environseeksThis commit lays the groundwork for additional performance imporovement to reduce the number of queries needed in the serializers used for LearnerMetricsViewSet