Bug 2053453 - glean-sym: Implement the EventMetric

.circleci/config.yml

@@ -121,6 +121,7 @@ commands:
       - run:
           name: Run glean-sym sample
           command: |
+            pip install uv
             cd samples/glean-sym-test
             make run
 

glean-core/glean-sym/src/metrics.rs

@@ -9,6 +9,7 @@
 // `glean-sym` tests ensure the vendored copy is unmodified.
 // This can be verified by running `cargo test -p glean-sym`.
 #![allow(clippy::all)]
+#![allow(unused)]
 #![cfg_attr(rustfmt, rustfmt_skip)]
 
 use crate::types::*;
@@ -1113,7 +1114,7 @@ impl DatetimeMetric {
     }
 }
 #[derive(uniffi::Record)]
-pub struct EventMetric {
+pub(crate) struct EventMetric {
     handle: u64,
 }
 impl EventMetric {

glean-core/glean-sym/src/types.rs

@@ -2,6 +2,8 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use std::{collections::HashMap, marker::PhantomData};
+
 /// Types defined in `glean.udl` and used in the public API.
 ///
 /// For now these are a copy of the same types in `glean-core`.
@@ -136,3 +138,75 @@ pub enum HistogramType {
 }
 
 pub type CowString = std::borrow::Cow<'static, str>;
+
+pub trait ExtraKeys {
+    /// List of allowed extra keys as strings.
+    const ALLOWED_KEYS: &'static [&'static str];
+
+    /// Convert the event extras into 2 lists:
+    ///
+    /// 1. The list of extra key indices.
+    ///    Unset keys will be skipped.
+    /// 2. The list of extra values.
+    fn into_ffi_extra(self) -> HashMap<String, String>;
+}
+
+pub enum NoExtraKeys {}
+
+impl ExtraKeys for NoExtraKeys {
+    const ALLOWED_KEYS: &'static [&'static str] = &[];
+
+    fn into_ffi_extra(self) -> HashMap<String, String> {
+        unimplemented!("non-existing extra keys can't be turned into a list")
+    }
+}
+
+/// Developer-facing API for recording event metrics.
+///
+/// Instances of this class type are automatically generated by the parsers
+/// at build time, allowing developers to record values that were previously
+/// registered in the metrics.yaml file.
+pub struct EventMetric<K> {
+    pub(crate) inner: crate::metrics::EventMetric,
+    extra_keys: PhantomData<K>,
+}
+
+impl<K: ExtraKeys> EventMetric<K> {
+    /// The public constructor used by automatically generated metrics.
+    pub fn new(meta: CommonMetricData) -> Self {
+        let allowed_extra_keys = K::ALLOWED_KEYS.iter().map(|s| s.to_string()).collect();
+        let inner = crate::metrics::EventMetric::new(meta, allowed_extra_keys);
+        Self {
+            inner,
+            extra_keys: PhantomData,
+        }
+    }
+
+    /// Records an event.
+    ///
+    /// # Arguments
+    ///
+    /// * `extra` - (optional) An object for the extra keys.
+    pub fn record<M: Into<Option<K>>>(&self, extra: M) {
+        let extra = extra
+            .into()
+            .map(|e| e.into_ffi_extra())
+            .unwrap_or_else(HashMap::new);
+        self.inner.record(extra);
+    }
+
+    /// **Exported for test purposes.**
+    ///
+    /// Gets the number of recorded errors for the given metric and error type.
+    ///
+    /// # Arguments
+    ///
+    /// * `error` - The type of error
+    ///
+    /// # Returns
+    ///
+    /// The number of errors reported.
+    pub fn test_get_num_recorded_errors(&self, error: ErrorType) -> i32 {
+        self.inner.test_get_num_recorded_errors(error)
+    }
+}

samples/glean-sym-test/Makefile

@@ -15,4 +15,4 @@ help:
 .PHONY: run
 run:  ## Run the sample app
 	cargo build --locked
-	env $(LIB_PATH_NAME)=target/debug python3 app.py
+	env $(LIB_PATH_NAME)=target/debug uv run pytest app.py

samples/glean-sym-test/app.py

@@ -15,37 +15,49 @@ def library_name(name):
     return f"lib{name}.{suffix}"
 
 
