Browse Source

perf(codeowners): speed up codeowners evaluation by pre-munging data (#77301)

via the benchmarking script i added and some print timing statements
within, i determined that for a particular slow codeowners evaluation,
95% of the time was spent munging the stacktrace, not running rule
evaluations.

We previously munged every rule, and this is wasted work as it is the
same for every rule. This PR changes it so we pre-munge the stacktraces
before passing them into the rules.

We'll deploy this with an option to evaluate if any errors / speed
regressions occur.
Josh Ferge 6 months ago
parent
commit
1e4df7564f

+ 65 - 0
bin/benchmark_codeowners/benchmark

@@ -0,0 +1,65 @@
+#!/usr/bin/env python
+# isort: skip_file
+
+"""
+This script benchmarks the performance of issue owner assignment in Sentry.
+Usage: python benchmark_codeowners/benchmark <path_to_code_mapping_file> <path_to_event_data_file>
+"""
+from sentry.runner import configure
+
+configure()
+import sys
+import random
+import string
+import time
+from sentry.models.organization import Organization
+from sentry.models.projectownership import ProjectOwnership
+from sentry.models.project import Project
+from sentry.utils import json
+
+
+def main(code_mapping_file, event_data_file):
+    def get_code_mapping():
+        with open(code_mapping_file) as f:
+            return json.loads(f.read())
+
+    def get_event_data():
+        with open(event_data_file) as f:
+            return json.loads(f.read())
+
+    code_mapping = get_code_mapping()
+    # create an organization
+    org_id = random.randint(1, 1000000)
+    org_name = "".join(random.choices(string.ascii_letters + string.digits, k=10))
+    org_slug = "".join(random.choices(string.ascii_lowercase + string.digits, k=10))
+    org, _ = Organization.objects.get_or_create(name=org_name, slug=org_slug, id=org_id)
+
+    # create a project
+    project_id = random.randint(1, 1000000)
+    project_name = "".join(random.choices(string.ascii_letters + string.digits, k=10))
+    project_slug = "".join(random.choices(string.ascii_lowercase + string.digits, k=10))
+    project, _ = Project.objects.get_or_create(
+        name=project_name, slug=project_slug, id=project_id, organization_id=org.id
+    )
+
+    # create a projectownership
+    ProjectOwnership.objects.get_or_create(
+        project_id=project.id,
+        schema=code_mapping,
+    )
+
+    event_data = get_event_data()
+
+    start = time.time()
+    ProjectOwnership.get_issue_owners(project.id, event_data)
+    elapsed_time = time.time() - start
+    print(f"Time taken: {elapsed_time:.6f} seconds")  # noqa
+
+
+if __name__ == "__main__":
+    if len(sys.argv) != 3:
+        print(  # noqa
+            "Usage: python benchmark_codeowners/benchmark <path_to_code_mapping_file> <path_to_event_data_file>"
+        )
+        sys.exit(1)
+    main(sys.argv[1], sys.argv[2])

+ 7 - 3
src/sentry/models/projectownership.py

@@ -9,6 +9,7 @@ from django.db import models
 from django.db.models.signals import post_delete, post_save
 from django.utils import timezone
 
+from sentry import options  # noqa
 from sentry.backup.scopes import RelocationScope
 from sentry.db.models import Model, region_silo_model, sane_repr
 from sentry.db.models.fields import FlexibleForeignKey, JSONField
@@ -16,7 +17,7 @@ from sentry.eventstore.models import Event, GroupEvent
 from sentry.models.activity import Activity
 from sentry.models.group import Group
 from sentry.models.groupowner import OwnerRuleType
-from sentry.ownership.grammar import Rule, load_schema, resolve_actors
+from sentry.ownership.grammar import Matcher, Rule, load_schema, resolve_actors
 from sentry.types.activity import ActivityType
 from sentry.types.actor import Actor
 from sentry.utils import metrics
@@ -202,6 +203,7 @@ class ProjectOwnership(Model):
                 *hydrated_ownership_rules[::-1],
                 *hydrated_codeowners_rules[::-1],
             ]
+
             rules_with_owners = list(
                 filter(
                     lambda item: len(item[1]) > 0,
@@ -358,10 +360,12 @@ class ProjectOwnership(Model):
         data: Mapping[str, Any],
     ) -> Sequence[Rule]:
         rules = []
-
         if ownership.schema is not None:
+            munged_data = None
+            if options.get("ownership.munge_data_for_performance"):
+                munged_data = Matcher.munge_if_needed(data)
             for rule in load_schema(ownership.schema):
-                if rule.test(data):
+                if rule.test(data, munged_data):
                     rules.append(rule)
 
         return rules

+ 7 - 0
src/sentry/options/defaults.py

@@ -2706,3 +2706,10 @@ register(
     default=0.0,
     flags=FLAG_AUTOMATOR_MODIFIABLE,
 )
+
+register(
+    "ownership.munge_data_for_performance",
+    type=Bool,
+    default=False,
+    flags=FLAG_AUTOMATOR_MODIFIABLE,
+)

+ 36 - 2
src/sentry/ownership/grammar.py

@@ -87,8 +87,15 @@ class Rule(namedtuple("Rule", "matcher owners")):
     def load(cls, data: Mapping[str, Any]) -> Rule:
         return cls(Matcher.load(data["matcher"]), [Owner.load(o) for o in data["owners"]])
 
-    def test(self, data: Mapping[str, Any]) -> bool | Any:
-        return self.matcher.test(data)
+    def test(
+        self,
+        data: Mapping[str, Any],
+        munged_data: tuple[Sequence[Mapping[str, Any]], Sequence[str]] | None,
+    ) -> bool | Any:
+        if munged_data:
+            return self.matcher.test_with_munged(data, munged_data)
+        else:
+            return self.matcher.test(data)
 
 
 class Matcher(namedtuple("Matcher", "type pattern")):
@@ -130,6 +137,33 @@ class Matcher(namedtuple("Matcher", "type pattern")):
 
         return frames, keys
 
+    def test_with_munged(
+        self, data: PathSearchable, munged_data: tuple[Sequence[Mapping[str, Any]], Sequence[str]]
+    ) -> bool:
+        """
+        Temporary function to test pre-munging data performance in production. will remove
+        and combine with test if prod deployment goes well.
+        """
+        if self.type == URL:
+            return self.test_url(data)
+        elif self.type == PATH:
+            return self.test_frames(*munged_data)
+        elif self.type == MODULE:
+            return self.test_frames(find_stack_frames(data), ["module"])
+        elif self.type.startswith("tags."):
+            return self.test_tag(data)
+        elif self.type == CODEOWNERS:
+            return self.test_frames(
+                *munged_data,
+                # Codeowners has a slightly different syntax compared to issue owners
+                # As such we need to match it using gitignore logic.
+                # See syntax documentation here:
+                # https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/creating-a-repository-on-github/about-code-owners
+                match_frame_value_func=lambda val, pattern: bool(codeowners_match(val, pattern)),
+                match_frame_func=lambda frame: frame.get("in_app") is not False,
+            )
+        return False
+
     def test(self, data: PathSearchable) -> bool:
         if self.type == URL:
             return self.test_url(data)