fix: Stop exhausting GitHub API rate limits on busy orgs

README.md

@@ -57,6 +57,8 @@ Steps to follow:
 dsn = https://foo@bar.ingest.sentry.io/foobar
 ```
 
+   - **Note:** The app caches this file, so changes to it can take up to 10 minutes to apply.
+
 4. Install [this GitHub App](https://github.com/apps/sentry-github-app-alpha)
    1. **"All repositories"** - this is the easiest option; all repositories will have telemetry from GitHub Actions sent.
    2. **"Only select repositories"**

cli.py

@@ -49,7 +49,7 @@ def main() -> int:
     with GithubAppToken(**web_app.config.gh_app._asdict()).get_token(
         installation_id
     ) as token:
-        dsn = fetch_dsn_for_github_org(org, token)
+        dsn = fetch_dsn_for_github_org(org, token, installation_id)
         client = GithubClient(token=token, dsn=dsn)
         client.send_trace(job)
 

requirements.txt

@@ -1,3 +1,4 @@
+cachetools==5.3.2
 flask==2.3.3
 google-cloud-secret-manager==2.11.0
 PyJWT[crypto]==2.4.0

src/github_sdk.py

@@ -49,7 +49,6 @@ def _fetch_github(self, url):
     def _get_extra_metadata(self, job):
         # XXX: This is the slowest call
         runs = self._fetch_github(job["run_url"]).json()
-        workflow = self._fetch_github(runs["workflow_url"]).json()
         repo = runs["repository"]["full_name"]
         meta = {
             # "workflow_name": workflow["name"],
@@ -68,7 +67,10 @@ def _get_extra_metadata(self, job):
                 "run_attempt": runs["run_attempt"],  # Rerunning a job
                 "event": runs["event"],
                 # It allows querying jobs within the same workflow (e.g. foo.yml)
-                "workflow": workflow["path"].rsplit("/")[-1],
+                # The run has the path, so there's no need to fetch its workflow, which
+                # fails for required workflows (`workflow_url` 422s). Their path can end
+                # in a ref (e.g. `foo.yml@refs/heads/main`), which isn't part of the name
+                "workflow": runs["path"].split("@", 1)[0].rsplit("/")[-1],
             },
         }
         if runs.get("pull_requests"):

src/sentry_config.py

@@ -3,10 +3,11 @@
 import base64
 import logging
 import os
+import threading
 from configparser import ConfigParser
-from functools import lru_cache
 
 import requests
+from cachetools import TTLCache
 
 LOGGING_LEVEL = os.environ.get("LOGGING_LEVEL", logging.INFO)
 logger = logging.getLogger(__name__)
@@ -15,9 +16,27 @@
 SENTRY_CONFIG_API_URL = (
     "https://api.github.com/repos/{owner}/.sentry/contents/sentry_config.ini"
 )
+# Fetching the config on every webhook exhausts busy orgs' GitHub API rate limit
+DSN_CACHE_TTL_SECONDS = 10 * 60
+# Module globals rather than @cached, whose closure would put every cached DSN in the
+# local variables Sentry captures with an error
+_dsn_cache = TTLCache(maxsize=1024, ttl=DSN_CACHE_TTL_SECONDS)
+_dsn_cache_lock = threading.Lock()
 
 
-def fetch_dsn_for_github_org(org: str, token: str) -> str:
+def fetch_dsn_for_github_org(org: str, token: str, installation_id: int) -> str:
+    # Only serve a DSN to the installation whose token could read it
+    key = (installation_id, org)
+    with _dsn_cache_lock:
+        dsn = _dsn_cache.get(key)
+    if dsn is None:
+        dsn = _fetch_dsn(org, token)
+        with _dsn_cache_lock:
+            _dsn_cache[key] = dsn
+    return dsn
+
+
+def _fetch_dsn(org: str, token: str) -> str:
     # Using the GH app token allows fetching the file in a private repo
     headers = {
         "Accept": "application/vnd.github+json",

src/web_app_handler.py

@@ -29,6 +29,9 @@ def handle_event(self, data, headers):
             reason = "Event not supported."
         elif data["action"] != "completed":
             reason = "We cannot do anything with this workflow state."
+        elif data["workflow_job"]["conclusion"] == "skipped":
+            # These are not traced, so don't spend GitHub API calls on them
+            reason = "Skipped jobs are not traced."
         else:
             # For now, this simplifies testing
             if self.dry_run:
@@ -43,7 +46,7 @@ def handle_event(self, data, headers):
                     installation_id
                 ) as token:
                     # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment
-                    dsn = fetch_dsn_for_github_org(org, token)
+                    dsn = fetch_dsn_for_github_org(org, token, installation_id)
                     client = GithubClient(
                         token=token,
                         dsn=dsn,
@@ -52,7 +55,7 @@ def handle_event(self, data, headers):
                     client.send_trace(data["workflow_job"])
             else:
                 # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment
-                dsn = fetch_dsn_for_github_org(org, token)
+                dsn = fetch_dsn_for_github_org(org, token, installation_id)
                 client = GithubClient(
                     token=self.config.gh.token,
                     dsn=dsn,

tests/conftest.py

@@ -4,6 +4,13 @@
 
 import pytest
 
+from src import sentry_config
+
+
+@pytest.fixture(autouse=True)
+def clear_dsn_cache():
+    sentry_config._dsn_cache.clear()
+
 
 @pytest.fixture
 def jobA_job():
@@ -17,12 +24,6 @@ def jobA_runs():
         return json.load(f)
 
 
-@pytest.fixture
-def jobA_workflow():
-    with open("tests/fixtures/jobA/workflow.json") as f:
-        return json.load(f)
-
-
 @pytest.fixture
 def skipped_workflow():
     with open("tests/fixtures/skipped_workflow.json") as f:
@@ -65,9 +66,3 @@ def failure_job():
 def failure_runs():
     with open("tests/fixtures/failedJob/runs.json") as f:
         return json.load(f)
-
-
-@pytest.fixture
-def failure_workflow():
-    with open("tests/fixtures/failedJob/workflow.json") as f:
-        return json.load(f)

tests/fixtures/jobA/runs.json

@@ -4,6 +4,7 @@
   "node_id": "WFR_kwLOAA1TcM59c-PH",
   "head_branch": "ahmed/add-naming-layer",
   "head_sha": "fd976218a94d8a0cd203c711ea7dbe68c573ef4d",
+  "path": ".github/workflows/acceptance.yml",
   "run_number": 53142,
   "event": "pull_request",
   "status": "completed",

tests/test_github_sdk.py

@@ -66,7 +66,6 @@ def test_trace_generation(
     mock_get_uuid,
     jobA_job,
     jobA_runs,
-    jobA_workflow,
     jobA_trace,
     uuid_list,
 ):
@@ -75,10 +74,6 @@ def test_trace_generation(
         "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951",
         json=jobA_runs,
     )
-    responses.get(
-        "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556",
-        json=jobA_workflow,
-    )
     client = GithubClient(dsn=DSN, token=TOKEN)
     trace = client._generate_trace(jobA_job)
     assert trace == jobA_trace
@@ -91,16 +86,11 @@ def test_trace_generation(
 def test_trace_generation_with_failing_steps(
     failure_job,
     failure_runs,
-    failure_workflow,
 ):
     responses.get(
         failure_job["run_url"],
         json=failure_runs,
     )
-    responses.get(
-        failure_runs["workflow_url"],
-        json=failure_workflow,
-    )
 
     client = GithubClient(dsn=DSN, token=TOKEN)
     trace = client._generate_trace(failure_job)
@@ -110,25 +100,46 @@ def test_trace_generation_with_failing_steps(
     assert trace["tags"]["event"] == "push"
 
 
+@pytest.mark.parametrize(
+    "path",
+    [
+        ".github/workflows/acceptance.yml",
+        ".github/workflows/acceptance.yml@main",
+        "getsentry/.github/.github/workflows/acceptance.yml@refs/heads/main",
+        "getsentry/.github/.github/workflows/acceptance.yml@1595d4b6de6a9e9751fb270a41019ce507d4099e",
+    ],
+)
+@responses.activate
+def test_trace_generation_for_required_workflow(jobA_job, jobA_runs, path):
+    # Fetching a required workflow's `workflow_url` fails with a 422, and `responses`
+    # raises on any request that isn't registered
+    jobA_runs[
+        "workflow_url"
+    ] = "https://api.github.com/repos/getsentry/sentry/actions/required_workflows/1"
+    # A required workflow's path can name the repo it lives in and end in a ref
+    jobA_runs["path"] = path
+    responses.get(jobA_job["run_url"], json=jobA_runs)
+
+    trace = GithubClient(dsn=DSN, token=TOKEN)._generate_trace(jobA_job)
+
+    assert trace["tags"]["workflow"] == "acceptance.yml"
+    assert len(responses.calls) == 1
+
+
 @freeze_time()
 @responses.activate
 @patch("src.github_sdk.get_uuid")
 def test_send_trace(
     mock_get_uuid,
     jobA_job,
     jobA_runs,
-    jobA_workflow,
     uuid_list,
 ):
     mock_get_uuid.side_effect = uuid_list
     responses.get(
         "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951",
         json=jobA_runs,
     )
-    responses.get(
-        "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556",
-        json=jobA_workflow,
-    )
 
     responses.post("https://foo@random.ingest.sentry.io/api/bar/envelope/")
 

tests/test_sentry_config_file.py

@@ -1,8 +1,11 @@
 from __future__ import annotations
 
+import base64
 from unittest import TestCase
 
+import pytest
 import responses
+from requests import HTTPError
 
 from src.sentry_config import fetch_dsn_for_github_org
 from src.sentry_config import SENTRY_CONFIG_API_URL as api_url
@@ -30,6 +33,15 @@
 }
 org = "armenzg"
 token = "foo_token"
+installation_id = 1
+
+
+def config_file_meta(dsn: str) -> dict:
+    contents = f"[sentry-github-actions-app]\ndsn = {dsn}\n"
+    return {
+        **sentry_config_file_meta,
+        "content": base64.b64encode(contents.encode()).decode(),
+    }
 
 
 class TestSentryConfigCase(TestCase):
@@ -45,7 +57,54 @@ def setUp(self) -> None:
 
     @responses.activate
     def test_fetch_parse_sentry_config_file(self) -> None:
-        assert fetch_dsn_for_github_org(org, token) == expected_dsn
+        assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn
+
+    @responses.activate
+    def test_dsn_is_cached_per_installation_and_org(self) -> None:
+        other_org_dsn = "https://e0bb9a6e1c4f4d3c9b3a6c2d8f1e7a5b@o1.ingest.sentry.io/2"
+        other_org_api_url = api_url.replace("{owner}", "other_org")
+        responses.get(other_org_api_url, json=config_file_meta(other_org_dsn))
+
+        # Every webhook comes with a new token
+        assert fetch_dsn_for_github_org(org, "token_1", 1) == expected_dsn
+        assert fetch_dsn_for_github_org(org, "token_2", 1) == expected_dsn
+        assert fetch_dsn_for_github_org("other_org", "token_3", 2) == other_org_dsn
+        responses.assert_call_count(self.api_url, 1)
+        responses.assert_call_count(other_org_api_url, 1)
+
+        # Another installation has to be able to read the org's config itself
+        responses.replace(responses.GET, self.api_url, status=404)
+        with pytest.raises(HTTPError):
+            fetch_dsn_for_github_org(org, "token_4", 2)
+
+    @responses.activate
+    def test_failed_fetch_is_not_cached(self) -> None:
+        responses.replace(responses.GET, self.api_url, status=403)
+        with pytest.raises(HTTPError):
+            fetch_dsn_for_github_org(org, token, installation_id)
+
+        # e.g. the org's GitHub API rate limit has been reset
+        responses.replace(
+            responses.GET,
+            self.api_url,
+            json=sentry_config_file_meta,
+            status=200,
+        )
+        assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn
+
+    @responses.activate
+    def test_failed_fetch_does_not_expose_cached_dsns(self) -> None:
+        fetch_dsn_for_github_org(org, token, installation_id)
+        responses.get(api_url.replace("{owner}", "other_org"), status=403)
+
+        with pytest.raises(HTTPError) as excinfo:
+            fetch_dsn_for_github_org("other_org", token, 2)
+
+        # Sentry sends every frame's local variables along with an error
+        tb = excinfo.tb
+        while tb:
+            assert expected_dsn not in repr(tb.tb_frame.f_locals)
+            tb = tb.tb_next
 
     def test_fetch_private_repo(self) -> None:
         pass