From c748b1cac0ba62276c57779839df4cc4b522d917 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Apr 2025 12:15:26 +0200 Subject: [PATCH 1/3] Update `shared` and `sentry_sdk` This pulls in an updated version of shared that has more caching, in particular around GitHub calls. Also updates the `sentry_sdk`, and configures it to automatically flag redis calls related to caching. --- codecov/settings_base.py | 4 ++-- pyproject.toml | 6 +++--- uv.lock | 14 +++++++------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/codecov/settings_base.py b/codecov/settings_base.py index 55dbaba7cf..b15a697634 100644 --- a/codecov/settings_base.py +++ b/codecov/settings_base.py @@ -411,9 +411,9 @@ dsn=SENTRY_DSN, event_scrubber=EventScrubber(denylist=SENTRY_DENY_LIST), integrations=[ - DjangoIntegration(), + DjangoIntegration(signals_spans=False), CeleryIntegration(), - RedisIntegration(), + RedisIntegration(cache_prefixes=["cache:"]), HttpxIntegration(), ], environment=SENTRY_ENV, diff --git a/pyproject.toml b/pyproject.toml index 6751d72dea..3a9698d950 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -40,8 +40,8 @@ dependencies = [ "redis==4.4.4", "regex==2023.12.25", "requests==2.32.3", - "sentry-sdk>=2.13.0", - "sentry-sdk[celery]==2.13.0", + "sentry-sdk>=2.25.1", + "sentry-sdk[celery]>=2.25.1", "shared", "simplejson==3.17.2", "starlette==0.40.0", @@ -74,4 +74,4 @@ dev-dependencies = [ ] [tool.uv.sources] -shared = { git = "https://github.com/codecov/shared", rev = "ece4366f3bce7fab2216704f85e365fbbe0f147d" } +shared = { git = "https://github.com/codecov/shared", rev = "ae1d4cf84490f0ae41395cda3567fe3db7124be3" } diff --git a/uv.lock b/uv.lock index 41d75731ed..ea617ea90b 100644 --- a/uv.lock +++ b/uv.lock @@ -389,9 +389,9 @@ requires-dist = [ { name = "redis", specifier = "==4.4.4" }, { name = "regex", specifier = "==2023.12.25" }, { name = "requests", specifier = "==2.32.3" }, - { name = "sentry-sdk", specifier = ">=2.13.0" }, - { name = "sentry-sdk", extras = ["celery"], specifier = "==2.13.0" }, - { name = "shared", git = "https://github.com/codecov/shared?rev=ece4366f3bce7fab2216704f85e365fbbe0f147d" }, + { name = "sentry-sdk", specifier = ">=2.25.1" }, + { name = "sentry-sdk", extras = ["celery"], specifier = ">=2.25.1" }, + { name = "shared", git = "https://github.com/codecov/shared?rev=ae1d4cf84490f0ae41395cda3567fe3db7124be3" }, { name = "simplejson", specifier = "==3.17.2" }, { name = "starlette", specifier = "==0.40.0" }, { name = "stripe", specifier = ">=11.4.1" }, @@ -1561,15 +1561,15 @@ wheels = [ [[package]] name = "sentry-sdk" -version = "2.13.0" +version = "2.25.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "certifi" }, { name = "urllib3" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/bb/41/97f673384dae5ed81cc2a568cc5c28e76deee85f8ba50def862e86150a5a/sentry_sdk-2.13.0.tar.gz", hash = "sha256:8d4a576f7a98eb2fdb40e13106e41f330e5c79d72a68be1316e7852cf4995260", size = 279937 } +sdist = { url = "https://files.pythonhosted.org/packages/85/2f/a0f732270cc7c1834f5ec45539aec87c360d5483a8bd788217a9102ccfbd/sentry_sdk-2.25.1.tar.gz", hash = "sha256:f9041b7054a7cf12d41eadabe6458ce7c6d6eea7a97cfe1b760b6692e9562cf0", size = 322190 } wheels = [ - { url = "https://files.pythonhosted.org/packages/ad/7e/e9ca09f24a6c334286631a2d32c267cdc5edad5ac03fd9d20a01a82f1c35/sentry_sdk-2.13.0-py2.py3-none-any.whl", hash = "sha256:6beede8fc2ab4043da7f69d95534e320944690680dd9a963178a49de71d726c6", size = 309078 }, + { url = "https://files.pythonhosted.org/packages/96/b6/84049ab0967affbc7cc7590d86ae0170c1b494edb69df8786707100420e5/sentry_sdk-2.25.1-py2.py3-none-any.whl", hash = "sha256:60b016d0772789454dc55a284a6a44212044d4a16d9f8448725effee97aaf7f6", size = 339851 }, ] [package.optional-dependencies] @@ -1580,7 +1580,7 @@ celery = [ [[package]] name = "shared" version = "0.1.0" -source = { git = "https://github.com/codecov/shared?rev=ece4366f3bce7fab2216704f85e365fbbe0f147d#ece4366f3bce7fab2216704f85e365fbbe0f147d" } +source = { git = "https://github.com/codecov/shared?rev=ae1d4cf84490f0ae41395cda3567fe3db7124be3#ae1d4cf84490f0ae41395cda3567fe3db7124be3" } dependencies = [ { name = "amplitude-analytics" }, { name = "boto3" }, From 7f75f3b274ff8d5d3f2a89c52f580f0ffadfde25 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Apr 2025 12:21:43 +0200 Subject: [PATCH 2/3] remove manual celery wrapping --- services/task/task.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/services/task/task.py b/services/task/task.py index 0c201bb4a2..cb5f137cff 100644 --- a/services/task/task.py +++ b/services/task/task.py @@ -2,12 +2,8 @@ from datetime import datetime, timedelta from typing import Iterable, List, Optional, Tuple -import celery from celery import Celery, chain, group, signature -from celery.canvas import Signature -from django.conf import settings from sentry_sdk import set_tag -from sentry_sdk.integrations.celery import _wrap_apply_async from shared import celery_config from core.models import Repository @@ -19,17 +15,6 @@ log = logging.getLogger(__name__) -if settings.SENTRY_ENV: - celery.group.apply_async = _wrap_apply_async(celery.group.apply_async) - celery.chunks.apply_async = _wrap_apply_async(celery.chunks.apply_async) - celery.canvas._chain.apply_async = _wrap_apply_async( - celery.canvas._chain.apply_async - ) - celery.canvas._chord.apply_async = _wrap_apply_async( - celery.canvas._chord.apply_async - ) - Signature.apply_async = _wrap_apply_async(Signature.apply_async) - class TaskService(object): def _create_signature(self, name, args=None, kwargs=None, immutable=False): From 005f2f82f36bba4e6aeb7268bd2a2713e1efcf2f Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 4 Apr 2025 13:07:06 +0200 Subject: [PATCH 3/3] turn shelter off in tests, and do mocking a different way --- codecov/settings_test.py | 2 +- codecov_auth/tests/test_signals.py | 92 ++++++++++++------------------ core/tests/test_signals.py | 33 +++-------- 3 files changed, 45 insertions(+), 82 deletions(-) diff --git a/codecov/settings_test.py b/codecov/settings_test.py index 2e98ad868a..27d0b5013b 100644 --- a/codecov/settings_test.py +++ b/codecov/settings_test.py @@ -4,7 +4,7 @@ ALLOWED_HOSTS = ["localhost"] CORS_ALLOWED_ORIGINS = ["http://localhost:9000", "http://localhost"] -SHELTER_ENABLED = True +SHELTER_ENABLED = False SHELTER_PUBSUB_PROJECT_ID = "test-project-id" SHELTER_PUBSUB_SYNC_REPO_TOPIC_ID = "test-topic-id" diff --git a/codecov_auth/tests/test_signals.py b/codecov_auth/tests/test_signals.py index 639978b453..6a77c01178 100644 --- a/codecov_auth/tests/test_signals.py +++ b/codecov_auth/tests/test_signals.py @@ -2,42 +2,37 @@ from unittest.mock import call import pytest -from django.test import TestCase +from django.test import TestCase, override_settings from shared.django_apps.codecov_auth.models import Service from shared.django_apps.codecov_auth.tests.factories import ( OrganizationLevelTokenFactory, OwnerFactory, ) +from utils.shelter import ShelterPubsub + @pytest.mark.django_db def test_shelter_org_token_sync(mocker): - publish = mocker.patch("google.cloud.pubsub_v1.PublisherClient.publish") + publish = mocker.patch("utils.shelter.ShelterPubsub.publish") # this triggers the publish via Django signals OrganizationLevelTokenFactory(id=91728376, owner=OwnerFactory(ownerid=111)) publish.assert_has_calls( [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 111}', - ), - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "org_token", "sync": "one", "id": 91728376}', - ), + call({"type": "owner", "sync": "one", "id": 111}), + call({"type": "org_token", "sync": "one", "id": 91728376}), ] ) -@mock.patch("google.cloud.pubsub_v1.PublisherClient.publish") +@mock.patch("utils.shelter.ShelterPubsub.publish") class TestCodecovAuthSignals(TestCase): def test_sync_on_create(self, mock_publish): OwnerFactory(ownerid=12345) mock_publish.assert_called_once_with( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', + {"type": "owner", "sync": "one", "id": 12345} ) def test_sync_on_update_upload_token_required_for_public_repos(self, mock_publish): @@ -46,14 +41,8 @@ def test_sync_on_update_upload_token_required_for_public_repos(self, mock_publis owner.save() mock_publish.assert_has_calls( [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), + call({"type": "owner", "sync": "one", "id": 12345}), + call({"type": "owner", "sync": "one", "id": 12345}), ] ) @@ -63,14 +52,8 @@ def test_sync_on_update_username(self, mock_publish): owner.save() mock_publish.assert_has_calls( [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), + call({"type": "owner", "sync": "one", "id": 12345}), + call({"type": "owner", "sync": "one", "id": 12345}), ] ) @@ -80,14 +63,8 @@ def test_sync_on_update_service(self, mock_publish): owner.save() mock_publish.assert_has_calls( [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ), + call({"type": "owner", "sync": "one", "id": 12345}), + call({"type": "owner", "sync": "one", "id": 12345}), ] ) @@ -96,26 +73,31 @@ def test_no_sync_on_update_other_fields(self, mock_publish): owner.name = "world" owner.save() mock_publish.assert_called_once_with( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', + {"type": "owner", "sync": "one", "id": 12345} ) - @mock.patch("logging.Logger.warning") - def test_sync_error(self, mock_log, mock_publish): - mock_publish.side_effect = Exception("publish error") - OwnerFactory(ownerid=12345) +@mock.patch("logging.Logger.warning") +@mock.patch("google.cloud.pubsub_v1.PublisherClient.publish") +@mock.patch("utils.shelter.ShelterPubsub.get_instance") +@override_settings(SHELTER_ENABLED=True) +@pytest.mark.django_db +def test_sync_error(mock_instance, mock_publish, mock_log): + mock_instance.return_value = ShelterPubsub() + mock_publish.side_effect = Exception("publish error") - # publish is still called, raises an Exception - mock_publish.assert_called_once_with( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 12345}', - ) + OwnerFactory(ownerid=12345) - mock_log.assert_called_once_with( - "Failed to publish a message", - extra=dict( - data_to_publish={"type": "owner", "sync": "one", "id": 12345}, - error=mock_publish.side_effect, - ), - ) + # publish is still called, raises an Exception + mock_publish.assert_called_once_with( + "projects/test-project-id/topics/test-topic-id", + b'{"type": "owner", "sync": "one", "id": 12345}', + ) + + mock_log.assert_called_once_with( + "Failed to publish a message", + extra=dict( + data_to_publish={"type": "owner", "sync": "one", "id": 12345}, + error=mock_publish.side_effect, + ), + ) diff --git a/core/tests/test_signals.py b/core/tests/test_signals.py index b133bbc971..2e52541f70 100644 --- a/core/tests/test_signals.py +++ b/core/tests/test_signals.py @@ -7,7 +7,7 @@ @pytest.mark.django_db def test_shelter_repo_sync(mocker): - publish = mocker.patch("google.cloud.pubsub_v1.PublisherClient.publish") + publish = mocker.patch("utils.shelter.ShelterPubsub.publish") # this triggers the publish via Django signals repo = RepositoryFactory( @@ -17,14 +17,8 @@ def test_shelter_repo_sync(mocker): # triggers publish on create publish.assert_has_calls( [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 555}', - ), - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "repo", "sync": "one", "id": 91728376}', - ), + call({"type": "owner", "sync": "one", "id": 555}), + call({"type": "repo", "sync": "one", "id": 91728376}), ] ) @@ -35,10 +29,7 @@ def test_shelter_repo_sync(mocker): assert len(publish_calls) == 3 # triggers publish on update - assert publish_calls[2] == call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "repo", "sync": "one", "id": 91728376}', - ) + assert publish_calls[2] == call({"type": "repo", "sync": "one", "id": 91728376}) # Does not trigger another publish with untracked field repo.message = "foo" @@ -54,14 +45,7 @@ def test_shelter_repo_sync(mocker): publish_calls = publish.call_args_list # 1 is for the new owner created assert len(publish_calls) == 5 - publish.assert_has_calls( - [ - call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "owner", "sync": "one", "id": 888}', - ), - ] - ) + publish.assert_has_calls([call({"type": "owner", "sync": "one", "id": 888})]) # Triggers call when private is changed repo.private = True @@ -73,7 +57,7 @@ def test_shelter_repo_sync(mocker): @pytest.mark.django_db def test_shelter_commit_sync(mocker): - publish = mocker.patch("google.cloud.pubsub_v1.PublisherClient.publish") + publish = mocker.patch("utils.shelter.ShelterPubsub.publish") # this triggers the publish via Django signals - has to have this format owner = OwnerFactory(ownerid=555) @@ -90,10 +74,7 @@ def test_shelter_commit_sync(mocker): assert len(publish_calls) == 3 # triggers publish on update - assert publish_calls[2] == call( - "projects/test-project-id/topics/test-topic-id", - b'{"type": "commit", "sync": "one", "id": 167829367}', - ) + assert publish_calls[2] == call({"type": "commit", "sync": "one", "id": 167829367}) commit.branch = "normal-incompatible-branch" commit.save()