Bug 1918097 - Add profiler markers for Glean::CounterMetric, and name lookup function r=chutten,florian,canaltinova

Differential Revision: https://phabricator.services.mozilla.com/D221795

Cargo.lock

@@ -1812,6 +1812,7 @@ version = "0.1.0"
 dependencies = [
  "bincode",
  "chrono",
+ "gecko-profiler",
  "glean",
  "inherent",
  "log",

toolkit/components/glean/api/Cargo.toml

@@ -10,9 +10,11 @@ license = "MPL-2.0"
 bincode = "1.0"
 chrono = "0.4.10"
 glean = { workspace = true }
+
 inherent = "1.0.0"
 log = "0.4"
 nsstring = { path = "../../../../xpcom/rust/nsstring", optional = true }
+gecko-profiler = { path = "../../../../tools/profiler/rust-api", optional = true }
 once_cell = "1.2.0"
 serde = { version = "1.0", features = ["derive"] }
 uuid = { version = "1.0", features = ["v4"] }
@@ -25,4 +27,4 @@ serde_json = "1"
 tempfile = "3.1.0"
 
 [features]
-with_gecko = ["xpcom", "nsstring"]
+with_gecko = ["xpcom", "nsstring", "gecko-profiler"]

toolkit/components/glean/api/src/private/counter.rs

@@ -2,14 +2,56 @@
 // 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 glean::traits::Counter;
 use inherent::inherent;
 use std::sync::Arc;
 
-use glean::traits::Counter;
-
 use super::{CommonMetricData, MetricId};
 use crate::ipc::{need_ipc, with_ipc_payload};
 
+#[cfg(feature = "with_gecko")]
+use super::profiler_utils::{lookup_canonical_metric_name, LookupError};
+
+#[cfg(feature = "with_gecko")]
+use gecko_profiler::gecko_profiler_category;
+
+#[cfg(feature = "with_gecko")]
+#[derive(serde::Serialize, serde::Deserialize, Debug)]
+struct CounterMetricMarker {
+    id: MetricId,
+    val: i32,
+}
+
+#[cfg(feature = "with_gecko")]
+impl gecko_profiler::ProfilerMarker for CounterMetricMarker {
+    fn marker_type_name() -> &'static str {
+        "CounterMetric"
+    }
+
+    fn marker_type_display() -> gecko_profiler::MarkerSchema {
+        use gecko_profiler::schema::*;
+        let mut schema = MarkerSchema::new(&[Location::MarkerChart, Location::MarkerTable]);
+        schema.set_tooltip_label("{marker.data.id} {marker.data.val}");
+        schema.set_table_label("{marker.name} - {marker.data.id}: {marker.data.val}");
+        schema.add_key_label_format_searchable(
+            "id",
+            "Metric",
+            Format::String,
+            Searchable::Searchable,
+        );
+        schema.add_key_label_format("val", "Value", Format::Integer);
+        schema
+    }
+
+    fn stream_json_marker_data(&self, json_writer: &mut gecko_profiler::JSONWriter) {
+        json_writer.string_property(
+            "id",
+            lookup_canonical_metric_name(&self.id).unwrap_or_else(LookupError::as_str),
+        );
+        json_writer.int_property("val", self.val.into());
+    }
+}
+
 /// A counter metric.
 ///
 /// Used to count things.
@@ -91,10 +133,31 @@ impl Counter for CounterMetric {
     /// Logs an error if the `amount` is 0 or negative.
     pub fn add(&self, amount: i32) {
         match self {
-            CounterMetric::Parent { inner, .. } => {
+            #[allow(unused)]
+            CounterMetric::Parent { id, inner, .. } => {
+                #[cfg(feature = "with_gecko")]
+                gecko_profiler::add_marker(
+                    "Counter::add",
+                    gecko_profiler_category!(Telemetry),
+                    Default::default(),
+                    CounterMetricMarker {
+                        id: *id,
+                        val: amount,
+                    },
+                );
                 inner.add(amount);
             }
             CounterMetric::Child(c) => {
+                #[cfg(feature = "with_gecko")]
+                gecko_profiler::add_marker(
+                    "Counter::add",
+                    gecko_profiler_category!(Telemetry),
+                    Default::default(),
+                    CounterMetricMarker {
+                        id: c.0,
+                        val: amount,
+                    },
+                );
                 with_ipc_payload(move |payload| {
                     if let Some(v) = payload.counters.get_mut(&c.0) {
                         *v += amount;

toolkit/components/glean/api/src/private/mod.rs

@@ -82,3 +82,63 @@ impl From<u32> for MetricId {
         Self(id)
     }
 }
+
+// We only access the methods here when we're building with Gecko, as that's
+// when we have access to the profiler. We don't need alternative (i.e.
+// non-gecko) implementations, as any imports from this sub-module are also
+// gated with the same #[cfg(feature...)]
+#[cfg(feature = "with_gecko")]
+pub(crate) mod profiler_utils {
+    #[derive(Debug)]
+    pub(crate) enum LookupError {
+        NullPointer,
+        Utf8ParseError(std::str::Utf8Error),
+    }
+
+    impl LookupError {
+        pub fn as_str(self) -> &'static str {
+            match self {
+                LookupError::NullPointer => "id not found",
+                LookupError::Utf8ParseError(_) => "utf8 parse error",
+            }
+        }
+    }
+
+    impl std::fmt::Display for LookupError {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            match self {
+                LookupError::NullPointer => write!(f, "id not found"),
+                LookupError::Utf8ParseError(p) => write!(f, "utf8 parse error: {}", p),
+            }
+        }
+    }
+
+    pub(crate) fn lookup_canonical_metric_name(
+        id: &super::MetricId,
+    ) -> Result<&'static str, LookupError> {
+        #[allow(unused)]
+        use std::ffi::{c_char, CStr};
+        extern "C" {
+            fn FOG_GetMetricIdentifier(id: u32) -> *const c_char;
+        }
+        // SAFETY: We check to make sure that the returned pointer is not null
+        // before trying to construct a string from it. As the string array that
+        // `FOG_GetMetricIdentifier` references is statically defined and allocated,
+        // we know that any strings will be guaranteed to have a null terminator,
+        // and will have the same lifetime as the program, meaning we're safe to
+        // return a static lifetime, knowing that they won't be changed "underneath"
+        // us. Additionally, we surface any errors from parsing the string as utf8.
+        unsafe {
+            let raw_name_ptr = FOG_GetMetricIdentifier(id.0);
+            if raw_name_ptr.is_null() {
+                Err(LookupError::NullPointer)
+            } else {
+                let name = CStr::from_ptr(raw_name_ptr).to_str();
+                match name {
+                    Ok(s) => Ok(s),
+                    Err(ut8err) => Err(LookupError::Utf8ParseError(ut8err)),
+                }
+            }
+        }
+    }
+}

toolkit/components/glean/build_scripts/glean_parser_ext/templates/js.jinja2

@@ -124,6 +124,34 @@ const char* GetMetricIdentifier(metric_entry_t entry) {
   return &gMetricStringTable[offset];
 }
 
+// WARNING: This WILL break if the definition of a `metric_entry_t` changes.
+// This needs to be updated whenever the lookup macros in
+// GleanJSMetricsLookup.cpp change (i.e. at the top of this file).
+//
+// Looking up a metric name involved indexing into the `gMetricStringTable` with
+// a valid `metric_entry_t` index. A metric_id (32-bit int) is contained as a
+// subset of the bits within a `metric_entry_t`, so we enumerate over the list
+// of possible indicies, given in `sMetricNameByLookupEntries`. This is a little
+// brittle, as how a `metric_entry_t` encodes a metric_id is an implementation
+// detail that we shouldn't heavily rely on. However, if profiles start failing,
+// or giving nonsensical names, it's probably this relationship that needs to be
+// fixed.
+/**
+ * Look up the string identifier of a metric by its 32-bit id.
+ * Returns nullptr if we couldn't find a matching identifier for the id.
+ */
+extern "C" const char* FOG_GetMetricIdentifier(
+    uint32_t metric_id) {
+  for (metric_entry_t entry : sMetricByNameLookupEntries) {
+    if (static_cast<uint32_t>(GLEAN_METRIC_ID(entry >> GLEAN_INDEX_BITS)) == metric_id) {
+      return GetMetricIdentifier(entry);
+    }
+  }
+
+  // We didn't find our metric_id in the list of metric entries.
+  return nullptr;
+}
+
 /**
  * Check that the found entry is pointing to the right key
  * and return it.

toolkit/components/glean/tests/pytest/metrics_test_output_js_cpp

@@ -434,6 +434,34 @@ const char* GetMetricIdentifier(metric_entry_t entry) {
   return &gMetricStringTable[offset];
 }
 
+// WARNING: This WILL break if the definition of a `metric_entry_t` changes.
+// This needs to be updated whenever the lookup macros in
+// GleanJSMetricsLookup.cpp change (i.e. at the top of this file).
+//
+// Looking up a metric name involved indexing into the `gMetricStringTable` with
+// a valid `metric_entry_t` index. A metric_id (32-bit int) is contained as a
+// subset of the bits within a `metric_entry_t`, so we enumerate over the list
+// of possible indicies, given in `sMetricNameByLookupEntries`. This is a little
+// brittle, as how a `metric_entry_t` encodes a metric_id is an implementation
+// detail that we shouldn't heavily rely on. However, if profiles start failing,
+// or giving nonsensical names, it's probably this relationship that needs to be
+// fixed.
+/**
+ * Look up the string identifier of a metric by its 32-bit id.
+ * Returns nullptr if we couldn't find a matching identifier for the id.
+ */
+extern "C" const char* FOG_GetMetricIdentifier(
+    uint32_t metric_id) {
+  for (metric_entry_t entry : sMetricByNameLookupEntries) {
+    if (static_cast<uint32_t>(GLEAN_METRIC_ID(entry >> GLEAN_INDEX_BITS)) == metric_id) {
+      return GetMetricIdentifier(entry);
+    }
+  }
+
+  // We didn't find our metric_id in the list of metric entries.
+  return nullptr;
+}
+
 /**
  * Check that the found entry is pointing to the right key
  * and return it.