Remove JS A/B-verification code (#51972)

We were still sampling a very low number of events here, but noone has
been looking at any differences since quite a while

src/sentry/lang/javascript/plugin.py

@@ -44,9 +44,7 @@ class JavascriptPlugin(Plugin2):
         return []
 
     def get_stacktrace_processors(self, data, stacktrace_infos, platforms, **kwargs):
-        if data.get("processed_by_symbolicator", False) and not data.get(
-            "symbolicator_stacktraces"
-        ):
+        if data.get("processed_by_symbolicator", False):
             return []
 
         if "javascript" in platforms or "node" in platforms:

src/sentry/lang/javascript/processing.py

@@ -1,10 +1,7 @@
 import logging
 from typing import Any, Callable, Dict, Optional
 
-from sentry.lang.javascript.utils import (
-    do_sourcemaps_processing_ab_test,
-    should_use_symbolicator_for_sourcemaps,
-)
+from sentry.lang.javascript.utils import should_use_symbolicator_for_sourcemaps
 from sentry.lang.native.error import SymbolicationFailed, write_error
 from sentry.lang.native.symbolicator import Symbolicator
 from sentry.models import EventError, Project
@@ -240,11 +237,8 @@ def process_js_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
     if not _handle_response_status(data, response):
         return data
 
-    should_do_ab_test = do_sourcemaps_processing_ab_test()
-    symbolicator_stacktraces = []
-
     processing_errors = response.get("errors", [])
-    if len(processing_errors) > 0 and not should_do_ab_test:
+    if len(processing_errors) > 0:
         data.setdefault("errors", []).extend(map_symbolicator_process_js_errors(processing_errors))
 
     assert len(stacktraces) == len(response["stacktraces"]), (stacktraces, response)
@@ -271,20 +265,12 @@ def process_js_stacktraces(symbolicator: Symbolicator, data: Any) -> Any:
             merged_frame = _merge_frame(merged_context_frame, complete_frame)
             new_frames.append(merged_frame)
 
-        # NOTE: we do *not* write the symbolicated frames into `data` (via the `sinfo` indirection)
-        # but we rather write that to a different event property that we will use for A/B testing.
-        if should_do_ab_test:
-            symbolicator_stacktraces.append(new_frames)
-        else:
-            sinfo.stacktrace["frames"] = new_frames
-
-            if sinfo.container is not None:
-                sinfo.container["raw_stacktrace"] = {
-                    "frames": new_raw_frames,
-                }
+        sinfo.stacktrace["frames"] = new_frames
 
-    if should_do_ab_test:
-        data["symbolicator_stacktraces"] = symbolicator_stacktraces
+        if sinfo.container is not None:
+            sinfo.container["raw_stacktrace"] = {
+                "frames": new_raw_frames,
+            }
 
     return data
 

src/sentry/lang/javascript/processor.py

@@ -2140,109 +2140,6 @@ class JavaScriptStacktraceProcessor(StacktraceProcessor):
                 "sourcemaps.processed", amount=len(self.sourcemaps_touched), skip_internal=True
             )
 
