feat(symbolication): Make frame order explicit (#103638)

This adds an enum `FrameOrder` with variants
`callee_first` and `caller_first` to distinguish two common ways stack
frames are stored, processed, and displayed (innermost frame at the top
and at the bottom, respectively). This enum is now passed to all
stacktrace-processing methods Symbolicator methods. Since Symbolicator
does not (yet) read the parameter, there are no functional changes.

The intention of this is to clarify in which order frames are sent to
Symbolicator, which currently differs between platforms because we never
properly documented or enforced this. Once Symbolicator becomes able to
process the new parameter, there may also be opportunities for
simplification.

As a somewhat related change, this also adjusts some JS tests so they
sort the "scraping_attempts" returned by Symbolicator before checking
them. This is sound because there is no intended order of scraping
attempts anyway, and makes sure tests pass once Symbolicator is allowed
to process frames in a different order.

Fixes INGEST-640.

Author: loewenheim

src/sentry/lang/java/processing.py

@@ -6,7 +6,7 @@
 from sentry.lang.java.utils import JAVA_PLATFORMS, get_jvm_images, get_proguard_images
 from sentry.lang.java.view_hierarchies import ViewHierarchies
 from sentry.lang.native.error import SymbolicationFailed, write_error
-from sentry.lang.native.symbolicator import Symbolicator
+from sentry.lang.native.symbolicator import FrameOrder, Symbolicator
 from sentry.models.eventerror import EventError
 from sentry.models.project import Project
 from sentry.models.release import Release
@@ -220,6 +220,9 @@ def process_jvm_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
         modules=modules,
         release_package=release_package,
         classes=window_class_names + exception_class_names,
+        # We are sending frames in the same order in which
+        # they were stored in the event, so this has to be "caller_first".
+        frame_order=FrameOrder.caller_first,
     )
 
     if not _handle_response_status(data, response):

src/sentry/lang/javascript/processing.py

@@ -5,7 +5,7 @@
 from sentry.debug_files.artifact_bundles import maybe_renew_artifact_bundles_from_processing
 from sentry.lang.javascript.utils import JAVASCRIPT_PLATFORMS
 from sentry.lang.native.error import SymbolicationFailed, write_error
-from sentry.lang.native.symbolicator import Symbolicator
+from sentry.lang.native.symbolicator import FrameOrder, Symbolicator
 from sentry.models.eventerror import EventError
 from sentry.stacktraces.processing import find_stacktraces_in_data
 from sentry.utils import metrics
@@ -252,6 +252,9 @@ def process_js_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
         modules=modules,
         release=data.get("release"),
         dist=data.get("dist"),
+        # We are sending frames in the same order in which
+        # they were stored in the event, so this has to be "caller_first".
+        frame_order=FrameOrder.caller_first,
     )
 
     if not _handle_response_status(data, response):

src/sentry/lang/native/processing.py

@@ -11,7 +11,7 @@
 
 from sentry import options
 from sentry.lang.native.error import SymbolicationFailed, write_error
