Make fewer requests in heartbeat, handle exceptions (#783)

This commit adjusts the heartbeat so that we make fewer requests in it.

Before, this PR we'd make (n * 3) + 1 API calls, where n equals the number of configured projects:

    `"all_projects_are_visible"`: `1` API call
    `"all_projects_have_permissions"`: `n` API calls (in a threadpool)
    `"all_projects_components_exist"`: `n` API calls (not in a threadpool)
    `"all_projects_issue_types_exist"`: `n` API calls (not in a threadpool)

With 22 configured prod projects, that's 67 API calls.

Now, we make `n + 3` API calls:
    `"all_projects_are_visible"`: `1` API call
    "all_projects_have_permissions": `1` API call
    "all_project_custom_components_exist" (renamed): `n` API calls (in a threadpool)
    "all_projects_issue_types_exist": `1` API call

For a grand total of 25 API calls in prod.

This was done by using different API endpoints (or params on those endpoints) to fetch more results at once. It's annoying that Jira doesn't provide an API endpoint to fetch components by project, but we were luckily able to make an improvement

This PR also adds a little more exception handling to functions we call in the heartbeat

Author: grahamalama

jbi/services/bugzilla.py

@@ -53,8 +53,12 @@ def _call(self, verb, url, *args, **kwargs):
         # https://bmo.readthedocs.io/en/latest/api/core/v1/general.html?highlight=x-bugzilla-api-key#authentication
         headers = kwargs.setdefault("headers", {})
         headers.setdefault("x-bugzilla-api-key", self.api_key)
-        resp = self._client.request(verb, url, *args, **kwargs)
-        resp.raise_for_status()
+        try:
+            resp = self._client.request(verb, url, *args, **kwargs)
+            resp.raise_for_status()
+        except requests.HTTPError:
+            logger.exception("%s %s", verb, url)
+            raise
         parsed = resp.json()
         if parsed.get("error"):
             raise BugzillaClientError(parsed["message"])
@@ -136,37 +140,46 @@ def __init__(self, client: BugzillaClient) -> None:
     def check_health(self) -> ServiceHealth:
         """Check health for Bugzilla Service"""
         logged_in = self.client.logged_in()
-
-        # Check that all JBI webhooks are enabled in Bugzilla,
-        # and report disabled ones.
         all_webhooks_enabled = False
         if logged_in:
-            jbi_webhooks = self.client.list_webhooks()
-            all_webhooks_enabled = len(jbi_webhooks) > 0
-            for webhook in jbi_webhooks:
-                # Report errors in each webhook
-                statsd.gauge(
-                    f"jbi.bugzilla.webhooks.{webhook.slug}.errors", webhook.errors
-                )
-                # Warn developers when there are errors
-                if webhook.errors > 0:
-                    logger.warning(
-                        "Webhook %s has %s error(s)", webhook.name, webhook.errors
-                    )
-                if not webhook.enabled:
-                    all_webhooks_enabled = False
-                    logger.error(
-                        "Webhook %s is disabled (%s errors)",
-                        webhook.name,
-                        webhook.errors,
-                    )
+            all_webhooks_enabled = self._all_webhooks_enabled()
 
         health: ServiceHealth = {
             "up": logged_in,
             "all_webhooks_enabled": all_webhooks_enabled,
         }
         return health
 
+    def _all_webhooks_enabled(self):
+        # Check that all JBI webhooks are enabled in Bugzilla,
+        # and report disabled ones.
+
+        try:
+            jbi_webhooks = self.client.list_webhooks()
+        except (BugzillaClientError, requests.HTTPError):
+            return False
+
+        if len(jbi_webhooks) == 0:
+            logger.info("No webhooks enabled")
+            return True
+
+        for webhook in jbi_webhooks:
+            # Report errors in each webhook
+            statsd.gauge(f"jbi.bugzilla.webhooks.{webhook.slug}.errors", webhook.errors)
+            # Warn developers when there are errors
+            if webhook.errors > 0:
+                logger.warning(
+                    "Webhook %s has %s error(s)", webhook.name, webhook.errors
+                )
+            if not webhook.enabled:
+                logger.error(
+                    "Webhook %s is disabled (%s errors)",
+                    webhook.name,
+                    webhook.errors,
+                )
+                return False
+        return True
+
     def add_link_to_jira(self, context: ActionContext):
         """Add link to Jira in Bugzilla ticket"""
         bug = context.bug

jbi/services/jira.py

@@ -7,7 +7,7 @@
 # https://docs.python.org/3/whatsnew/3.11.html#pep-563-may-not-be-the-future
 from __future__ import annotations
 
-import concurrent.futures
+import concurrent
 import json
 import logging
 from functools import lru_cache
@@ -93,7 +93,6 @@ def raise_for_status(self, *args, **kwargs):
             raise
 
     get_server_info = instrumented_method(Jira.get_server_info)
-    get_permissions = instrumented_method(Jira.get_permissions)
     get_project_components = instrumented_method(Jira.get_project_components)
     projects = instrumented_method(Jira.projects)
     update_issue = instrumented_method(Jira.update_issue)
@@ -103,6 +102,19 @@ def raise_for_status(self, *args, **kwargs):
     create_issue = instrumented_method(Jira.create_issue)
     get_project = instrumented_method(Jira.get_project)
 
+    @instrumented_method
+    def permitted_projects(self, permissions: Iterable):
+        """Fetches projects that the user has the required permissions for
+
+        https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-permissions/#api-rest-api-2-permissions-project-post
+        """
+
+        response = self.post(
+            "/rest/api/2/permissions/project",
+            json={"permissions": list(JIRA_REQUIRED_PERMISSIONS)},
+        )
+        return response["projects"]
+
 
 class JiraService:
     """Used by action workflows to perform action-specific Jira tasks"""
@@ -124,16 +136,24 @@ def check_health(self, actions: Actions) -> ServiceHealth:
         health: ServiceHealth = {
             "up": is_up,
             "all_projects_are_visible": is_up and self._all_projects_visible(actions),
-            "all_projects_have_permissions": self._all_projects_permissions(actions),
-            "all_projects_components_exist": is_up
-            and self._all_projects_components_exist(actions),
+            "all_projects_have_permissions": is_up
+            and self._all_projects_permissions(actions),
+            "all_project_custom_components_exist": is_up
+            and self._all_project_custom_components_exist(actions),
             "all_projects_issue_types_exist": is_up
             and self._all_project_issue_types_exist(actions),
         }
         return health
 
     def _all_projects_visible(self, actions: Actions) -> bool:
-        visible_projects = {project["key"] for project in self.fetch_visible_projects()}
+        try:
+            visible_projects = {
+                project["key"] for project in self.fetch_visible_projects()
+            }
+        except requests.HTTPError:
+            logger.exception("Error fetching visible Jira projects")
+            return False
+
         missing_projects = actions.configured_jira_projects_keys - visible_projects
         if missing_projects:
             logger.error(
@@ -142,97 +162,96 @@ def _all_projects_visible(self, actions: Actions) -> bool:
             )
         return not missing_projects
 
-    def _all_projects_permissions(self, actions: Actions):
+    def _all_projects_permissions(self, actions: Actions) -> bool:
         """Fetches and validates that required permissions exist for the configured projects"""
-        all_projects_perms = self._fetch_project_permissions(actions)
-        return self._validate_permissions(all_projects_perms)
+        try:
+            projects = self.client.permitted_projects(JIRA_REQUIRED_PERMISSIONS)
+        except requests.HTTPError:
+            logger.exception(
+                "Encountered error when trying to fetch permitted projects"
+            )
+            return False
 
-    def _fetch_project_permissions(self, actions: Actions):
-        """Fetches permissions for the configured projects"""
+        projects_with_required_perms = {project["key"] for project in projects}
+        missing_perms = (
+            actions.configured_jira_projects_keys - projects_with_required_perms
+        )
+        if missing_perms:
+            logger.warning(
+                "Missing permissions for projects %s", ", ".join(missing_perms)
+            )
+            return False
 
-        all_projects_perms = {}
-        # Query permissions for all configured projects in parallel threads.
+        return True
+
+    def _all_project_custom_components_exist(self, actions: Actions):
         with concurrent.futures.ThreadPoolExecutor(max_workers=4) as executor:
-            futures_to_projects = {
-                executor.submit(
-                    self.client.get_permissions,
-                    project_key=project_key,
-                    permissions=",".join(JIRA_REQUIRED_PERMISSIONS),
-                ): project_key
-                for project_key in actions.configured_jira_projects_keys
+            futures = {
+                executor.submit(self._check_project_components, action): action
+                for action in actions
+                if action.parameters.jira_components.set_custom_components
             }
-            # Obtain futures' results unordered.
-            for future in concurrent.futures.as_completed(futures_to_projects):
-                project_key = futures_to_projects[future]
-                response = future.result()
-                all_projects_perms[project_key] = response["permissions"]
-        return all_projects_perms
-
-    def _validate_permissions(self, all_projects_perms):
-        """Validates permissions for the configured projects"""
-        misconfigured = []
-        for project_key, obtained_perms in all_projects_perms.items():
-            missing_required_perms = JIRA_REQUIRED_PERMISSIONS - set(
-                obtained_perms.keys()
-            )
-            not_given = set(
-                entry["key"]
-                for entry in obtained_perms.values()
-                if not entry["havePermission"]
+
+            success = True
+            for future in concurrent.futures.as_completed(futures):
+                success = success and future.result()
+        return success
+
+    def _check_project_components(self, action):
+        project_key = action.parameters.jira_project_key
+        specified_components = set(
+            action.parameters.jira_components.set_custom_components
+        )
+
+        try:
+            all_project_components = self.client.get_project_components(project_key)
+        except requests.HTTPError:
+            logger.exception("Error checking project components for %s", project_key)
+            return False
+
+        try:
+            all_components_names = set(comp["name"] for comp in all_project_components)
+        except KeyError:
+            logger.exception(
+                "Unexpected get_project_components response for %s",
+                action.whiteboard_tag,
             )
-            if missing_permissions := missing_required_perms.union(not_given):
-                misconfigured.append((project_key, missing_permissions))
-        for project_key, missing in misconfigured:
+            return False
+
+        unknown = specified_components - all_components_names
+        if unknown:
             logger.error(
-                "Configured credentials don't have permissions %s on Jira project %s",
-                ",".join(missing),
+                "Jira project %s does not have components %s",
                 project_key,
-                extra={
-                    "jira": {
-                        "project": project_key,
-                    }
-                },
+                unknown,
             )
-        return not misconfigured
-
-    def _all_projects_components_exist(self, actions: Actions):
-        components_by_project = {
-            action.parameters.jira_project_key: action.parameters.jira_components.set_custom_components
-            for action in actions
-        }
-        success = True
-        for project, specified_components in components_by_project.items():
-            all_project_components = self.client.get_project_components(project)
-            all_components_names = set(comp["name"] for comp in all_project_components)
-            unknown = set(specified_components) - all_components_names
-            if unknown:
-                logger.error(
-                    "Jira project %s does not have components %s",
-                    project,
-                    unknown,
-                )
-                success = False
+            return False
 
-        return success
+        return True
 
     def _all_project_issue_types_exist(self, actions: Actions):
+        projects = self.client.projects(included_archived=None, expand="issueTypes")
         issue_types_by_project = {
-            action.parameters.jira_project_key: set(
-                action.parameters.issue_type_map.values()
-            )
-            for action in actions
+            project["key"]: {issue_type["name"] for issue_type in project["issueTypes"]}
+            for project in projects
         }
-        success = True
-        for project, specified_issue_types in issue_types_by_project.items():
-            response = self.client.get_project(project)
-            all_issue_types_names = set(it["name"] for it in response["issueTypes"])
-            unknown = set(specified_issue_types) - all_issue_types_names
-            if unknown:
-                logger.error(
-                    "Jira project %s does not have issue type %s", project, unknown
-                )
-                success = False
-        return success
+        missing_issue_types_by_project = {}
+        for action in actions:
+            action_issue_types = set(action.parameters.issue_type_map.values())
+            project_issue_types = issue_types_by_project.get(
+                action.jira_project_key, set()
+            )
+            if missing_issue_types := action_issue_types - project_issue_types:
+                missing_issue_types_by_project[
+                    action.jira_project_key
+                ] = missing_issue_types
+        if missing_issue_types_by_project:
+            logger.warning(
+                "Jira projects with missing issue types",
+                extra=missing_issue_types_by_project,
+            )
+            return False
+        return True
 
     def get_issue(self, context: ActionContext, issue_key):
         """Return the Jira issue fields or `None` if not found."""

tests/unit/services/test_jira.py

@@ -113,31 +113,32 @@ def test_jira_does_not_retry_4XX(mocked_responses, context_create_example):
         (["Foo"], [], False),
     ],
 )
-def test_all_projects_components_exist(
-    action_factory,
+def test_all_project_custom_components_exist(
     jira_components,
     project_components,
     expected_result,
     mocked_responses,
+    action_factory,
 ):
     url = f"{get_settings().jira_base_url}rest/api/2/project/ABC/components"
-    mocked_responses.add(
-        responses.GET,
-        url,
-        json=project_components,
-    )
+    if jira_components:
+        mocked_responses.add(
+            responses.GET,
+            url,
+            json=project_components,
+        )
     action = action_factory(
         parameters={
             "jira_project_key": "ABC",
             "jira_components": JiraComponents(set_custom_components=jira_components),
         }
     )
     actions = Actions(root=[action])
-    result = jira.get_service()._all_projects_components_exist(actions)
+    result = jira.get_service()._all_project_custom_components_exist(actions)
     assert result is expected_result
 
 
-def test_all_projects_components_exist_no_components_param(
+def test_all_project_custom_components_exist_no_components_param(
     action_factory, mocked_responses
 ):
     action = action_factory(
@@ -146,13 +147,7 @@ def test_all_projects_components_exist_no_components_param(
         }
     )
     actions = Actions(root=[action])
-    url = f"{get_settings().jira_base_url}rest/api/2/project/ABC/components"
-    mocked_responses.add(
-        responses.GET,
-        url,
-        json=[],
-    )
-    result = jira.get_service()._all_projects_components_exist(actions)
+    result = jira.get_service()._all_project_custom_components_exist(actions)
     assert result is True