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

Bust Symbolicator list_files cache on uploads #53049

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

Swatinem
Copy link
Member

By attaching a random querystring parameter to the Sentry internal source, this will cause a cache miss in Symbolicators list_files cache, thus fetching and discovering the uploaded debug file immediately.

This is very similar to bump_reprocessing_revision, except it has a TTL. Also the code for bump_reprocessing_revision is overly complex for reasons I don’t understand so I don’t want to touch that.

I tested this with a local symbolicator that has caching turned on, see this comment:

# NOTE:
# When running this test against a local symbolicator instance,
# make sure that instance has its caches disabled. This test assumes
# that a symbol upload has immediate effect, whereas in reality the
# negative cache needs to expire first.

By attaching a random querystring parameter to the Sentry internal source, this will cause a cache miss in Symbolicators `list_files` cache, thus fetching and discovering the uploaded debug file immediately.
@Swatinem Swatinem requested a review from a team July 18, 2023 09:35
@Swatinem Swatinem self-assigned this Jul 18, 2023
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 18, 2023
@loewenheim
Copy link
Contributor

Does the altered URL have any effect on how e.g. object candidates are displayed? Or in other words, is this URL visible to users anywhere?

@Swatinem
Copy link
Member Author

Does the altered URL have any effect on how e.g. object candidates are displayed? Or in other words, is this URL visible to users anywhere?

I double-checked, and the URL is never exposed to users, and also not used for caching of downloaded files, but just for caching of list_files requests.

Downloaded file caches and candidates are only using the uri which is pretty much a constructed string containing the file_id.

@loewenheim
Copy link
Contributor

Nice. Then I would just like to request comments somewhere in the code explaining what this query parameter is for, so that we can still understand this in a few years 😅

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #53049 (b3d0208) into master (54a1d8f) will decrease coverage by 1.69%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #53049      +/-   ##
==========================================
- Coverage   79.47%   77.78%   -1.69%     
==========================================
  Files        4936     4934       -2     
  Lines      207488   207474      -14     
  Branches    35423    35429       +6     
==========================================
- Hits       164892   161379    -3513     
- Misses      37565    41058    +3493     
- Partials     5031     5037       +6     
Impacted Files Coverage Δ
src/sentry/lang/native/sources.py 77.91% <100.00%> (+1.26%) ⬆️
src/sentry/models/debugfile.py 70.57% <100.00%> (-1.27%) ⬇️
src/sentry/tasks/assemble.py 76.17% <100.00%> (-11.21%) ⬇️

... and 248 files with indirect coverage changes

@Swatinem Swatinem merged commit 99b7d19 into master Jul 18, 2023
@Swatinem Swatinem deleted the swatinem/cache-bust-dif-upload branch July 18, 2023 10:19
@Swatinem Swatinem added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Jul 18, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 6faec71

getsentry-bot added a commit that referenced this pull request Jul 18, 2023
This reverts commit 99b7d19.

Co-authored-by: Swatinem <580492+Swatinem@users.noreply.github.com>
@Swatinem
Copy link
Member Author

Reverting because the PR hardcodes the "default" redis cluster which fails on deploys

@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2023
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 Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants