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

Fixing issue #6892: collect and report aggregate stats about client browsers through telemetry #7658

Merged
merged 5 commits into from
Nov 4, 2020

Conversation

paulbusse
Copy link
Contributor

@paulbusse paulbusse commented Nov 3, 2020

Summary

The change collects the HTTP user agent in (kolibri/core/auth/api.py)SessionViewset.get_session_response and passes it on to UserSessionLog.update_log. The latter function uses the ua_parser module to parse the HTTP user agent string into the requested string. and saves it to the database.

The data is summarized in extract_facility_statistics.

Other changes:

  • ua_parser 0.10.0 was added to the dependencies
  • logger/migrations/0008_usersessionlog_device_info.py was generated
  • test_extract_facility_statistic was adapted to add the new field

Reviewer guidance

  • the update_log definition has been adapted. I could only find one call to the function. So I did not default the new argument.
  • ua_parser is very fault resistant, I used that in the code. I don't check for None on the families
  • I used a multiline statement to generate the device_info string, that may be against coding guidelines.

I tested this using all available browsers (chrome, edge, firefox, comodo dragon) on all available os's (windows 10, ubuntu). The pingback is not tested.

References


Contributor Checklist

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

Testing:

  • 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

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

paulbusse added 3 commits October 28, 2020 09:29
- Collect the user agent from the request
- call update_log adapted
kolibri/core/logger/models.py
- definition UserSessionLog: added user_agent string
- Collect the user agent from the request
- call update_log adapted
kolibri/core/logger/models.py
- definition UserSessionLog: added user_agent string

kolibri/core/logger/models.py
- Added import from ua parser
- In UserSessionLog.update_log, parse the user_agent string and add the
device_info to the logentry

requirements/base.txt: added ua_parser.

Generated core/logger/migrations/0008_usersessionlog_device_info.py to added
the device info column in the usersessionlog table.
- Added the calculations of the device_info summary as dis to extract_facility_statistics.

kolibri/core/analytics/test/test_utils.py
- Updated the test_extract_facility_statistic
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Everything looks good - one place where the code cold be clearer (in the place with the multi-line string concatenation as you suspected).

Tests only failed because of linting - the easiest way to manage this is to run:

pre-commit install
pre-commit run --all-files

This will run our linting pass on all the files

kolibri/core/logger/models.py Outdated Show resolved Hide resolved
@rtibbles rtibbles added this to the 0.15.0 milestone Nov 3, 2020
@rtibbles rtibbles changed the title Fixing issue #6982: collect and report aggregate stats about client browsers through telemetry Fixing issue #6892: collect and report aggregate stats about client browsers through telemetry Nov 3, 2020
Replacing multiline per rtibbles suggestion

Co-authored-by: Richard Tibbles <[email protected]>
@paulbusse
Copy link
Contributor Author

Thanks for the tip.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I pushed the linting fixes so we can get this merged straight away. Great stuff!

@rtibbles
Copy link
Member

rtibbles commented Nov 4, 2020

Test failure appears to be erroneous, but Travis is not letting me restart the job. Nothing in the PR is touching the code paths that the failing test covers. Merging. Will fix on the release branch if it continues to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

collect and report aggregate stats about client browsers through telemetry
2 participants