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

Stop tracking registry accesses #197

Merged
merged 1 commit into from
Apr 20, 2022
Merged

Stop tracking registry accesses #197

merged 1 commit into from
Apr 20, 2022

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Apr 12, 2022

PULL REQUEST

Overview

As we have high priority work that trumps DB performance tracking and optimisations, we wanted to have some quick wins before we park this effort. This PR provides those quick wins by removing registry tracking.

What's done here: Stop tracking registry accesses and drop all DB collections related to those.

Example for Visual Changes

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Issues Closed

@ro-tex ro-tex self-assigned this Apr 12, 2022
@ro-tex ro-tex requested a review from MSevey April 13, 2022 11:56
@MSevey
Copy link
Contributor

MSevey commented Apr 13, 2022

Is registry tracking useful information to have at some point? Are we just dropping it because it is currently impacting performance?

If so, we could disable the API but still keep the data collections right?

@ro-tex
Copy link
Contributor Author

ro-tex commented Apr 14, 2022

Is registry tracking useful information to have at some point? Are we just dropping it because it is currently impacting performance?

If so, we could disable the API but still keep the data collections right?

  1. We don't have any indication that we have (or don't have) any performance issues. I keep repeating this but I want to make it clear that we're basing all of this on gut feeling and no data.
  2. Personally, I find it positive to have this metric and I would have preferred to keep it. The fact is, though, that since its inception nobody has ever looked at it, so maybe I'm alone in my interest in it.
  3. The data collection is the only action we perform on this data. The DB writes are the only thing that could potentially affect our performance.

In a perfect world, my preferred solution would be:

  1. Keep gathering the metrics.
  2. Replace our current strategy of one DB record per read/write with an aggregation - one record which holds the number of reads and write performed by a single user for each day. (Anon actions will be grouped together.) This will reduce the fidelity of the statistics (we'll only know the calls per day and not specific times during the day) but it will replace a relatively expensive record creation with a relatively inexpensive atomic increment of an integer field.
  3. Run a batch job (either manually or in some form of compat code) that aggregates all existing statistics into such aggregations.

The reason not to go for that solution right now is that it's going to take longer (1 day of dev time + however long it takes to merge it) vs this near-instant solution (15-30 minutes + however long it takes to merge).

@ro-tex ro-tex requested a review from MSevey April 14, 2022 10:40
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

LGTM

@ro-tex ro-tex enabled auto-merge April 20, 2022 15:20
@MSevey MSevey requested a review from peterjan April 20, 2022 16:03
@ro-tex ro-tex merged commit 348c073 into main Apr 20, 2022
@ro-tex ro-tex deleted the ivo/stop_tracking_registry branch April 20, 2022 16:03
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.

3 participants