errors: Compute deployment metadata on the right deployment.

When I added this "Deployed code" feature to the error reporting,
I apparently hadn't worked out enough of how this code works to
realize that `notify_server_error` may be in a different process,
at a different time and potentially even on a different machine
from the actual error being reported.

Given that architecture, all the data about the error must be computed
in `AdminNotifyHandler`, before sending the report through the queue,
or else it risks being wrong.  The job of `notify_server_error` and
friends is only to format the data and send it off.  So, move the
implementation of this feature in order to do that.

(@showell added some "nocoverage" directives here for code that
is hard to test (exceptions being thrown, deployment files not
existing) and that was originally part of a file that didn't
require 100% coverage)

zerver/lib/error_notify.py

@@ -1,8 +1,6 @@
 # System documented in https://zulip.readthedocs.io/en/latest/subsystems/logging.html
 
 import logging
-import os
-import subprocess
 
 from collections import defaultdict
 from django.conf import settings
@@ -14,7 +12,6 @@ from typing import Any, Dict, Optional, Text
 from zerver.models import get_system_bot
 from zerver.lib.actions import internal_send_message
 from zerver.lib.response import json_success, json_error
-from version import ZULIP_VERSION
 
 def format_subject(subject: str) -> str:
     """
@@ -35,30 +32,14 @@ def user_info_str(report: Dict[str, Any]) -> str:
     user_info += " on %s deployment"  % (report['deployment'],)
     return user_info
 
-def try_git_describe() -> Optional[str]:
+def deployment_repr(report: Dict[str, Any]) -> str:
-    try:
-        return subprocess.check_output(
-            ['git',
-             '--git-dir', os.path.join(os.path.dirname(__file__), '../../.git'),
-             'describe', '--tags', '--always', '--dirty', '--long'],
-            stderr=subprocess.PIPE,
-        ).strip().decode('utf-8')
-    except Exception:
-        return None
-
-def deployment_repr() -> str:
     deployment = 'Deployed code:\n'
-
+    for (label, field) in [('git', 'git_described'),
-    git_described = try_git_describe()
+                           ('ZULIP_VERSION', 'zulip_version_const'),
-    if git_described is not None:
+                           ('version', 'zulip_version_file'),
-        deployment += '- git: %s\n' % (git_described,)
+                           ]:
-
+        if report[field] is not None:
-    deployment += '- ZULIP_VERSION: %s\n' % (ZULIP_VERSION,)
+            deployment += '- %s: %s\n' % (label, report[field])
-
-    version_path = os.path.join(os.path.dirname(__file__), '../../version')
-    if os.path.exists(version_path):
-        deployment += '- version: %s\n' % (open(version_path).read().strip(),)
-
     return deployment
 
 def notify_browser_error(report: Dict[str, Any]) -> None:
@@ -113,7 +94,7 @@ def zulip_server_error(report: Dict[str, Any]) -> None:
 
     logger_str = logger_repr(report)
     user_info = user_info_str(report)
-    deployment = deployment_repr()
+    deployment = deployment_repr(report)
 
     if report['has_request']:
         request_repr = (
@@ -138,7 +119,7 @@ def email_server_error(report: Dict[str, Any]) -> None:
 
     logger_str = logger_repr(report)
     user_info = user_info_str(report)
-    deployment = deployment_repr()
+    deployment = deployment_repr(report)
 
     if report['has_request']:
         request_repr = (

zerver/logging_handlers.py

@@ -2,6 +2,8 @@
 
 import logging
 import platform
+import os
+import subprocess
 import traceback
 from typing import Any, Dict, Optional
 
@@ -13,6 +15,26 @@ from django.views.debug import ExceptionReporter, get_exception_reporter_filter
 
 from zerver.lib.logging_util import find_log_caller_module
 from zerver.lib.queue import queue_json_publish
+from version import ZULIP_VERSION
+
+def try_git_describe() -> Optional[str]:
+    try:
+        return subprocess.check_output(
+            ['git',
+             '--git-dir', os.path.join(os.path.dirname(__file__), '../.git'),
+             'describe', '--tags', '--always', '--dirty', '--long'],
+            stderr=subprocess.PIPE,
+        ).strip().decode('utf-8')
+    except Exception:  # nocoverage
+        return None
+
+def add_deployment_metadata(report: Dict[str, Any]) -> None:
+    report['git_described'] = try_git_describe()
+    report['zulip_version_const'] = ZULIP_VERSION
+
+    version_path = os.path.join(os.path.dirname(__file__), '../version')
+    if os.path.exists(version_path):
+        report['zulip_version_file'] = open(version_path).read().strip()  # nocoverage
 
 def add_request_metadata(report: Dict[str, Any], request: HttpRequest) -> None:
     report['has_request'] = True
@@ -72,6 +94,8 @@ class AdminNotifyHandler(logging.Handler):
             report['node'] = platform.node()
             report['host'] = platform.node()
 
+            add_deployment_metadata(report)
+
             if record.exc_info:
                 stack_trace = ''.join(traceback.format_exception(*record.exc_info))
                 message = str(record.exc_info[1])