From 517538a296f953eb6fc7ca23e4d919f7d0fa4cdf Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Wed, 19 Feb 2025 16:05:28 +0800 Subject: [PATCH] management: Don't silence send_server_data_to_push_bouncer exceptions. When these exceptions are thrown from the request-to-bouncer functions inside of manage.py register_server/update_analytics_counts, they shouldn't be silenced, merely calling maybe_mark_pushes_disabled in the background. This results in the occurrence of the error not being shown to the user. Failure to upload analytics data when running these commands should result in a loud, obvious error. Failure of running register_server before this change: ``` ./manage.py register_server This command registers your server for the Mobile Push Notifications Service. Doing so will share basic metadata with the service's maintainers: * This server's configured hostname: zulipdev.com:9991 * This server's configured contact email address: desdemona+admin@zulip.com * Metadata about each organization hosted by the server; see: Use of this service is governed by the Zulip Terms of Service: Do you want to agree to the Zulip Terms of Service and proceed? [Y/n] Mobile Push Notification Service registration successfully updated! ``` The occurrence of the error is not revealed to the user. Same concern applies to the update_analytics_counts command. After this change: ``` ./manage.py register_server This command registers your server for the Mobile Push Notifications Service. Doing so will share basic metadata with the service's maintainers: <...> Do you want to agree to the Zulip Terms of Service and proceed? [Y/n] Traceback (most recent call last): File "/srv/zulip/./manage.py", line 150, in execute_from_command_line(sys.argv) File "/srv/zulip/./manage.py", line 115, in execute_from_command_line utility.execute() File "/srv/zulip-venv-cache/bb36fc1fcb6d8c70a9a0bcb7bac45d78623a9ff4/zulip-py3-venv/lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/srv/zulip-venv-cache/bb36fc1fcb6d8c70a9a0bcb7bac45d78623a9ff4/zulip-py3-venv/lib/python3.10/site-packages/django/core/management/base.py", line 413, in run_from_argv self.execute(*args, **cmd_options) File "/srv/zulip/zerver/lib/management.py", line 97, in execute super().execute(*args, **options) File "/srv/zulip-venv-cache/bb36fc1fcb6d8c70a9a0bcb7bac45d78623a9ff4/zulip-py3-venv/lib/python3.10/site-packages/django/core/management/base.py", line 459, in execute output = self.handle(*args, **options) File "/srv/zulip/zerver/management/commands/register_server.py", line 137, in handle send_server_data_to_push_bouncer(consider_usage_statistics=False, raise_on_error=True) File "/srv/zulip/zerver/lib/remote_server.py", line 453, in send_server_data_to_push_bouncer response = send_to_push_bouncer( File "/srv/zulip/zerver/lib/remote_server.py", line 233, in send_to_push_bouncer raise JsonableError(msg) zerver.lib.exceptions.JsonableError: Duplicate registration detected. ``` --- analytics/management/commands/update_analytics_counts.py | 2 +- zerver/lib/remote_server.py | 8 +++++++- zerver/management/commands/register_server.py | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/analytics/management/commands/update_analytics_counts.py b/analytics/management/commands/update_analytics_counts.py index 541ebf07b5..01efeac32e 100644 --- a/analytics/management/commands/update_analytics_counts.py +++ b/analytics/management/commands/update_analytics_counts.py @@ -95,4 +95,4 @@ class Command(ZulipBaseCommand): logger.info("Sleeping %d seconds before reporting...", delay) time.sleep(delay) - send_server_data_to_push_bouncer(consider_usage_statistics=True) + send_server_data_to_push_bouncer(consider_usage_statistics=True, raise_on_error=True) diff --git a/zerver/lib/remote_server.py b/zerver/lib/remote_server.py index 7b5b4d62b7..e2a224e7fa 100644 --- a/zerver/lib/remote_server.py +++ b/zerver/lib/remote_server.py @@ -387,13 +387,17 @@ def should_send_analytics_data() -> bool: # nocoverage return settings.ANALYTICS_DATA_UPLOAD_LEVEL > AnalyticsDataUploadLevel.NONE -def send_server_data_to_push_bouncer(consider_usage_statistics: bool = True) -> None: +def send_server_data_to_push_bouncer( + consider_usage_statistics: bool = True, raise_on_error: bool = False +) -> None: logger = logging.getLogger("zulip.analytics") # first, check what's latest try: result = send_to_push_bouncer("GET", "server/analytics/status", {}) except (JsonableError, orjson.JSONDecodeError) as e: maybe_mark_pushes_disabled(e, logger) + if raise_on_error: # nocoverage + raise return # Gather only entries with IDs greater than the last ID received by the push bouncer. @@ -452,6 +456,8 @@ def send_server_data_to_push_bouncer(consider_usage_statistics: bool = True) -> "POST", "server/analytics", request.model_dump(round_trip=True) ) except (JsonableError, orjson.JSONDecodeError) as e: + if raise_on_error: # nocoverage + raise maybe_mark_pushes_disabled(e, logger) return diff --git a/zerver/management/commands/register_server.py b/zerver/management/commands/register_server.py index 6d93868b27..5822c07a4d 100644 --- a/zerver/management/commands/register_server.py +++ b/zerver/management/commands/register_server.py @@ -134,7 +134,7 @@ that registration and saving the updated secret in "/api/v1/remotes/server/register", request ) - send_server_data_to_push_bouncer(consider_usage_statistics=False) + send_server_data_to_push_bouncer(consider_usage_statistics=False, raise_on_error=True) if response.json()["created"]: print(