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.

Sign up for GitHub

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

Jump to bottom

Distributed error reporting: Task to ping telemetry with all the tracked errors #12357

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Distributed error reporting: Task to ping telemetry with all the tracked errors #12357

merged 6 commits into from
Jul 1, 2024

Conversation

thesujai
Copy link
Contributor

Summary

  1. Create and register task ping_error_reports that will post to telemetry
  2. Enqueue the ping_error_reports when _ping is successful, else do nothing

References

Closes #12356

Reviewer guidance

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

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: medium labels Jun 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 25, 2024 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-.pex
Windows Installer (EXE) kolibri-0.16.2b2.dev0+git.1188.g8aa4d738-windows-setup-unsigned.exe
Debian Package kolibri_0.16.2b2.dev0+git.1188.g8aa4d738-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.2b2.dev0+git.1188.g8aa4d738-0.4.2.dmg
Android Package (APK) kolibri-0.16.2b2.dev0+git.1188.g8aa4d738-0.1.3-debug.apk
TAR file kolibri-0.16.2b2.dev0+git.1188.g8aa4d738.tar.gz
WHL file kolibri-0.16.2b2.dev0+git.1188.g8aa4d738-py2.py3-none-any.whl

akolson
akolson requested changes Jun 26, 2024
View reviewed changes
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @thesujai! Great work. I left a few comments to improve the code.

@thesujai
Copy link
Contributor Author

Resolved all reviews

akolson
akolson reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

LGTM! @rtibbles any other thoughts on the ping task?

rtibbles
rtibbles reviewed Jul 1, 2024
View reviewed changes
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.

Some possible cleanup that might be good here.

kolibri/core/analytics/tasks.py Outdated Show resolved Hide resolved
kolibri/core/errorreports/tasks.py Outdated Show resolved Hide resolved
kolibri/core/errorreports/tasks.py Outdated Show resolved Hide resolved
kolibri/core/errorreports/tasks.py Outdated Show resolved Hide resolved
@thesujai
Copy link
Contributor Author

thesujai commented Jul 1, 2024

Done with the cleaup

akolson
akolson reviewed Jul 1, 2024
View reviewed changes
kolibri/core/errorreports/tasks.py Outdated
return json.dumps(errors_list, cls=DjangoJSONEncoder)


def mark_errors_as_sent(errors):
Copy link
Member

Choose a reason for hiding this comment

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

Theres not much benefit in having this as a method of its own. We can perform the queryset update within the ping_error_reports task. Richard previous asked about this!

akolson
akolson reviewed Jul 1, 2024
View reviewed changes
kolibri/core/errorreports/tasks.py Outdated
data=errors_json,
headers={"Content-Type": "application/json"},
)
mark_errors_as_sent(errors)
Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/learningequality/kolibri/pull/12357/files#r1661429030

akolson
akolson approved these changes Jul 1, 2024
View reviewed changes
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Changes made! Thanks @thesujai

@akolson akolson merged commit 2d89272 into learningequality:distributed-error-reporting Jul 1, 2024
10 checks passed
@akolson akolson mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtibbles rtibbles rtibbles left review comments

@akolson akolson akolson approved these changes

Assignees
No one assigned
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: medium SIZE: small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thesujai @rtibbles @akolson