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

perf(replay): reduce postgres queries in recording consumer using get_from_cache #77365

Merged
merged 15 commits into from
Sep 20, 2024

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Sep 11, 2024

@cmanallen pointed out the queries we make in _handle_breadcrumb, used to check feature flags and project options, could overload Postgres as we scale. To fix this we can do the query at the top-level and use get_from_cache

@aliu39 aliu39 requested a review from a team as a code owner September 11, 2024 23:20
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/replays/usecases/ingest/dom_index.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #77365       +/-   ##
===========================================
- Coverage   88.40%   78.18%   -10.22%     
===========================================
  Files        2998     6959     +3961     
  Lines      186451   308986   +122535     
  Branches    30634    50556    +19922     
===========================================
+ Hits       164826   241590    +76764     
- Misses      15602    60936    +45334     
- Partials     6023     6460      +437     

@@ -294,8 +295,9 @@ def process_message(buffer: RecordingBuffer, message: bytes) -> None:
else None
)

project = Project.objects.get_from_cache(decoded_message["project_id"])
Copy link
Member

Choose a reason for hiding this comment

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

How much memory will this consume?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the cache? I don't know. Got the inspiration from occurrence_consumer.py

Copy link
Member

Choose a reason for hiding this comment

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

we already are usually caching the project for other places in ingest, so it probably won't be very much additional if any

src/sentry/replays/usecases/ingest/dom_index.py Outdated Show resolved Hide resolved
src/sentry/replays/usecases/ingest/dom_index.py Outdated Show resolved Hide resolved
@aliu39 aliu39 enabled auto-merge (squash) September 19, 2024 21:00
@aliu39 aliu39 merged commit f68f0d0 into master Sep 20, 2024
49 of 50 checks passed
@aliu39 aliu39 deleted the aliu/reduce-dom-index-queries branch September 20, 2024 22:15
Copy link

sentry-io bot commented Sep 24, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Project.DoesNotExist: Project matching query does not exist. sentry.silo.base in override View Issue
  • ‼️ AttributeError: 'list' object has no attribute 'get' sentry.replays.usecases.ingest.dom_index in cre... View Issue
  • ‼️ OperationalError: OperationalError('server closed the connection unexpectedly\n\tThis probably means the server ter... sentry.models.options.project_option in reload_... View Issue

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants