feat: migrate to API rewrite #351
Conversation
ecxyzzy
changed the title
|
The term indicator issue is tracked on the API end, and I can write a solution for it before this gets merged if needed, but most of the substantial bugs and blockers should be resolved. Thanks for the help and sorry about the delay. |
All good, it can wait since we still have an outstanding issue on our end to make the term indicator relevant. |
There was a problem hiding this comment.
Thanks, search page is much faster now and mostly everything looks to be resolved.
I did notice that AP classes in the prerequisite tree have nonexistent links to course pages but that was preexisting problem so I opened a separate issue for it (#385)
Co-authored-by: Jacob Sommer <[email protected]>
|
The latest staging build now points to an API staging instance which has the GraphQL change. It still seems quite slow to me which makes me think this is a symptom of a larger issue. Could you confirm whether this is the case? |
|
Yeah, still seems slow. |
|
Also, the review form is still broken. Seems to just be that the GraphQL response for professors is not mapped properly. |
|
Some really rough performance measurements from throwing print statements in a few places. In general, API rewrite does seem to be a lot slower at responding for course queries, instructors queries seem fine from what I can tell (this is with the batch REST requests for courses and single GraphQL query for instructors). Some improvement could probably be done on our end as well so I might draft up a PR for that. edit: my brain is fried right now. omitted the api times since i didn't measure them right. total time should be roughly accurate, it varies a lot between runs though api rewritesearch page
course page
professor page
originalsearch page
course page
professor page
|
|
Thanks for running the numbers. They're definitely worse than I thought; while I suspect it is mostly cold start and/or DB overhead, it's still a pretty hefty penalty even if it doesn't happen that often. I'll look into this further after Wednesday since I should be done with everything except finals by then. |
|
Take your time. For future reference, I ran some more robust tests. Max, p95, and p99 stood out to me as significantly higher with the rewrite. response times (ms) with one request sent every second for 60 seconds original
rewritewith batch REST requests for course query
with single GraphQL request for course query
|
There was a problem hiding this comment.
Something's up with the prisma set up on the staging instances. Following endpoints aren't working:
- https://staging-117.api-next.peterportal.org/v1/rest/week
- https://staging-117.api-next.peterportal.org/v1/rest/websoc/terms
- https://staging-117.api-next.peterportal.org/v1/rest/grades/raw?department=COMPSCI&courseNumber=122B
Overall, does seem significantly faster. There is still the persisting slowdown with having to make the extra requests for prerequisite list, prerequisite for, and instructor history on course pages though, especially for those with a lot of prerequisites/prerequisite fors, such as Math 2B.
Below is for Math 2B (tested locally). Requests are for instructor history, prerequisite list, and prerequisite for respectively.
Is there a reason why you didn't include the nested GraphQL queries for the api rewrite? I feel like it's been causing a bit of a headache.
I can think of an improvement for this implementation though.
We are requesting much more information than needed for instructor history and prerequisites.
e.g. for each instructor in instructor history
{
"_0": {
"name": "Natalia Komarova",
"shortenedName": "KOMAROVA, N.",
"ucinetid": "komarova",
"title": "Professor",
"department": "Mathematics",
"schools": [
"School of Biological Sciences",
"School of Physical Sciences"
],
"relatedDepartments": [
"BIO SCI",
"DEV BIO",
"ECO EVO",
"MOL BIO",
"NEURBIO",
"CHEM",
"EARTHSS",
"MATH",
"PHY SCI",
"PHYSICS"
],
"courseHistory": {
"MATH 2B": [
...In the prior implementation with the nested queries, we only selected ucinetid, shortened name, and full name for each instructor. I assume doing the same here could result in significant improvements. Same goes for prerequisite list/for, we only selected id, department, number, and title. Whether or not this will be as fast as the single nested query though, I'm not sure.
Thanks for pointing this out, I think I forgot to upgrade the query engine version to the one that the Node 20 lambda runtime supports. Pretty sure things should be working now though.
Yeah now that I'm actually integrating the new API with PPC it seems like there was a reason that things were done a certain way. (Go figure.) I think a way forward that we could consider is to have a separate sub-endpoint that returns nested entries for courses/instructors. I have a rough idea of how to implement this but will still need to work through it, and I'm unsure as to what the performance will look like compared to fetching everything, but it stands to reason that we could reduce the overhead of making multiple HTTP requests. |
|
I think the performance issues should be resolved for good this time but can't tell for sure due to regional latency. Could you check on your end? |
|
Deployed staging instance to https://staging-351.peterportal.org |
Summary
Migrate PeterPortal Client to the API rewrite.
After [too long] in development, hopefully it will have been worth the wait.TODO before merging: remove the hardcoded API URLs and update the CI variables to point to the API rewrite.
Test Plan
Overall: Check that the site still works like it does before, and doesn't crash. I tested most cases I could think of, but there are probably some edge and corner cases that I missed since I'm not that familiar with the codebase.
Specific issues:
Known Issues and Next Steps
Changing search tabs doesn't populate results #352Not fetching professor data when user clicks professor tab #305
Issues
Closes #224; closes #238; closes #301; closes #343; closes #344; closes #350.