telemetry: additional metrics (error, consistent start/success) - #778
Draft
bfjelds (bfjelds) wants to merge 6 commits into
Draft
Conversation
…rollback_runtime_success metrics Moved here from PR 774 (which keeps just the correlation_id/operation_id infrastructure) so all new-metric additions -- including the new command_error metric added in this PR -- live together. - manual_rollback_start (engine/manual_rollback/mod.rs::execute_rollback): fired unconditionally on every invocation (stage-only/finalize-only/ combined), mirroring update_start placement/semantics. Required adding #[derive(Debug, Clone, Copy)] to ManualRollbackRequestKind (had no derives at all). - runtime_update_success (engine/runtime_update.rs::finalize_update): fired synchronously on success, since runtime update needs no reboot and so never goes through engine::rollback post-reboot boot-validation flow where clean_install_success/ab_update_success/ manual_rollback_success are fired. - manual_rollback_runtime_success (engine/manual_rollback/mod.rs:: finalize_rollback): closes a parallel gap -- manual rollback of a *runtime* update also never reached the boot-validation flow, so it had no success metric at all (only the A/B rollback case did, via manual_rollback_success). cargo build -p trident, cargo test -p trident --lib: 417/417 pass (unchanged from before the move).
file:line where the error was originally constructed was already tracked internally (used by Serialize, Debug, and the ProtoTridentError conversion) but had no public getter. Needed by the upcoming command_error telemetry metric, which reports kind/subkind/location for every command that fails.
…subkind/location Works for both CLI and gRPC/daemon, since both already funnel through operation_context (from PR 774): - New run_command<T>() in operation_context.rs: same as run_with_operation, but specifically for the Result<T, TridentError> shape both funnel points actually use. Fires a command_error metric (kind/subkind/ location) if the wrapped closure returns Err, while operation_id/command context is still active -- so it's correlated the same way command_start already is. - main.rs's CLI dispatch (5 call sites: install/update/commit/rollback/ rebuild_raid) and server/tridentserver/mod.rs's servicing_request() (covers every gRPC operation from one call site) both switched from run_with_operation to run_command. - trident_api::error::TridentError::location(): new public accessor for the file:line the error was originally constructed at (already tracked internally for Serialize/Debug/ProtoTridentError, just had no getter). - docs/Reference/Agent-Configuration.md: document the new command_error event and its kind/subkind/location fields. Verified: cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 421/421 pass (8 new in operation_context, including a test using a capturing tracing_subscriber::Layer to assert command_error actually carries the right kind/subkind/location for a constructed TridentError).
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
bfjelds (bfjelds)
changed the title
This was referenced Sep 4, 2026
Tags every trident_start event with whether the host is running the CIH (Azure Container Linux) image, using the existing cih::is_cih() check already used elsewhere in Trident::update(). Best-effort: a failure to determine this reports false rather than failing startup. Not documented in Agent-Configuration.md: that section only covers fields attached to every event (host metadata) and the dedicated command_error case -- acl is a one-off field on a single event, and carving out a doc entry for every future one-off metric field would bloat it. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass.
…nfo::System::uptime() sysinfo::System::uptime() truncates /proc/uptime to whole seconds (splits on the decimal point and discards the fractional part entirely, confirmed in its vendored source -- not even a rounding, a hard truncation). Switched to nix::time::clock_gettime(ClockId::CLOCK_BOOTTIME), which reads the kernel clock directly at nanosecond resolution and correctly includes any suspended time (unlike CLOCK_MONOTONIC on some kernels). Converted via the crates already-available impl From<TimeSpec> for Duration, then .as_secs_f64() for a fractional- seconds value. Enabled nix's time feature (previously default-features = false with only fs/user/socket/signal enabled) in the workspace Cargo.toml. Best-effort: clock_gettime with a valid clock ID essentially never fails on Linux, but falls back to 0.0 with a warning rather than failing startup if it somehow does. cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass.
0.0 is indistinguishable from a real (if implausible) zero-second uptime. f64::NAN is a genuine \ not available\ sentinel here: verified against the vendored serde_json source that both Value::from(f64) (used by every json!(value) call in tracestream.rs/appinsights.rs) and the actual JSON serializer's serialize_f64 treat non-finite floats as Value::Null / JSON null respectively -- no error, no special-casing needed anywhere in this codebase's telemetry pipeline. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
CLI failure telemetry remains incomplete, and ACL detection failures are incorrectly reported as negative results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds expanded telemetry for command failures, servicing lifecycle events, and startup context.
Changes:
- Adds correlated
command_errormetrics for CLI and gRPC operations. - Adds manual rollback and runtime-update lifecycle metrics.
- Enriches startup telemetry with ACL detection and system uptime.
File summaries
| File | Description |
|---|---|
Cargo.toml |
Enables Nix time APIs. |
docs/Reference/Agent-Configuration.md |
Documents command-error telemetry. |
crates/trident/src/main.rs |
Instruments CLI servicing commands. |
crates/trident/src/lib.rs |
Re-exports instrumentation and enriches startup metrics. |
crates/trident/src/logging/operation_context.rs |
Implements and tests command-error reporting. |
crates/trident/src/server/tridentserver/mod.rs |
Instruments gRPC servicing commands. |
crates/trident/src/engine/runtime_update.rs |
Emits runtime-update success. |
crates/trident/src/engine/manual_rollback/mod.rs |
Emits rollback start and runtime success. |
crates/trident/src/engine/manual_rollback/utils.rs |
Makes rollback request kinds loggable. |
crates/trident_api/src/error.rs |
Exposes error-origin locations. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Ok(is_cih) => is_cih, | ||
| Err(e) => { | ||
| warn!("Failed to determine if host is running CIH: {e:?}"); | ||
| false |
| } => { | ||
| let ops = cli::to_operations(allowed_operations); | ||
| run_with_operation(&command_name("install", &ops), || { | ||
| run_command(&command_name("install", &ops), || { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement (
manual_rollback_start,runtime_update_success,manual_rollback_runtime_success) to be consistent with other operation metrics.Adds a new
command_errormetric that fires whenever a command fails, for both CLI and gRPC/daemon, included in metric:kind: top-level error category (e.g.internal,invalid-input,servicing)subkind: the specific error within that category, when one applieslocation:file:linewhere the error was originally raisedRelated PRs in stack: