diff --git a/.circleci/config.yml b/.circleci/config.yml index 154e3d841e..0e6b32897a 100644 --- a/.circleci/config.yml +++ b/.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 diff --git a/glean-core/glean-sym/src/metrics.rs b/glean-core/glean-sym/src/metrics.rs index 0465cf6ae3..c5b083f6f8 100644 --- a/glean-core/glean-sym/src/metrics.rs +++ b/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 { diff --git a/glean-core/glean-sym/src/types.rs b/glean-core/glean-sym/src/types.rs index 783e431de8..84c283514b 100644 --- a/glean-core/glean-sym/src/types.rs +++ b/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; +} + +pub enum NoExtraKeys {} + +impl ExtraKeys for NoExtraKeys { + const ALLOWED_KEYS: &'static [&'static str] = &[]; + + fn into_ffi_extra(self) -> HashMap { + 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 { + pub(crate) inner: crate::metrics::EventMetric, + extra_keys: PhantomData, +} + +impl EventMetric { + /// 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>>(&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) + } +} diff --git a/samples/glean-sym-test/Makefile b/samples/glean-sym-test/Makefile index 2bce144b2a..c66081abdb 100644 --- a/samples/glean-sym-test/Makefile +++ b/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 diff --git a/samples/glean-sym-test/app.py b/samples/glean-sym-test/app.py index ea0609a548..f31855d883 100644 --- a/samples/glean-sym-test/app.py +++ b/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"] diff --git a/samples/glean-sym-test/pyproject.toml b/samples/glean-sym-test/pyproject.toml new file mode 100644 index 0000000000..4790743b34 --- /dev/null +++ b/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", +] diff --git a/samples/glean-sym-test/services/lib.rs b/samples/glean-sym-test/services/lib.rs index 406baab771..f653b5058e 100644 --- a/samples/glean-sym-test/services/lib.rs +++ b/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); } diff --git a/samples/glean-sym-test/services/metrics.yaml b/samples/glean-sym-test/services/metrics.yaml index a4e3fed98f..edaaea2fe7 100644 --- a/samples/glean-sym-test/services/metrics.yaml +++ b/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" diff --git a/tools/glean-sym-parser/src/lib.rs b/tools/glean-sym-parser/src/lib.rs index efb25ed738..0ea90ce12e 100755 --- a/tools/glean-sym-parser/src/lib.rs +++ b/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 }