-xul = cdll.LoadLibrary(library_name("xul"))
-services = cdll.LoadLibrary(library_name("services"))
-
-with tempfile.TemporaryDirectory() as data_path:
-    startup_fn = xul.startup
-    startup_fn.argtypes = [ctypes.c_char_p]
-    startup_fn(str.encode(data_path))
-
-    services_record = services.record
-    services_record.argtypes = [ctypes.c_int32]
-
-    amount = 31
-    services_record(amount)
-
-    xul.submit()
-    xul.shutdown()
-
-    # Check that
-    # * We submitted one ping only
-    # * It's the `prototype` ping
-    # * It contains 2 metrics with the expected values
-    path = os.path.join(data_path, "sent_pings")
-    for root, dirs, files in os.walk(path):
-        assert len(files) == 1
-        assert "prototype-" in files[0]
-
-        sent_ping = os.path.join(path, files[0])
-        data = open(sent_ping).read()
-        end_first_object = data.find("}")
-        payload = json.loads(data[end_first_object + 1 :])
-        counter = payload["metrics"]["counter"]
-
-        assert 1 == counter["test.metrics.sample_counter"]
-        assert amount == counter["dylib.counting"]
+def test_run():
+    xul = cdll.LoadLibrary(library_name("xul"))
+    services = cdll.LoadLibrary(library_name("services"))
+
+    with tempfile.TemporaryDirectory() as data_path:
+        startup_fn = xul.startup
+        startup_fn.argtypes = [ctypes.c_char_p]
+        startup_fn(str.encode(data_path))
+
+        services_record = services.record
+        services_record.argtypes = [ctypes.c_int32]
+
+        amount = 31
+        services_record(amount)
+
+        xul.submit()
+        xul.shutdown()
+
+        # Check that
+        # * We submitted one ping only
+        # * It's the `prototype` ping
+        # * It contains 2 metrics with the expected values
+        path = os.path.join(data_path, "sent_pings")
+        for root, dirs, files in os.walk(path):
+            assert len(files) == 1
+            assert "prototype-" in files[0]
+
+            sent_ping = os.path.join(path, files[0])
+            data = open(sent_ping).read()
+            end_first_object = data.find("}")
+            payload = json.loads(data[end_first_object + 1 :])
+            counter = payload["metrics"]["counter"]
+            events = payload["events"]
+
+            assert 1 == counter["test.metrics.sample_counter"]
+            assert amount == counter["dylib.counting"]
+
+            assert 2 == len(events)
+
+            no_extra = events[0]
+            assert "event" == no_extra["name"]
+
+            with_extra = events[1]
+            assert "event_with_extras" == with_extra["name"]
+            extras = with_extra["extra"]
+            assert "true", extras["is_set"]

samples/glean-sym-test/pyproject.toml

@@ -0,0 +1,8 @@
+[project]
+name = "glean-sym-sample"
+version = "0.0.0"
+requires-python = ">=3.9"
+
+dependencies = [
+  "pytest==8.4.2",
+]

samples/glean-sym-test/services/lib.rs

@@ -32,5 +32,9 @@ unsafe extern "C" fn record(amount: i32) {
     let stored = glean_metrics::dylib::data.test_get_value(None).unwrap();
     assert_eq!("value", stored);
 
+    glean_metrics::dylib::event.record(None);
+    let extra = glean_metrics::dylib::EventWithExtrasExtra { is_set: Some(true) };
+    glean_metrics::dylib::event_with_extras.record(extra);
+
     glean_metrics::dylib::timing.stop_and_accumulate(tid);
 }

samples/glean-sym-test/services/metrics.yaml

@@ -48,3 +48,35 @@ dylib:
     expires: never
     send_in_pings:
       - prototype
+
+  event:
+    type: event
+    description: |
+      Just testing strings
+    bugs:
+      - https://bugzilla.mozilla.org/123456789
+    data_reviews:
+      - N/A
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never
+    send_in_pings:
+      - prototype
+
+  event_with_extras:
+    type: event
+    description: |
+      Just testing strings
+    bugs:
+      - https://bugzilla.mozilla.org/123456789
+    data_reviews:
+      - N/A
+    notification_emails:
+      - CHANGE-ME@example.com
+    expires: never
+    send_in_pings:
+      - prototype
+    extra_keys:
+      is_set:
+        type: boolean
+        description: "True if this extra is set"

tools/glean-sym-parser/src/lib.rs

@@ -25,6 +25,7 @@ const PREAMBLE: &str = r#"// DO NOT MODIFY!
 // `glean-sym` tests ensure the vendored copy is unmodified.
 // This can be verified by running `cargo test -p glean-sym`.
 #![allow(clippy::all)]
+#![allow(unused)]
 #![cfg_attr(rustfmt, rustfmt_skip)]
 
 use crate::types::*;
@@ -90,9 +91,14 @@ pub fn generate(content: &str) -> String {
         let structname = ident.0.to_lowercase().replace("_", "");
         let ident = format_ident!("{}", ident.0);
         let extern_fn_ident = format_ident!("uniffi_glean_core_fn_clone_{}", structname);
+        let visibility = if structname == "eventmetric" {
+            quote! { pub(crate) }
+        } else {
+            quote! { pub }
+        };
         tokens.push(quote! {
             #[derive(uniffi::Record)]
-            pub struct #ident {
+            #visibility struct #ident {
                 handle: u64
             }