ref: Move all symbolicator-related code in store.py to a separate file (NATIVE-259)

[relaxolotl]

aka reduce the number of angry markus "why is my review requested on this" comments on native PRs.

Ingest can stop being pulled in for reviews on PRs that do not
require their feedback or should not require their feedback.

[untitaker]

thank you!

[flub]

generally LGTM. I put this in NATIVE-259 if you want to link the ticket btw

[relaxolotl]

nts: figure out an answer to this, namely "is this ok?" and how to make it ok if it isn't

also probably good to think about the the line that has been drawn by splitting all of the symbolicator stuff out into its own file: are there other places where it's using internal store.py stuff? is there symbolication logic in store.py that could and should be moved out into a method in this file? what are the low hanging fruit for those?

[relaxolotl]

for now, the compromise is to rename _do_process_event to do_process_event and focus on just splitting the code in this PR. patching up some of the lines drawn by this split, such as using what is supposed to be an internal store.py function in symbolicate.py can come in a follow-up PR.

[relaxolotl]

@untitaker can you take a second look at this when you have time?

[untitaker]

there is one change to the task name attribute that really needs to be reverted and tested (probably manually), otherwise I believe this is good to go

[untitaker]

I previously said we need to import/re-export all of those new tasks from tasks/store.py, then retracted that comment because that's not how celery dispatch actually works. I mistakenly assumed that Celery works off of import paths (it can do that, it's just not how we set it up). However, if we rename those identifiers away from sentry.tasks.store to sentry.tasks.symbolication I think we have a problem again: In-flight symbolication tasks will fail to dispatch when spawned from old code and run with new codebase.

To test this:

1. check out master, do not start or have sentry devserver or sentry run worker running
2. open sentry shell, import one of the tasks from this module and spawn it (mytask.delay(...)) -- you don't really need to provide correct arguments, you just need to demonstrate whether celery finds the right code to execute, in any case you should not get an error in the shell
3. optional verification step: sentry queues list should list non-zero task count for the symbolication queue (depending on which task you picked). If it is zero, you either didn't spawn the task successfully (step 2 failed) or have some background process running (step 1 failed)
4. check out this branch
5. run sentry devserver --workers or run sentry run worker (doesn't matter)
6. that command will spew an error message that the task could not be imported

if you repeat this test without switching between branches, celery will either execute the task successfully or at least tell you that you spawned it with the wrong arguments (depending on what you passed into .delay()) -- that's why your tests pass

this matters because during deployment we have the same scenario: the code that spawns the celery task can be old while the worker that is supposed to process the task may already run on the new version

If I remember correctly preserving the old value for name= is enough. Having a way to define a legacy alias would be nice too. I think this can be easily patched into the instrumented_task handler, but I am not sure.

We investigated and talked about this in a call but didn't define clear action items afterwards and didn't write anything down (sorry)

[relaxolotl]

can confirm this explodes as described:

17:24:30 worker  | sentry/.venv/lib/python3.6/site-packages/celery/fixups/django.py:206: UserWarning: Using settings.DEBUG leads to a memory
17:24:30 worker  |             leak, never use this setting in production environments!
17:24:30 worker  |   leak, never use this setting in production environments!''')
17:24:31 worker  | Traceback (most recent call last):
17:24:31 worker  |   File "sentry/.venv/lib/python3.6/site-packages/celery/worker/consumer/consumer.py", line 562, in on_task_received
17:24:31 worker  |     strategy = strategies[type_]
17:24:31 worker  | KeyError: 'sentry.tasks.store.symbolicate_event_low_priority'
17:24:31 worker  | 17:24:31 [ERROR] celery.worker.consumer.consumer: Received unregistered task of type 'sentry.tasks.store.symbolicate_event_low_priority'.
17:24:31 worker  | The message has been ignored and discarded.
17:24:31 worker  | 
17:24:31 worker  | Did you remember to import the module containing this task?
17:24:31 worker  | Or maybe you're using relative imports?
17:24:31 worker  | 
17:24:31 worker  | Please see
17:24:31 worker  | http://docs.celeryq.org/en/latest/internals/protocol.html
17:24:31 worker  | for more information.

for now i'll keep the old names - will follow up with a second PR that explores adding in legacy names. the hardest part will be to figure out when to axe legacy names, but it's better than having to lose events every time a rename happens.

[flub]

I'm a strong -1 on renaming any of these tasks

[untitaker]

yes 👍

this is necessary because if celery is not instructed to import the module, it may be that the tasks cannot be discovered by their task name. the tests and probably local manual testing work regardless of this change because celery probably imports sentry.tasks.symbolication as a side effect of loading another module

[flub]

sentry/src/sentry/api/serializers/models/project.py

Line 44 in b576d6f

from sentry.tasks.store import should_demote_symbolication

also needs to be updated with this move

[relaxolotl]

since it's unlikely that somebody in the future will be reading all of the comments in a PR, i've added this in to explain why the naming scheme in this file continues to use tasks.store instead of tasks.symbolication.

[flub]

have we considered a different split instead?

• src/sentry/tasks/store/process.py
• src/sentry/tasks/store/symbolication.py

That would also make sense and avoid this kerfuffle as well plus paves the way for more structure in the whole store subtree of tasks.

(apologies to bring this up so late, but this isn't a complete drastic change i think)

[relaxolotl]

i can make this change so that the "store" bit isn't completely misleading - i'll continue the conversation in #29421 instead of here, just so this can be merged and consequently unblock a PR that depends on this.

[relaxolotl]

jk i'm not doing this that would require me to update imports in 22 files for the store.py rename

[relaxolotl]

@flub imports are now updated, and should be ready for a re-review.

did we test locally if events still get processed and symbolicated etc?

i think the conversation in #28973 (comment) covers this, and i'm keeping the original task names (and metrics names for consistency & discoverability) so things continue running smoothly as a result.