-        # If we do some A/B testing for symbolicator, we want to compare the stack traces
-        # processed by both the existing processor (this one), and the symbolicator result,
-        # and log any differences.
-        # Q: what do we want to diff? raw/_stacktraces? With the full source context?
-        # Processing Errors?
-        # We also need to account for known differences? Like symbolicator not
-        # outputting a trailing empty line, whereas the python processor does.
-        if symbolicator_stacktraces := self.data.pop("symbolicator_stacktraces", None):
-
-            metrics.incr("sourcemaps.ab-test.performed")
-
-            interesting_keys = {
-                "abs_path",
-                "filename",
-                "lineno",
-                "colno",
-                "function",
-                "context_line",
-                "module",
-                "in_app",
-            }
-            known_diffs = {
-                "context_line",
-                "data.sourcemap",
-            }
-
-            def filtered_frame(frame: dict) -> dict:
-                new_frame = {key: value for key, value in frame.items() if key in interesting_keys}
-
-                ds = get_path(frame, "data", "sourcemap")
-                # The python code does some trimming of the `data.sourcemap` prop to
-                # 150 characters with a trailing `...`, so replicate this here to avoid some
-                # bogus differences
-                if ds is not None and len(ds) > 150:
-                    ds = ds[:147] + "..."
-                new_frame["data.sourcemap"] = ds
-
-                # The code in `get_event_preprocessors` backfills the `module`.
-                # Contrary to its name, this runs *after* the stacktrace processor,
-                # so we want to backfill this here for all the frames as well:
-                abs_path = new_frame.get("abs_path")
-                if (
-                    new_frame.get("module") is None
-                    and abs_path
-                    and abs_path.startswith(("http:", "https:", "webpack:", "app:"))
-                ):
-                    new_frame["module"] = generate_module(abs_path)
-
-                return new_frame
-
-            def without_known_diffs(frame: dict) -> dict:
-                {key: value for key, value in frame.items() if key not in known_diffs}
-
-            different_frames = []
-            for symbolicator_stacktrace, stacktrace_info in zip(
-                symbolicator_stacktraces,
-                (
-                    sinfo
-                    for sinfo in self.stacktrace_infos
-                    # only include `stacktrace_infos` that have a stacktrace with frames
-                    if get_path(sinfo.container, "stacktrace", "frames", filter=True)
-                ),
-            ):
-                python_stacktrace = stacktrace_info.container.get("stacktrace")
-
-                for symbolicator_frame, python_frame in zip(
-                    symbolicator_stacktrace,
-                    python_stacktrace["frames"],
-                ):
-                    symbolicator_frame = filtered_frame(symbolicator_frame)
-                    python_frame = filtered_frame(python_frame)
-
-                    if symbolicator_frame != python_frame:
-                        # skip some insignificant differences
-                        if without_known_diffs(symbolicator_frame) == without_known_diffs(
-                            python_frame
-                        ):
-                            # python emits a `debug-id://` prefix whereas symbolicator does not
-                            # OR: python adds a `data.sourcemap` even though it could not be resolved
-                            if (python_frame.get("data.sourcemap") or "").startswith(
-                                "debug-id://"
-                            ) or symbolicator_frame.get("data.sourcemap") is None:
-                                continue
-
-                            # with minified files and high column numbers, we might have a difference in
-                            # source context slicing, probably due to encodings
-                            if python_frame.get("colno", 0) > 10_000 and python_frame.get(
-                                "context_line"
-                            ) != symbolicator_frame.get("context_line"):
-                                continue
-
-                        different_frames.append(
-                            {"symbolicator": symbolicator_frame, "python": python_frame}
-                        )
-
-            if different_frames:
-                with sentry_sdk.push_scope() as scope:
-                    scope.set_extra("different_frames", different_frames)
-                    scope.set_extra("event_id", self.data.get("event_id"))
-                    sentry_sdk.capture_message(
-                        "JS symbolication differences between symbolicator and python."
-                    )
-
     def suspected_console_errors(self, frames):
         def is_suspicious_frame(frame) -> bool:
             function = frame.get("function", None)

src/sentry/lang/javascript/utils.py

@@ -4,7 +4,6 @@ from sentry import options
 
 SYMBOLICATOR_SOURCEMAPS_PROJECTS_OPTION = "symbolicator.sourcemaps-processing-projects"
 SYMBOLICATOR_SOURCEMAPS_SAMPLE_RATE_OPTION = "symbolicator.sourcemaps-processing-sample-rate"
-SYMBOLICATOR_SOURCEMAPS_AB_TEST_OPTION = "symbolicator.sourcemaps-processing-ab-test"
 
 
 def should_use_symbolicator_for_sourcemaps(project_id: int) -> bool:
@@ -16,7 +15,3 @@ def should_use_symbolicator_for_sourcemaps(project_id: int) -> bool:
         return True
 
     return options.get(SYMBOLICATOR_SOURCEMAPS_SAMPLE_RATE_OPTION) > random.random()
-
-
-def do_sourcemaps_processing_ab_test() -> bool:
-    return options.get(SYMBOLICATOR_SOURCEMAPS_AB_TEST_OPTION) > random.random()

src/sentry/options/defaults.py

