telemetry: enrich platform and startup metrics

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d0e7cadc-8f44-443f-affe-6ce8e856a218

Author: bfjelds

Cargo.toml

@@ -61,6 +61,7 @@ nix = { version = "0.30.1", features = [
     "user",
     "socket",
     "signal",
+    "time",
 ], default-features = false }
 oci-client = "0.15.0"
 once_cell = "1.19"

crates/osutils/src/virt.rs

@@ -1,4 +1,24 @@
-//! This module contains helper functions for working with virtualized environments.
+//! Detects whether Trident is running inside a virtual machine, and if so,
+//! which hypervisor, by reading DMI/SMBIOS sysfs files. Shared by
+//! `trident::diagnostics` (the support-bundle `HostDescription::virt_type`,
+//! the detected hypervisor name or `None`) and `trident::logging::tracestream`
+//! (the `platform_info.vm` telemetry field, a plain boolean), so the two
+//! never diverge on what counts as "virtual". Also home to the unrelated
+//! `virtdeploy` best-effort detection used by boot-order workarounds.
+//!
+//! `detect_hypervisor` only recognizes hypervisors that override the DMI
+//! vendor/product strings to a known value (QEMU/KVM, Hyper-V). It does not
+//! use the CPUID hypervisor-present bit, since a hardened or nested
+//! hypervisor can mask both signals; this is a best-effort heuristic, not a
+//! guarantee.
+
+use anyhow::Context;
+
+/// Path to DMI file with vendor information.
+const DMI_SYS_VENDOR_FILE: &str = "/sys/class/dmi/id/sys_vendor";
+
+/// Path to DMI file with product name.
+const DMI_PRODUCT_NAME_FILE: &str = "/sys/class/dmi/id/product_name";
 
 /// Does a best-effort check to determine whether we are running in a virtdeploy VM.
 pub fn is_virtdeploy() -> bool {
@@ -14,3 +34,51 @@ pub fn is_virtdeploy() -> bool {
 
     false
 }
+
+/// Returns the detected hypervisor name (e.g. `"qemu"`, `"hyperv"`), or
+/// `None` if the DMI vendor/product strings were read successfully but did
+/// not match a known hypervisor (including physical hardware).
+///
+/// Returns `Err` whenever a required DMI file could not be read, so callers
+/// that want to report collection failures (see
+/// `trident::diagnostics::get_virtualization_info`) can distinguish
+/// "unreadable" from "read fine, not virtualized". The error carries the
+/// failing file's path via context, so it remains actionable after being
+/// formatted/logged.
+pub fn detect_hypervisor() -> Result<Option<String>, anyhow::Error> {
+    let vendor = std::fs::read_to_string(DMI_SYS_VENDOR_FILE)
+        .with_context(|| format!("Failed to read '{DMI_SYS_VENDOR_FILE}'"))?
+        .trim()
+        .to_lowercase();
+    if vendor.contains("qemu") {
+        return Ok(Some("qemu".to_string()));
+    }
+
+    let product = std::fs::read_to_string(DMI_PRODUCT_NAME_FILE)
+        .with_context(|| format!("Failed to read '{DMI_PRODUCT_NAME_FILE}'"))?
+        .trim()
+        .to_lowercase();
+    if vendor.contains("microsoft corporation") && product.contains("virtual machine") {
+        return Ok(Some("hyperv".to_string()));
+    }
+
+    Ok(None)
+}
+
+/// Convenience wrapper over [`detect_hypervisor`] for callers (e.g.
+/// telemetry) that only care whether this is a VM, not which hypervisor,
+/// and don't need to distinguish "unreadable DMI files" from "not a VM" --
+/// both cases are reported as `false`.
+pub fn is_virtual() -> bool {
+    detect_hypervisor().unwrap_or(None).is_some()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_is_virtual_matches_detect_hypervisor() {
+        assert_eq!(is_virtual(), detect_hypervisor().unwrap_or(None).is_some());
+    }
+}

crates/trident/src/diagnostics.rs

@@ -18,6 +18,7 @@ use osutils::{
     findmnt::FindMnt,
     lsblk,
     pcrlock::{self, LogOutput},
+    virt,
 };
 use trident_api::{
     config::{Check, Health},
@@ -35,12 +36,6 @@ use crate::{
 /// Name of the top-level directory in the diagnostics tarball
 const DIAGNOSTICS_BUNDLE_PREFIX: &str = "trident-diagnostics";
 
-/// Path to DMI file with vendor information
-const DMI_SYS_VENDOR_FILE: &str = "/sys/class/dmi/id/sys_vendor";
-
-/// Path to DMI file with product name
-const DMI_PRODUCT_NAME_FILE: &str = "/sys/class/dmi/id/product_name";
-
 /// Name of the trident systemd service
 const TRIDENT_SERVICE_NAME: &str = "trident.service";
 
@@ -330,33 +325,13 @@ fn collect_service_status(
 
 fn get_virtualization_info(failures: &mut Vec<CollectionFailure>) -> Option<String> {
     debug!("Collecting virtualization info");
-    let content = match fs::read_to_string(DMI_SYS_VENDOR_FILE) {
-        Ok(c) => c,
+    match virt::detect_hypervisor() {
+        Ok(virt_type) => virt_type,
         Err(e) => {
-            record_failure(failures, "virtualization info (dmi_sys vendor)", &e);
-            return None;
-        }
-    };
-
-    let vendor = content.trim().to_lowercase();
-    if vendor.contains("qemu") {
-        return Some("qemu".to_string());
-    }
-
-    let product = match fs::read_to_string(DMI_PRODUCT_NAME_FILE) {
-        Ok(p) => p,
-        Err(e) => {
-            record_failure(failures, "virtualization info (dmi_sys product)", &e);
-            return None;
+            record_failure(failures, "virtualization info", &e);
+            None
         }
-    };
-
-    let product = product.trim().to_lowercase();
-    if vendor.contains("microsoft corporation") && product.contains("virtual machine") {
-        return Some("hyperv".to_string());
     }
-
-    None
 }
 
 struct FileToCollect {

crates/trident/src/lib.rs

@@ -320,7 +320,17 @@ impl Trident {
             ));
         }
 
-        tracing::info!(metric_name = "trident_start");
+        // See `logging::tracestream::clock_boottime_uptime_secs` for the
+        // NaN-fallback rationale and its regression test.
+        let uptime_secs = logging::tracestream::clock_boottime_uptime_secs(
+            nix::time::clock_gettime(nix::time::ClockId::CLOCK_BOOTTIME),
+        );
+        // `acl` and `arch` are process-lifetime, host-level facts (like
+        // `vm`), so they're stamped onto every telemetry event via
+        // `PLATFORM_INFO` (see
+        // `logging::tracestream::populate_platform_info`) instead of
+        // being scoped to just this one metric.
+        tracing::info!(metric_name = "trident_start", value = uptime_secs,);
 
         Ok(Self {
             host_config,

crates/trident/src/logging/tracestream.rs

@@ -23,14 +23,41 @@ use tracing_subscriber::{layer::Layer, registry::LookupSpan};
 use crate::command_kind::CommandKind;
 use trident_api::error::TridentError;
 
+/// Reads `CLOCK_BOOTTIME` (nanosecond-resolution time since boot,
+/// including any suspended time -- unlike `sysinfo::System::uptime()` or a
+/// naive `/proc/uptime` parse, which only expose whole-second resolution)
+/// and converts it into the `trident_start` uptime metric value. Used by
+/// `Trident::new`'s startup telemetry (see the `trident_start` call site
+/// in `lib.rs`), which passes it the live `nix::time::clock_gettime`
+/// result.
+///
+/// Best-effort: `clock_gettime` with a valid clock ID essentially never
+/// fails on Linux, but falls back to `f64::NAN` -- which `json!()`
+/// serializes as JSON `null`, a genuine "not available" rather than a
+/// misleading literal zero (see
+/// `test_clock_boottime_uptime_secs_nan_fallback_serializes_as_json_null`
+/// below) -- rather than failing startup if it somehow does.
+pub(crate) fn clock_boottime_uptime_secs(
+    clock_read: Result<nix::sys::time::TimeSpec, nix::errno::Errno>,
+) -> f64 {
+    clock_read
+        .map(|ts| std::time::Duration::from(ts).as_secs_f64())
+        .unwrap_or_else(|e| {
+            warn!("Failed to read CLOCK_BOOTTIME: {e}");
+            f64::NAN
+        })
+}
+
 use osutils::{
     dmi, files,
     osrelease::{OsRelease, OS_RELEASE_PATH},
-    uname,
+    uname, virt,
 };
+use sysdefs::arch::SystemArchitecture;
 
 use crate::{
-    datastore::DataStore, logging::operation_context, TRIDENT_METRICS_FILE_PATH, TRIDENT_VERSION,
+    datastore::DataStore, init::acl, logging::operation_context, TRIDENT_METRICS_FILE_PATH,
+    TRIDENT_VERSION,
 };
 
 lazy_static::lazy_static! {
@@ -802,6 +829,30 @@ fn populate_platform_info() -> BTreeMap<String, Value> {
         "unknown".to_string()
     });
     platform_info.insert("kernel_version".to_string(), json!(kernel_release.trim()));
+
+    // Whether this host is virtualized (see `osutils::virt` for the
+    // detection heuristic and its caveats).
+    platform_info.insert("vm".to_string(), json!(virt::is_virtual()));
+
+    let arch: &'static str = SystemArchitecture::current().into();
+    platform_info.insert("arch".to_string(), json!(arch));
+    // Best-effort: a failure to determine whether this is an ACL (Azure
+    // Container Linux) host must never fail startup, it only means this
+    // one field is missing from every telemetry event for this
+    // invocation. A detection failure is reported as "unknown", not
+    // "false" -- conflating "known non-ACL" with "couldn't tell" would
+    // misclassify a host whose ACL check simply failed to run as
+    // definitively non-ACL.
+    let acl = match acl::is_acl() {
+        Ok(true) => "true",
+        Ok(false) => "false",
+        Err(e) => {
+            warn!("Failed to determine if host is running ACL: {e:?}");
+            "unknown"
+        }
+    };
+    platform_info.insert("acl".to_string(), json!(acl));
+
     platform_info
 }
 
@@ -937,6 +988,63 @@ mod tests {
         );
     }
 
+    #[test]
+    /// Regression test for a Copilot review finding on PR #778: claimed
+    /// that `clock_boottime_uptime_secs`'s `f64::NAN` fallback, passed
+    /// through `tracing::info!(value = ...)` to
+    /// `TraceEntryVisitor::record_f64`'s `json!(value)`, would make
+    /// `serde_json` unable to serialize the entry, panicking instead of
+    /// staying best-effort. Calls the exact function `lib.rs`'s
+    /// `trident_start` metric uses (see its call site), forcing the
+    /// failure branch with a synthetic `Errno`, then sends the resulting
+    /// value through the real tracing -> visitor -> file-write pipeline
+    /// (not just a standalone `serde_json` snippet): `json!(f64)` goes
+    /// through `Value::from(f64)`, which maps non-finite floats to
+    /// `Value::Null` rather than erroring, so the written line contains a
+    /// valid JSON `"value":null` and the file write completes normally.
+    fn test_clock_boottime_uptime_secs_nan_fallback_serializes_as_json_null() {
+        let uptime_secs = clock_boottime_uptime_secs(Err(nix::errno::Errno::EINVAL));
+        assert!(
+            uptime_secs.is_nan(),
+            "a failed clock read must fall back to NaN, not a misleading literal value"
+        );
+
+        let temp_dir = tempfile::tempdir().unwrap();
+        let metrics_path = temp_dir.path().join("metrics.jsonl");
+        let tracestream = TraceStream::default();
+        let trace_sender = tracestream
+            .make_trace_sender_with_metrics_path(metrics_path.to_str().unwrap())
+            .with_filter(filter::LevelFilter::INFO);
+
+        let _guard = tracing::subscriber::set_default(
+            tracing_subscriber::Registry::default().with(trace_sender),
+        );
+
+        tracing::info!(metric_name = "trident_start", value = uptime_secs);
+
+        std::thread::sleep(std::time::Duration::from_millis(100));
+
+        let file = File::open(&metrics_path).unwrap();
+        let reader = BufReader::new(file);
+        let lines: Vec<String> = reader.lines().map(|l| l.unwrap()).collect();
+
+        let entry_line = lines
+            .iter()
+            .find(|line| line.contains(r#""metric_name":"trident_start""#))
+            .expect("expected trident_start metric not found in the local metrics file");
+
+        // Confirm the line is valid, parseable JSON (i.e. writing a NaN
+        // value never broke serialization) and that the value field landed
+        // as JSON null rather than a literal 0 or a serialization failure.
+        let parsed: serde_json::Value =
+            serde_json::from_str(entry_line).expect("trace entry line must be valid JSON");
+        assert_eq!(
+            parsed.get("value"),
+            Some(&Value::Null),
+            "NaN metric value should serialize as JSON null, not a literal 0 or an error"
+        );
+    }
+
     #[test]
     /// Regression test: `TraceStream::set_installation_id` must actually
     /// reach the serialized trace entry's `additional_fields.installation_id`
@@ -1309,13 +1417,31 @@ mod functional_test {
             "kernel_version".to_string(),
             json!(uname::kernel_release().unwrap().trim()),
         );
+        expected_platform_info.insert("vm".to_string(), json!(virt::is_virtual()));
 
         // Call the function to get the actual result.
         let platform_info = populate_platform_info();
 
-        // Assert that the actual result matches the expected result.
+        let expected_arch: &'static str = SystemArchitecture::current().into();
+        assert_eq!(platform_info.get("arch").unwrap(), &json!(expected_arch));
+        // Host-dependent (like the fields above): just assert the field is
+        // present and one of the values `acl::is_acl()` can actually
+        // produce, rather than a fixed expectation, since whether the VM
+        // running this test is an ACL host isn't controlled by the test.
+        let acl = platform_info.get("acl").unwrap().as_str().unwrap();
+        assert!(
+            ["true", "false", "unknown"].contains(&acl),
+            "unexpected acl value: {acl}"
+        );
+
+        // Assert that the actual result matches the expected result for
+        // every field with a fixed expectation (arch/acl are asserted
+        // separately above since they are host-dependent).
+        let mut actual_platform_info = platform_info;
+        actual_platform_info.remove("arch");
+        actual_platform_info.remove("acl");
         assert_eq!(
-            platform_info, expected_platform_info,
+            actual_platform_info, expected_platform_info,
             "Platform info does not match the expected result"
         );
     }

docs/Reference/Telemetry.md

@@ -44,6 +44,19 @@ host along with the metrics/spans themselves:
 - `kernel_version`: the running kernel release (`uname -r`).
 - `total_cpu`: the number of CPUs.
 - `total_memory_gib`: total memory, in GiB.
+- `vm`: whether the host appears to be a virtual machine, detected from DMI
+  vendor/product strings (`/sys/class/dmi/id/sys_vendor`,
+  `/sys/class/dmi/id/product_name`). Best-effort: reports `false` if the
+  DMI files are unreadable or the hypervisor is not one of the recognized
+  vendors, not necessarily "definitely bare metal".
+- `arch`: the CPU architecture Trident is running on, as reported by
+  `SystemArchitecture` (e.g. `amd64`, `arm64` -- not the kernel's own
+  `x86_64`/`aarch64` naming).
+- `acl`: whether the host is running Azure Container Linux (ACL), as a
+  string -- `"true"`, `"false"`, or `"unknown"` if detection itself
+  failed. Best-effort, like `vm` above: an `"unknown"` detection failure
+  is kept distinct from a known negative, rather than folded into
+  `"false"`.
 - `trident_version`: the running Trident version.
 - `installation_id`: an ID that lets separate events be correlated back to
   the same host installation over time.