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

[bfjelds (bfjelds)]

Implement (manual_rollback_start, runtime_update_success, manual_rollback_runtime_success, stream_image_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 database-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 #779
• telemetry: enable Telemetry=OptIn by default for test/dev images #781

[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]

🟢 Approval recommended

The telemetry paths, failure classification, persistence ordering, and regression coverage are coherent with no unresolved blocking issue identified.

Review details

• Files reviewed: 20/20 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Command coverage, gRPC timing, transport classification, and archived failure telemetry remain inconsistent.

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

Review details

• Files reviewed: 20/20 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Several command paths remain uninstrumented, and gRPC-client telemetry currently starts after the operation completes.

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

Review details

Suppressed comments (2)

crates/trident/src/grpc_client/mod.rs:41

• Issue: The client operation finishes before its command wrapper starts. Evidence: runtime.block_on(run_client(args)) performs the connection and complete RPC first; only afterward does run_command_if emit command_start, so the start timestamp is after completion, client-side events lack the operation context, and a panic in run_client bypasses panic reporting. Suggestion: execute runtime.block_on(run_client(args)) inside the wrapper closure and derive the reporting decision from the resulting wrapped error.

    let command = args.command.name().replace('-', "_");
    let client_result = runtime.block_on(run_client(args));

crates/trident/src/grpc_client/mod.rs:57

• Issue: This predicate does not recognize the documented “connection dropped mid-call” case. Evidence: it only matches ConnectionError, which is created during the initial connect; failures after connection are wrapped as RequestError(Status) or ResponseError(Status) in tridentclient.rs, so they produce no client-side command_error even though the daemon may never have reported one. Suggestion: classify transport-originated request/stream statuses separately from statuses actually returned by the daemon and report the former.

    let is_transport_failure = client_result.as_ref().err().is_some_and(|e| {
        e.chain().any(|cause| {
            matches!(
                cause.downcast_ref::<TridentClientError>(),
                Some(TridentClientError::ConnectionError(..))
            )

• Files reviewed: 20/20 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Several command paths remain uninstrumented, and gRPC-client telemetry is emitted after execution with incomplete transport-failure classification.

Review details

Suppressed comments (8)

Previously missed (3) — in code that hasn't changed since the last review.

crates/trident/src/grpc_client/mod.rs:40

• Issue: The new gRPC-client command tag does not identify the RPC actually dispatched. Evidence: ClientCommands::name() returns compatibility names such as client-update, and this also ignores allowed_operations; a stage-only update is sent as update_stage by run_client but tagged here as client_update, unlike the daemon/direct-CLI update_stage convention. Suggestion: derive the telemetry name from the selected RPC/stage-finalize operations and rely on source = grpc-client rather than encoding the client in command.
  crates/trident/src/main.rs:190
• Issue: command_start still lacks database_id even when an existing datastore already has one. Evidence: this preflight attaches only installation_id; attach_database_id_if_present is not called until Trident::new (lib.rs:286-294), after run_command has emitted the start event. The daemon startup/per-request refresh has the same omission, despite Telemetry.md stating every event includes database_id. Suggestion: pre-attach the existing datastore's database ID alongside the installation ID before emitting command telemetry in both entry points.
  crates/trident/src/server/tridentserver/mod.rs:211
• Issue: Per-request telemetry can attach an installation ID from a different datastore than the request actually uses. Evidence: servicing handlers use the self.agent_config loaded for this server, but this helper reloads configuration from disk; if the configured datastore path changes while the daemon is running, telemetry refreshes from the new path while the request still operates on the original one. A reload failure also needlessly skips refresh despite the server already having valid configuration. Suggestion: use the server's existing configuration.

docs/Reference/Telemetry.md:53

• Issue: This documented guarantee is not implemented for all command failures. Evidence: run_trident deliberately returns from validate, get, diagnose, offline-initialize, rollback --check, and start-network before run_command (main.rs:65-147), and daemon reading_request likewise executes failures without the wrapper (server/tridentserver/mod.rs:390-407). This reintroduces the coverage gaps that earlier revisions addressed and conflicts with the PR description's “whenever a command fails.” Suggestion: Route these command paths through the error wrapper, or narrow both the PR contract and this documentation to the subset that is actually instrumented.

If a command fails, a `command_error` event is also sent (tagged with the
same `operation_id`/`command` as above), breaking the failure down into:

crates/trident/src/grpc_client/mod.rs:41

• Issue: command_start is emitted only after the gRPC-client command has already completed. Evidence: runtime.block_on(run_client(args)) runs before run_command_if, so the operation context is absent throughout connection/request processing; a panic there also bypasses run_command_if's panic reporting. Suggestion: Execute run_client inside the command wrapper so start/error telemetry brackets the real invocation.

    let command = args.command.name().replace('-', "_");
    let client_result = runtime.block_on(run_client(args));

crates/trident/src/grpc_client/mod.rs:57

• Issue: The transport predicate misses the documented “dropped mid-call” case. Evidence: a failure while awaiting the initial RPC is wrapped as TridentClientError::RequestError, and a stream read failure is wrapped as ResponseError (tridentclient.rs:242-249,269-284); this match recognizes only the initial connect() failure. Those dropped-call failures therefore produce no client-side command_error, even though the daemon may never have reported one. Suggestion: classify transport-derived request/response statuses as transport failures while continuing to suppress statuses confirmed to be daemon responses.

    let is_transport_failure = client_result.as_ref().err().is_some_and(|e| {
        e.chain().any(|cause| {
            matches!(
                cause.downcast_ref::<TridentClientError>(),
                Some(TridentClientError::ConnectionError(..))
            )

crates/trident/src/server/tridentserver/services/mod.rs:48

• Issue: The new telemetry location for every missing request field points to this shared helper rather than the service call site that detected the missing field. Evidence: TridentError::new is #[track_caller], but this wrapper is not, so all commands collapse to services/mod.rs:50; for fields within the same command, kind, subkind, and location then become identical. Suggestion: propagate the caller location through this helper.

    fn reject_invalid_argument(
        &self,
        command: &str,
        field: &str,
        message: impl Into<String>,
    ) -> Status {

crates/trident/src/server/tridentserver/services/mod.rs:82

• Issue: Invalid-field telemetry similarly records this helper as the error origin instead of the rejecting service call site. Evidence: the tracked TridentError::new call receives this untracked wrapper as its immediate caller, so all parse failures report services/mod.rs:84. Suggestion: propagate the original caller location.

    fn reject_invalid_field(
        &self,
        command: &str,
        field: &str,
        reason: impl Into<String>,
        message: impl Into<String>,
    ) -> Status {

• Files reviewed: 20/20 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Error telemetry currently misclassifies daemon admission responses and includes excluded read-only operations.

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

Review details

• Files reviewed: 20/20 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Command names remain inconsistent across entry points, and some daemon-generated servicing failures bypass error telemetry.

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

Review details

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

[Copilot]

🟡 Changes recommended

Reboot error reporting can leak its deduplication flag and suppress a later command’s error metric.

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

Review details

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

[Copilot]

🟡 Changes recommended

Invalid-request handlers perform potentially blocking SQLite refreshes before entering their blocking sections.

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

Review details

Suppressed comments (1)

crates/trident/src/server/tridentserver/services/mod.rs:94

• Issue: This second rejection path also performs synchronous SQLite refresh work before entering block_in_place. Evidence: refresh_ids may block for the configured five-second busy timeout, so concurrent invalid-URL requests can stall the async runtime workers. Suggestion: perform the refresh as the first operation inside the existing blocking section.

        self.refresh_ids();

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

[Copilot]

🟡 Changes recommended

Mixed append and non-append writers can overwrite concurrent gRPC-client metrics and corrupt the JSONL file.

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

Review details

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

[Copilot]

🟡 Changes recommended

Read-only grpc-client rollback --check transport failures are incorrectly reported as servicing command errors.

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

Review details

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

[Copilot]

🟡 Changes recommended

The public telemetry documentation incorrectly claims all daemon rejections already emit command_error.

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

Review details

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