Merge pull request #1012 from mdboom/python-core-metrics

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v31.2.0...main)
 
+* Python
+    * BUGFIX: Core metrics are now present in every ping, even if submit is called before initialize has a chance to complete. ([#1012](https://github.com/mozilla/glean/pull/1012))
+
 # v31.2.0 (2020-06-24)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v31.1.2...v31.2.0)

glean-core/python/glean/glean.py

@@ -417,18 +417,20 @@ def _initialize_core_metrics(cls) -> None:
         """
         from ._builtins import metrics
 
-        metrics.glean.baseline.locale.set(_util.get_locale_tag())
-        metrics.glean.internal.metrics.os_version.set(platform.release())
-        metrics.glean.internal.metrics.architecture.set(platform.machine())
-        metrics.glean.internal.metrics.locale.set(_util.get_locale_tag())
+        metrics.glean.baseline.locale._set_sync(_util.get_locale_tag())
+        metrics.glean.internal.metrics.os_version._set_sync(platform.release())
+        metrics.glean.internal.metrics.architecture._set_sync(platform.machine())
+        metrics.glean.internal.metrics.locale._set_sync(_util.get_locale_tag())
 
         if cls._configuration.channel is not None:
-            metrics.glean.internal.metrics.app_channel.set(cls._configuration.channel)
+            metrics.glean.internal.metrics.app_channel._set_sync(
+                cls._configuration.channel
+            )
 
-        metrics.glean.internal.metrics.app_build.set(cls._application_id)
+        metrics.glean.internal.metrics.app_build._set_sync(cls._application_id)
 
         if cls._application_version is not None:
-            metrics.glean.internal.metrics.app_display_version.set(
+            metrics.glean.internal.metrics.app_display_version._set_sync(
                 cls._application_version
             )
 

glean-core/python/glean/metrics/string.py

@@ -66,6 +66,19 @@ def set(self, value: str) -> None:
         def set():
             _ffi.lib.glean_string_set(self._handle, _ffi.ffi_encode_string(value))
 
+    def _set_sync(self, value: str) -> None:
+        """
+        Set a string value, synchronously.
+
+        Args:
+            value (str): This is a user-defined string value. If the length of
+                the string exceeds the maximum length, it will be truncated.
+        """
+        if self._disabled:
+            return
+
+        _ffi.lib.glean_string_set(self._handle, _ffi.ffi_encode_string(value))
+
     def test_has_value(self, ping_name: Optional[str] = None) -> bool:
         """
         Tests whether a value is stored for the metric for testing purposes

glean-core/python/tests/test_glean.py

@@ -682,3 +682,51 @@ def test_confirm_enums_match_values_in_glean_parser():
     ]:
         for name in gp_enum.__members__.keys():
             assert g_enum[name.upper()].value == gp_enum[name].value
+
+
+def test_presubmit_makes_a_valid_ping(tmpdir, ping_schema_url, monkeypatch):
+    # Bug 1648140: Submitting a ping prior to initialize meant that the core
+    # metrics wouldn't yet be set.
+
+    info_path = Path(str(tmpdir)) / "info.txt"
+
+    Glean._reset()
+
+    ping_name = "preinit_ping"
+    ping = PingType(
+        name=ping_name, include_client_id=True, send_if_empty=True, reason_codes=[]
+    )
+
+    # This test relies on testing mode to be disabled, since we need to prove the
+    # real-world async behaviour of this.
+    Dispatcher._testing_mode = False
+    Dispatcher._queue_initial_tasks = True
+
+    # Submit a ping prior to calling initialize
+    ping.submit()
+    Glean.initialize(
+        application_id=GLEAN_APP_ID,
+        application_version=glean_version,
+        upload_enabled=True,
+        configuration=Glean._configuration,
+    )
+
+    monkeypatch.setattr(
+        Glean._configuration, "ping_uploader", _RecordingUploader(info_path)
+    )
+
+    # Wait until the work is complete
+    Dispatcher._task_worker._queue.join()
+
+    while not info_path.exists():
+        time.sleep(0.1)
+
+    with info_path.open("r") as fd:
+        url_path = fd.readline()
+        serialized_ping = fd.readline()
+
+    assert ping_name == url_path.split("/")[3]
+
+    assert 0 == validate_ping.validate_ping(
+        io.StringIO(serialized_ping), sys.stdout, schema_url=ping_schema_url,
+    )