-from sentry.lang.native.symbolicator import Symbolicator
+from sentry.lang.native.symbolicator import FrameOrder, Symbolicator
 from sentry.lang.native.utils import (
     get_event_attachment,
     get_os_from_event,
@@ -471,7 +471,13 @@ def process_native_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
 
     metrics.incr("process.native.symbolicate.request")
     response = symbolicator.process_payload(
-        platform=data.get("platform"), stacktraces=stacktraces, modules=modules, signal=signal
+        platform=data.get("platform"),
+        stacktraces=stacktraces,
+        modules=modules,
+        # Frames were reversed in `get_frames_for_symbolication`,
+        # so this has to be "callee_first".
+        frame_order=FrameOrder.callee_first,
+        signal=signal,
     )
 
     if not _handle_response_status(data, response):

src/sentry/lang/native/symbolicator.py

@@ -47,6 +47,18 @@ class SymbolicatorPlatform(Enum):
     native = "native"
 
 
+class FrameOrder(Enum):
+    """The order in which stack frames are sent to
+    and returned from Symbolicator."""
+
+    # Caller frames come before callee frames. This is the
+    # order in which stack frames are stored in events.
+    caller_first = "caller_first"
+    # Callee frames come before caller frames. This is the
+    # order in which stack frames are usually displayed.
+    callee_first = "callee_first"
+
+
 @dataclass(frozen=True)
 class SymbolicatorTaskKind:
     """Bundles information about a symbolication task:
@@ -261,8 +273,20 @@ def process_applecrashreport(self, platform: str, report: CachedAttachment):
         return process_response(res)
 
     def process_payload(
-        self, platform, stacktraces, modules, signal=None, apply_source_context=True
+        self, platform, stacktraces, modules, frame_order, signal=None, apply_source_context=True
     ):
+        """
+        Process a native event by symbolicating its frames.
+
+        :param platform: The event's platform. This should be either unset or one of "objc", "cocoa", "swift", "native", "c", "csharp".
+        :param stacktraces: The event's stacktraces. Frames must contain an `instruction_address`.
+                            Frames are expected to be ordered according to the frame_order (see below).
+        :param modules: ProGuard modules and source bundles. They must contain a `uuid` and have a
+                        `type` of either "proguard" or "source".
+        :param frame_order: The order of frames within stacktraces. See the documentation of `FrameOrder`.
+        :param signal: A numeric crash signal value. This is optional.
+        :param apply_source_context: Whether to add source context to frames.
+        """
         (sources, process_response) = sources_for_symbolication(self.project)
         scraping_config = get_scraping_config(self.project)
         json = {
@@ -271,6 +295,7 @@ def process_payload(
             "options": {
                 "dif_candidates": True,
                 "apply_source_context": apply_source_context,
+                "frame_order": frame_order.value,
             },
             "stacktraces": stacktraces,
             "modules": modules,
@@ -283,7 +308,22 @@ def process_payload(
         res = self._process("symbolicate_stacktraces", "symbolicate", json=json)
         return process_response(res)
 
-    def process_js(self, platform, stacktraces, modules, release, dist, apply_source_context=True):
+    def process_js(
+        self, platform, stacktraces, modules, release, dist, frame_order, apply_source_context=True
+    ):
+        """
+        Process a JS event by remapping its frames with sourcemaps.
+
+        :param platform: The event's platform. This should be unset, "javascript", or "node".
+        :param stacktraces: The event's stacktraces. Frames must contain a `function` and a `module`.
+                            Frames are expected to be ordered according to the frame_order (see below).
+        :param modules: Minified source files/sourcemaps Thy must contain a `type` field with value "sourcemap",
+                        a `code_file`, and a `debug_id`.
+        :param release: The event's release.
+        :param dist: The event's dist.
+        :param frame_order: The order of frames within stacktraces. See the documentation of `FrameOrder`.
+        :param apply_source_context: Whether to add source context to frames.
+        """
         source = get_internal_artifact_lookup_source(self.project)
         scraping_config = get_scraping_config(self.project)
 
@@ -292,7 +332,10 @@ def process_js(self, platform, stacktraces, modules, release, dist, apply_source
             "source": source,
             "stacktraces": stacktraces,
             "modules": modules,
-            "options": {"apply_source_context": apply_source_context},
+            "options": {
+                "apply_source_context": apply_source_context,
+                "frame_order": frame_order.value,
+            },
             "scraping": scraping_config,
         }
 
@@ -311,6 +354,7 @@ def process_jvm(
         modules,
         release_package,
         classes,
+        frame_order,
         apply_source_context=True,
     ):
         """
@@ -320,10 +364,12 @@ def process_jvm(
         :param platform: The event's platform. This should be either unset or "java".
         :param exceptions: The event's exceptions. These must contain a `type` and a `module`.
         :param stacktraces: The event's stacktraces. Frames must contain a `function` and a `module`.
+                            Frames are expected to be ordered according to the frame_order (see below).
         :param modules: ProGuard modules and source bundles. They must contain a `uuid` and have a
                         `type` of either "proguard" or "source".
         :param release_package: The name of the release's package. This is optional.
                                 Used for determining whether frames are in-app.
+        :param frame_order: The order of frames within stacktraces. See the documentation of `FrameOrder`.
         :param apply_source_context: Whether to add source context to frames.
         """
         source = get_internal_source(self.project)
@@ -335,7 +381,10 @@ def process_jvm(
             "stacktraces": stacktraces,
             "modules": modules,
             "classes": classes,
-            "options": {"apply_source_context": apply_source_context},
+            "options": {
+                "apply_source_context": apply_source_context,
+                "frame_order": frame_order.value,
+            },
         }
 
         if release_package is not None:

src/sentry/profiles/task.py

@@ -29,7 +29,12 @@
 from sentry.constants import DataCategory
 from sentry.lang.javascript.processing import _handles_frame as is_valid_javascript_frame
 from sentry.lang.native.processing import _merge_image
-from sentry.lang.native.symbolicator import Symbolicator, SymbolicatorPlatform, SymbolicatorTaskKind
+from sentry.lang.native.symbolicator import (
+    FrameOrder,
+    Symbolicator,
+    SymbolicatorPlatform,
+    SymbolicatorTaskKind,
+)
 from sentry.lang.native.utils import native_images_from_data
 from sentry.models.eventerror import EventError
 from sentry.models.files.utils import get_profiles_storage
@@ -424,6 +429,9 @@ def _symbolicate_profile(profile: Profile, project: Project) -> bool:
                     profile=profile,
                     modules=raw_modules,
                     stacktraces=raw_stacktraces,
+                    # Frames in a profile aren't inherently ordered,
+                    # but returned inlinees should be ordered callee first.
+                    frame_order=FrameOrder.callee_first,
                     platform=platform,
                 )
 
@@ -599,6 +607,7 @@ def symbolicate(
     profile: Profile,
     modules: list[Any],
     stacktraces: list[Any],
+    frame_order: FrameOrder,
     platform: str,
 ) -> Any:
     if platform in SHOULD_SYMBOLICATE_JS:
@@ -608,6 +617,7 @@ def symbolicate(
             modules=modules,
             release=profile.get("release"),
             dist=profile.get("dist"),
+            frame_order=frame_order,
             apply_source_context=False,
         )
     elif platform == "android":
@@ -617,13 +627,15 @@ def symbolicate(
             stacktraces=stacktraces,
             modules=modules,
             release_package=profile.get("transaction_metadata", {}).get("app.identifier"),
+            frame_order=frame_order,
             apply_source_context=False,
             classes=[],
         )
     return symbolicator.process_payload(
         platform=platform,
         stacktraces=stacktraces,
         modules=modules,
+        frame_order=frame_order,
         apply_source_context=False,
     )
 
@@ -638,6 +650,7 @@ def run_symbolicate(
     profile: Profile,
     modules: list[Any],
     stacktraces: list[Any],
+    frame_order: FrameOrder,
     platform: str,
 ) -> tuple[list[Any], list[Any], bool]:
     symbolication_start_time = time()
@@ -665,6 +678,7 @@ def on_symbolicator_request() -> None:
                 profile=profile,
                 stacktraces=stacktraces,
                 modules=modules,
+                frame_order=frame_order,
                 platform=platform,
             )
 
@@ -943,6 +957,9 @@ def on_symbolicator_request() -> None:
                         )
                     },
                 ],
+                # Methods in a profile aren't inherently ordered, but the order of returned
+                # inlinees should be caller first.
+                frame_order=FrameOrder.caller_first,
                 platform=profile["platform"],
             )
             if response:

tests/relay_integration/lang/javascript/test_plugin.py

@@ -354,7 +354,10 @@ def test_sourcemap_source_expansion(self) -> None:
             }
         ]
 
-        assert event.data["scraping_attempts"] == [
+        scraping_attempts = sorted(
+            event.data["scraping_attempts"], key=lambda attempt: attempt["url"]
+        )
+        assert scraping_attempts == [
             {"status": "not_attempted", "url": "http://example.com/file.min.js"},
             {"status": "not_attempted", "url": "http://example.com/file.sourcemap.js"},
             {"status": "not_attempted", "url": "http://example.com/file1.js"},
@@ -461,7 +464,10 @@ def test_sourcemap_webpack(self) -> None:
         exception = event.interfaces["exception"]
         frame_list = exception.values[0].stacktrace.frames
 
-        assert event.data["scraping_attempts"] == [
+        scraping_attempts = sorted(
+            event.data["scraping_attempts"], key=lambda attempt: attempt["url"]
+        )
+        assert scraping_attempts == [
             {"url": "http://example.com/webpack1.min.js", "status": "not_attempted"},
             {"url": "http://example.com/webpack1.min.js.map", "status": "not_attempted"},
             {"url": "http://example.com/webpack2.min.js", "status": "not_attempted"},
@@ -564,7 +570,10 @@ def test_sourcemap_embedded_source_expansion(self) -> None:
             }
         ]
 
-        assert event.data["scraping_attempts"] == [
+        scraping_attempts = sorted(
+            event.data["scraping_attempts"], key=lambda attempt: attempt["url"]
+        )
+        assert scraping_attempts == [
             {"status": "not_attempted", "url": "http://example.com/embedded.js"},
             {"status": "not_attempted", "url": "http://example.com/embedded.js.map"},
         ]
@@ -634,7 +643,10 @@ def test_sourcemap_nofiles_source_expansion(self) -> None:
 
         assert "errors" not in event.data
 
-        assert event.data["scraping_attempts"] == [
+        scraping_attempts = sorted(
+            event.data["scraping_attempts"], key=lambda attempt: attempt["url"]
+        )
+        assert scraping_attempts == [
             {"url": "app:///nofiles.js", "status": "not_attempted"},
             {"url": "app:///nofiles.js.map", "status": "not_attempted"},
         ]
@@ -710,11 +722,14 @@ def test_indexed_sourcemap_source_expansion(self) -> None:
 
         assert "errors" not in event.data
 
-        assert event.data["scraping_attempts"] == [
-            {"status": "not_attempted", "url": "http://example.com/indexed.min.js"},
-            {"status": "not_attempted", "url": "http://example.com/indexed.sourcemap.js"},
+        scraping_attempts = sorted(
+            event.data["scraping_attempts"], key=lambda attempt: attempt["url"]
+        )
+        assert scraping_attempts == [
             {"status": "not_attempted", "url": "http://example.com/file1.js"},
             {"status": "not_attempted", "url": "http://example.com/file2.js"},
+            {"status": "not_attempted", "url": "http://example.com/indexed.min.js"},
+            {"status": "not_attempted", "url": "http://example.com/indexed.sourcemap.js"},
         ]
 
         exception = event.interfaces["exception"]