Skip to content
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

Remove use of python ua-parser from the backend. #8401

Merged
merged 1 commit into from Sep 7, 2021

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Sep 6, 2021

Summary

  • The Python ua-parser module is exhaustive, and as a result requires at least a thousand regexes.
  • Currently it initializes them all at module import, which causes anywhere up to a 5 second delay in initializing Kolibri on a highly specced dev machine
  • We could defer import until after initialization, but that would still mean a very lengthy initialization process at some point
  • To avoid this, instead just do all user agent parsing in the frontend using the ua-parser-js library (which is far more performant than other user agent libraries), and which we were already partially using in vendored form
  • Removes vendoring, and imports this library
  • Uses this for all browser and os info inspection from the user agent
  • Explicitly sends this info with session requests to be recorded in the user session logs
  • Changes all session requests to use POST or PUT
  • Sets 'active' on login POSTs so that user session logs are created
  • Removes unneeded SessionResource

References

Removes major initialization bottleneck, reducing startup time from ~5s to < 0.1s.

Follow up from #7658

Reviewer guidance

Does login and subsequent pings to the session endpoint work?
Does appropriate data get recorded in the UserSessionLog?
Is startup performance on Android improved?


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Sep 6, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Sep 6, 2021
Only do user agent parsing in frontend.
@indirectlylit
Copy link
Contributor

~5s to < 0.1s.

Wow, that's huge!

Will review and test later this week

Follow up from #7658

Since that was merged into develop after 0.14, presumably the slow startup time didn't hit most users (except I suppose S2S?)

@rtibbles
Copy link
Member Author

rtibbles commented Sep 7, 2021

Since that was merged into develop after 0.14, presumably the slow startup time didn't hit most users (except I suppose S2S?)

Yes, and that's why I targeted it to 0.15, so we could prevent this issue even there.

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, nothing to comment.
All the functionalities work well without regressions.
Tested it in linux (dev), android and windows 10. I haven't measured the improvement when starting but data logs are now better, at least detecting the Linux version.

Before:
before

After:
after

@rtibbles rtibbles merged commit 61a06db into learningequality:release-v0.15.x Sep 7, 2021
@rtibbles rtibbles deleted the js_only_uap branch September 7, 2021 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants