fix(derived_code_mappings): Do not abort if a repo fails to process #44126

armenzg · 2023-02-03T19:55:13Z

This fixes an issue where we would give up processing as soon as any Github API error would be returned (to the exception of some defined cases).

This also moves handling of errors to the caller in order to have a centralized processing.

This needs to land after #44110.

It seems that we raise unidentified API errors in get_tree. get_cached_repo_files does not catch it. The iteration of the repositories happens within the try/block, thus, failing to work as soon as one repo fails to process.

armenzg · 2023-02-03T20:02:58Z

src/sentry/integrations/github/client.py

-                logger.warning(f"The Github App does not have access to {repo_full_name}.")
-            else:
-                # Raise it so we stop hammering the API
-                raise error


In here we raise any non-identified error and this would cause the processing of all repos to abort.

armenzg · 2023-02-03T20:27:10Z

src/sentry/integrations/github/client.py

If you use "Hide whitespace" this PR is easier to review.

armenzg · 2023-02-03T20:28:10Z

src/sentry/integrations/github/client.py

Removing this except block and letting _populate_trees handle it.

sentry/src/sentry/integrations/github/client.py

Lines 231 to 241 in 28b4d56

try:

# The Github API rate limit is reset every hour

# Spread the expiration of the cache of each repo across the day

trees[repo_full_name] = self._populate_tree(

repo_info, only_use_cache, (3600 * 24) + (3600 * (index % 24))

)

except ApiError as error:

process_error(error, extra)

except Exception:

# Report for investigatagiation but do not stop processing

logger.exception(

armenzg · 2023-02-03T20:30:05Z

src/sentry/integrations/github/client.py

Removing this except block and letting the derived tasks handle it over there:
https://github.com/getsentry/sentry/blob/67327c3e2b5b59e7afc61bc26330ce987202b945/src/sentry/tasks/derive_code_mappings.py#L76-L90

armenzg · 2023-02-03T20:34:28Z

src/sentry/integrations/github/client.py

This block basically comes from get_tree. We're now handling in here to allow keep moving forward with processing repositories rather than aborting.

armenzg · 2023-02-03T20:36:47Z

src/sentry/integrations/github/client.py

If a call to _populate_tree raises an error, we will now report to Sentry and not abort the processing.

armenzg · 2023-02-03T20:38:01Z

src/sentry/tasks/derive_code_mappings.py

We now do a catch all in the new except block.

armenzg · 2023-02-03T20:49:24Z

tests/sentry/integrations/github/test_integration.py

        self.set_rate_limit(MINIMUM_REQUESTS + 50)
        expected_trees = {
-            "Test-Organization/bar": RepoTree(Repo("Test-Organization/bar", "main"), []),
-            "Test-Organization/baz": RepoTree(Repo("Test-Organization/baz", "master"), []),


bar and baz return 4xx errors, thus, we do not store them in the trees object anymore.

armenzg · 2023-02-03T21:05:26Z

tests/sentry/integrations/github/test_integration.py

    def test_get_trees_for_org_works(self):
        """Fetch the tree representation of a repo"""
        installation = self.get_installation_helper()
+        cache.clear()


I noticed that the next test would sometimes fail because the cache side effects of this test would affect the next.

I tried to add this as part of tearDown but it did not seem to matter. I will investigate on another occasion.

armenzg · 2023-02-03T21:05:52Z

tests/sentry/integrations/github/test_integration.py

        with patch("sentry.integrations.github.client.MINIMUM_REQUESTS", new=5, autospec=False):
            # We start with one request left before reaching the minimum remaining requests floor
            self.set_rate_limit(remaining=6)
+            assert cache.get(repos_key) is None


This is a good enough test to notice the cache cleared.

One of our customers is intermittently exhausting their rate limit, thus, affecting the commit_context tasks. Changes included: * get_repositories will not make API requests if the remaining requests is 200 or less. We will keep fetching repositories in the next hour when the limit is reset. * Spread caching of repositories across 24 hours. This will make the task only use 1/24th of the required API requests. Fixes (SENTRY-Y3M)[https://sentry.sentry.io/issues/3900033302/]

This fixes an issue where we would give up processing as soon as any Github API error would be returned (to the exception of some defined cases). This also moves handling of errors to the caller in order to have a centralized processing. It seems that [we raise unidentified API errors](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L124-L126) in get_tree. [get_cached_repo_files does not catch it](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L130-L161). [The iteration of the repositories happens within the try/block](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L170-L176), thus, failing to work as soon as one repo fails to process.

snigdhas · 2023-02-06T23:16:54Z

src/sentry/integrations/github/client.py

        """
+
+        def process_error(error: ApiError, extra: Dict[str, str]) -> None:
+            msg = "Continuing exectuion."


execution

This is easier to read if we just include it as part of the logger messages below since it's not conditional

src/sentry/integrations/github/client.py

…44126) This fixes an issue where we would give up processing as soon as any Github API error would be returned (to the exception of some defined cases). This also moves handling of errors to the caller in order to have a centralized processing. It seems that [we raise unidentified API errors](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L124-L126) in get_tree. [get_cached_repo_files does not catch it](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L130-L161). [The iteration of the repositories happens within the try/block](https://github.com/getsentry/sentry/blob/d0252f70b97ceef909c86128566716255fab7eab/src/sentry/integrations/github/client.py#L170-L176), thus, failing to work as soon as one repo fails to process.

armenzg self-assigned this Feb 3, 2023

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 3, 2023

vercel bot deployed to Preview – sentry February 3, 2023 19:57 View deployment

vercel bot deployed to Preview – storybook February 3, 2023 20:01 View deployment

armenzg changed the title ~~ref(dervied_code_mappings): Adjust handling of errors~~ feat(derived_code_mappings): Do not abort if a repo fails to process Feb 3, 2023

armenzg changed the title ~~feat(derived_code_mappings): Do not abort if a repo fails to process~~ fix(derived_code_mappings): Do not abort if a repo fails to process Feb 3, 2023

vercel bot deployed to Preview – sentry February 3, 2023 20:25 View deployment

vercel bot deployed to Preview – storybook February 3, 2023 20:27 View deployment

armenzg force-pushed the armenzg/derived-code-mappings/catch-at-the-right-place branch from 67327c3 to 03bde1d Compare February 3, 2023 20:47

vercel bot deployed to Preview – sentry February 3, 2023 20:50 View deployment

vercel bot deployed to Preview – storybook February 3, 2023 20:50 View deployment

vercel bot deployed to Preview – sentry February 3, 2023 21:05 View deployment

vercel bot deployed to Preview – storybook February 3, 2023 21:05 View deployment

armenzg commented Feb 3, 2023

View reviewed changes

armenzg marked this pull request as ready for review February 3, 2023 21:06

armenzg requested review from a team February 3, 2023 21:06

Base automatically changed from armenzg/derived-code-mappings/prevent-api-rate-limit-exhaustion to master February 6, 2023 18:22

armenzg marked this pull request as draft February 6, 2023 19:27

armenzg added 3 commits February 6, 2023 14:40

Minor fix

28b4d56

armenzg force-pushed the armenzg/derived-code-mappings/catch-at-the-right-place branch from b2b73d0 to 28b4d56 Compare February 6, 2023 19:44

vercel bot deployed to Preview – sentry February 6, 2023 19:47 View deployment

vercel bot deployed to Preview – storybook February 6, 2023 19:48 View deployment

armenzg marked this pull request as ready for review February 6, 2023 19:49

scttcper approved these changes Feb 6, 2023

View reviewed changes

snigdhas reviewed Feb 6, 2023

View reviewed changes

armenzg commented Feb 7, 2023

View reviewed changes

src/sentry/integrations/github/client.py Outdated Show resolved Hide resolved

Apply suggestions from code review

02c7160

armenzg enabled auto-merge (squash) February 7, 2023 14:11

vercel bot deployed to Preview – sentry February 7, 2023 14:12 View deployment

vercel bot deployed to Preview – storybook February 7, 2023 14:14 View deployment

armenzg merged commit e7544a3 into master Feb 7, 2023

armenzg deleted the armenzg/derived-code-mappings/catch-at-the-right-place branch February 7, 2023 14:29

github-actions bot locked and limited conversation to collaborators Feb 23, 2023

	try:
	# The Github API rate limit is reset every hour
	# Spread the expiration of the cache of each repo across the day
	trees[repo_full_name] = self._populate_tree(
	repo_info, only_use_cache, (3600 * 24) + (3600 * (index % 24))
	)
	except ApiError as error:
	process_error(error, extra)
	except Exception:
	# Report for investigatagiation but do not stop processing
	logger.exception(

Uh oh!

fix(derived_code_mappings): Do not abort if a repo fails to process #44126

fix(derived_code_mappings): Do not abort if a repo fails to process #44126

Uh oh!

Conversation

armenzg commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armenzg Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armenzg Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

armenzg commented Feb 3, 2023 •

edited

Loading

armenzg Feb 3, 2023 •

edited

Loading

armenzg Feb 3, 2023 •

edited

Loading