@@ -566,8 +566,6 @@ register(
 register(
     "symbolicator.sourcemaps-processing-sample-rate", default=0.0, flags=FLAG_AUTOMATOR_MODIFIABLE
 )
-# Use a fraction of Symbolicator Source Maps processing events for A/B testing.
-register("symbolicator.sourcemaps-processing-ab-test", default=0.0, flags=FLAG_AUTOMATOR_MODIFIABLE)
 
 # Normalization after processors
 register("store.normalize-after-processing", default=0.0, flags=FLAG_AUTOMATOR_MODIFIABLE)  # unused

tests/relay_integration/lang/javascript/test_plugin.py

@@ -21,7 +21,6 @@ from sentry.models import (
 )
 from sentry.models.releasefile import update_artifact_index
 from sentry.testutils import RelayStoreHelper
-from sentry.testutils.helpers import override_options
 from sentry.testutils.helpers.datetime import before_now, iso_format
 from sentry.testutils.skips import requires_symbolicator
 from sentry.utils import json
@@ -645,95 +644,6 @@ class TestJavascriptIntegration(RelayStoreHelper):
         assert second_frame.pre_context == first_frame.pre_context
         assert second_frame.post_context == first_frame.post_context
 
-    @requires_symbolicator
-    @pytest.mark.symbolicator
-    def test_sourcemap_source_expansion_ab_test(self, process_with_symbolicator):
-        self.project.update_option("sentry:scrape_javascript", False)
-        release = Release.objects.create(
-            organization_id=self.project.organization_id, version="abc"
-        )
-        release.add_project(self.project)
-
-        for file in ["file.min.js", "file1.js", "file2.js", "file.sourcemap.js"]:
-            with open(get_fixture_path(file), "rb") as f:
-                f1 = File.objects.create(
-                    name=file,
-                    type="release.file",
-                    headers={},
-                )
-                f1.putfile(f)
-
-            ReleaseFile.objects.create(
-                name=f"http://example.com/{f1.name}",
-                release_id=release.id,
-                organization_id=self.project.organization_id,
-                file=f1,
-            )
-
-        data = {
-            "timestamp": self.min_ago,
-            "message": "hello",
-            "platform": "javascript",
-            "release": "abc",
-            "exception": {
-                "values": [
-                    {
-                        "type": "Error",
-                        "stacktrace": {
-                            "frames": [
-                                {
-                                    "abs_path": "http://example.com/file.min.js",
-                                    "filename": "file.min.js",
-                                    "lineno": 1,
-                                    "colno": 39,
-                                },
-                                # NOTE: Intentionally source is not retrieved from this HTML file
-                                {
-                                    "function": 'function: "HTMLDocument.<anonymous>"',
-                                    "abs_path": "http//example.com/index.html",
-                                    "filename": "index.html",
-                                    "lineno": 283,
-                                    "colno": 17,
-                                    "in_app": False,
-                                },
-                            ]
-                        },
-                    }
-                ]
-            },
-        }
-
-        with override_options({"symbolicator.sourcemaps-processing-ab-test": 1.0}):
-            event = self.post_and_retrieve_event(data)
-
-        assert event.data["errors"] == [
-            {"type": "js_no_source", "url": "http//example.com/index.html"}
-        ]
-
-        exception = event.interfaces["exception"]
-        frame_list = exception.values[0].stacktrace.frames
-
-        frame = frame_list[0]
-        assert frame.pre_context == ["function add(a, b) {", '\t"use strict";']
-        expected = "\treturn a + b; // fôo"
-        assert frame.context_line == expected
-        assert frame.post_context == ["}", ""]
-
-        raw_frame_list = exception.values[0].raw_stacktrace.frames
-        raw_frame = raw_frame_list[0]
-        assert not raw_frame.pre_context
-        assert (
-            raw_frame.context_line
-            == 'function add(a,b){"use strict";return a+b}function multiply(a,b){"use strict";return a*b}function '
-            'divide(a,b){"use strict";try{return multip {snip}'
-        )
-        assert raw_frame.post_context == ["//@ sourceMappingURL=file.sourcemap.js", ""]
-        assert raw_frame.lineno == 1
-
-        # Since we couldn't expand source for the 2nd frame, both
-        # its raw and original form should be identical
-        assert raw_frame_list[1] == frame_list[1]
-
     @requires_symbolicator
     @pytest.mark.symbolicator
     def test_sourcemap_embedded_source_expansion(self, process_with_symbolicator):