telemetry: additional metrics (error, consistent start/success)

[bfjelds (bfjelds)]

Implement (manual_rollback_start, runtime_update_success, manual_rollback_runtime_success) to be consistent with other operation metrics.

Adds a new command_error metric 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 applies
• location: file:line where the error was originally raised

Related PRs in stack:

• datastore: add generic key-value storage and persisted correlation-id #773
• telemetry: compile-in Azure Monitor connection string, best-effort AppInsights tracing #774
• [this PR] telemetry: additional metrics (error, consistent start/success) #778
• telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id #779

[azure-pipelines]

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.

[Copilot]

🟡 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_error metrics 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.

[Copilot]

🟡 Changes recommended

CLI startup events lose correlation metadata, and runtime-update success is omitted from persisted metrics archives.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Correlation-ID pre-warming creates missing datastores and bypasses the host-provisioning guard for several CLI commands.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Initial install/update telemetry can remain uncorrelated, and runtime rollback persistence is duplicated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/trident/src/main.rs:202

• Issue: The missing-datastore check also skips pre-warming for install and update, so the first such CLI invocation still emits command_start without a correlation_id. Evidence: Those commands are intentionally exempt from the provisioning guard below, and Trident::new creates the ID only after run_command has already emitted command_start. Suggestion: Skip creation only for commands that require an existing datastore; allow install/update to create and attach the ID before entering run_command.

                    if !agent_config.datastore_path().exists() {
                        return Ok(None);

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Several non-servicing CLI failures still bypass the promised command_error telemetry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Read-only and response-encoded gRPC failures remain outside the new command-error instrumentation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Several newly wrapped CLI and read-RPC paths still omit telemetry setup or persisted correlation metadata.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/trident/src/server/tridentserver/mod.rs:381

• Issue: Read RPCs can still lose the persisted correlation ID that the new command events are supposed to carry. Evidence: if the daemon starts before the datastore exists and another process later creates it, only servicing_request re-checks the datastore (lines 209-245); reading_request reaches run_command directly here, so its command_start/command_error remain uncorrelated until a write RPC happens. Suggestion: factor the pre-warm/re-check into a shared helper and call it before both servicing and reading command wrappers.

        let f = move || operation_context::run_command(name, f);

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Diagnostic metrics are truncated, and malformed gRPC requests still bypass command-error telemetry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/trident/src/server/tridentserver/mod.rs:264

• Issue: This wrapper still misses concrete gRPC command failures, so the advertised daemon coverage is incomplete. Evidence: multiple handlers reject malformed requests before calling servicing_request—for example install/update return Status::invalid_argument for missing stage/config/finalize fields, rollback does the same for missing stage/finalize, and streaming rejects an invalid URL. Those requests emit neither command_start nor command_error. Suggestion: establish command telemetry around the full RPC handler (for example via middleware), or move request validation into a common wrapped path and map these validation failures to the documented error dimensions.

        let f = move || operation_context::run_command(name, f);

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Invalid configurations can create datastores prematurely, while validation RPC failures still bypass command-error telemetry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

crates/trident/src/server/tridentserver/mod.rs:230

• Issue: The same pre-warm side effect affects first install/update RPCs with invalid configuration content. Evidence: refresh_correlation_id creates the datastore before the request closure runs, but the service handlers parse their raw host configuration only inside Trident::new; a parse failure therefore leaves behind a datastore that would not otherwise have been created. Suggestion: validate the request configuration before permitting this helper to create a missing datastore, or defer persistence of the new correlation ID until validation succeeds.

        let can_initialize_datastore = name.starts_with("install") || name.starts_with("update");
        match AgentConfig::load().and_then(|agent_config| {
            if !can_initialize_datastore && !agent_config.datastore_path().exists() {
                return Ok(None);
            }
            DataStore::open_or_create(agent_config.datastore_path())
                .and_then(|mut ds| ds.correlation_id())
                .map(Some)

crates/trident/src/server/tridentserver/mod.rs:397

• Issue: The stated Ok-encoded failure gap already exists, so gRPC failures are still not fully covered. Evidence: validate_host_configuration in services/validation.rs:32-38 converts a TridentError into Ok(ValidateHostConfigurationResponse { ok: false, error }) and bypasses reading_request, meaning it emits neither command_start nor command_error. Suggestion: run that RPC under an operation context and report its response-level validation error, or refactor the common wrapper to support this response shape.

        // Known remaining gap: an RPC that encodes a domain-level failure
        // in an `Ok` response instead of returning `Err(TridentError)` (if
        // any -- none currently do) would still not get a command_error,
        // since that only fires on `Err`.

crates/trident/src/server/tridentserver/mod.rs:230

• Issue: Reloading AgentConfig here can attach telemetry to a different datastore than the request actually uses. Evidence: every service closure uses the self.agent_config captured when TridentServer was constructed, but this helper rereads the on-disk configuration; if that file changes during the daemon lifetime, command_start and subsequent metrics can receive the correlation ID from the new path while servicing still targets the old path. Suggestion: use self.agent_config.datastore_path() consistently with the request handlers.

        match AgentConfig::load().and_then(|agent_config| {
            if !can_initialize_datastore && !agent_config.datastore_path().exists() {
                return Ok(None);
            }
            DataStore::open_or_create(agent_config.datastore_path())
                .and_then(|mut ds| ds.correlation_id())
                .map(Some)

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Telemetry initialization can erase prior metrics, omit existing correlation IDs, and create datastores for invalid requests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/trident/src/main.rs:135

• Issue: Invalid install/update configurations return before the persisted correlation ID is loaded. Evidence: these early run_command calls fire command_start/command_error before the pre-warm at lines 165–175; even when the datastore already exists, both events therefore omit the installation's correlation_id, contrary to the documented per-event metadata. Suggestion: load from an existing datastore before this preflight, while still deferring datastore creation until parsing succeeds when the datastore is absent.

    if let Some(path) = &config_path {
        if !path.exists() {
            return run_command(&command, || {

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Balanced