Browse Source

Clean up symbolicate task scheduling (#53597)

We have observed (via metrics and transactions) two things that should
ideally have been impossible:

- Some code was still running through the
`JavaScriptStacktraceProcessor` and executing its SQL queries and
fetching its files
- Some non-JS Symbolicators were cleaning up sourcemap-caches, which
should never have been created on them in the first place.

It might be possible that these two issues would have happened due to
some confusion of how we were relying on the `processed_by_symbolicator`
flag to decise which processing to do.

Now the logic is as follows:
- An event comes in and depending on `platform` it might be routed to JS
or native symbolication.
- At the end of symbolication, based on the `task_kind`, the event might
be scheduled for a second *native* symbolication round.
- The queue/worker/symbolicator assignment is also based on `task_kind`,
which should be assigned in a single place, and updated in another place
when scheduling the second native symbolication round.
Arpad Borsos 1 year ago
parent
commit
e43d305e20

+ 0 - 8
src/sentry/lang/javascript/plugin.py

@@ -4,7 +4,6 @@ from sentry.utils.safe import get_path
 
 from .errorlocale import translate_exception
 from .errormapping import rewrite_exception
-from .processor import JavaScriptStacktraceProcessor
 
 
 # TODO: We still need `preprocess_event` tasks and the remaining, non-symbolication specific
@@ -42,10 +41,3 @@ class JavascriptPlugin(Plugin2):
         if data.get("platform") in ("javascript", "node"):
             return [preprocess_event]
         return []
-
-    def get_stacktrace_processors(self, data, stacktrace_infos, platforms, **kwargs):
-        if data.get("processed_by_symbolicator", False):
-            return []
-
-        if "javascript" in platforms or "node" in platforms:
-            return [JavaScriptStacktraceProcessor]

+ 1 - 6
src/sentry/lang/javascript/processing.py

@@ -1,5 +1,5 @@
 import logging
-from typing import Any, Callable, Dict, Optional
+from typing import Any, Dict
 from urllib.parse import unquote
 
 from sentry.debug_files.artifact_bundles import maybe_renew_artifact_bundles_from_processing
@@ -250,7 +250,6 @@ def process_js_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
     ]
 
     metrics.incr("sourcemaps.symbolicator.events")
-    data["processed_by_symbolicator"] = True
 
     if not any(stacktrace["frames"] for stacktrace in stacktraces):
         metrics.incr("sourcemaps.symbolicator.events.skipped")
@@ -307,7 +306,3 @@ def process_js_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
             }
 
     return data
-
-
-def get_js_symbolication_function(data: Any) -> Optional[Callable[[Symbolicator, Any], Any]]:
-    return process_js_stacktraces

+ 21 - 21
src/sentry/tasks/symbolication.py

@@ -10,6 +10,7 @@ from sentry import options
 from sentry.eventstore import processing
 from sentry.eventstore.processing.base import Event
 from sentry.killswitches import killswitch_matches_context
+from sentry.lang.javascript.processing import process_js_stacktraces
 from sentry.lang.native.symbolicator import RetrySymbolication, Symbolicator, SymbolicatorTaskKind
 from sentry.models import Organization, Project
 from sentry.processing import realtime_metrics
@@ -80,26 +81,21 @@ def should_demote_symbolication(project_id: int) -> bool:
             return False
 
 
+# This is f*** joke:
+# The `mock.patch` in `test_symbolication.py` will not work with a static import,
+# so we gotta import the function dynamically here. Great! Hooray!
+def get_native_symbolication_function(data: Any) -> Optional[Callable[[Symbolicator, Any], Any]]:
+    from sentry.lang.native.processing import get_native_symbolication_function
+
+    return get_native_symbolication_function(data)
+
+
 def get_symbolication_function(
-    data: Any, is_in_symbolicate_event: bool = False
+    data: Any,
 ) -> Tuple[bool, Optional[Callable[[Symbolicator, Any], Any]]]:
-    # for JS events that have *not* been processed yet:
-    if data["platform"] in ("javascript", "node") and not data.get(
-        "processed_by_symbolicator", False
-    ):
-        from sentry.lang.javascript.processing import (
-            get_js_symbolication_function,
-            process_js_stacktraces,
-        )
-
-        # if we are already in `symbolicate_event`, we do *not* want to sample events
-        if is_in_symbolicate_event:
-            return True, process_js_stacktraces
-        else:
-            return True, get_js_symbolication_function(data)
+    if data["platform"] in ("javascript", "node"):
+        return True, process_js_stacktraces
     else:
-        from sentry.lang.native.processing import get_native_symbolication_function
-
         return False, get_native_symbolication_function(data)
 
 
@@ -151,11 +147,11 @@ def _do_symbolicate_event(
             return
 
     def _continue_to_process_event(was_killswitched: bool = False) -> None:
-        # for JS events, we check `get_symbolication_function` again *after*
-        # symbolication, because maybe we need to feed it to another round of
+        # After JS processing, we check `get_native_symbolication_function`,
+        # because maybe we need to feed it to another round of
         # `symbolicate_event`, but for *native* that time.
         if not was_killswitched and task_kind.is_js:
-            _, symbolication_function = get_symbolication_function(data, True)
+            symbolication_function = get_native_symbolication_function(data)
             if symbolication_function:
                 submit_symbolicate(
                     task_kind=task_kind.with_js(False),
@@ -176,7 +172,11 @@ def _do_symbolicate_event(
             has_attachments=has_attachments,
         )
 
-    _, symbolication_function = get_symbolication_function(data, True)
+    if not task_kind.is_js:
+        symbolication_function = get_native_symbolication_function(data)
+    else:
+        symbolication_function = process_js_stacktraces
+
     symbolication_function_name = getattr(symbolication_function, "__name__", "none")
 
     if symbolication_function is None or killswitch_matches_context(