telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id - #779
Conversation
|
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. |
| --filepath "$METRICS_FILE" \ | ||
| --mapping "$KUSTO_TABLE_MAPPING" | ||
| --mapping "$KUSTO_TABLE_MAPPING" \ | ||
| --ignore-schema |
There was a problem hiding this comment.
adding ignore-schema because we removed asset_id which is a required field in the telemetry schema.
other options would be:
- remove asset_id from required list in schema
- use asset_id as installation_id
1599407 to
c96c0af
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Install finalization overwrites the staged servicing ID, breaking cross-invocation correlation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates telemetry identifiers to improve privacy and correlate servicing operations across invocations.
Changes:
- Removes hardware-derived
asset_idand renamescorrelation_idtoinstallation_id. - Persists and emits per-servicing-operation
servicing_idvalues. - Adjusts telemetry documentation and Kusto ingestion.
File summaries
| File | Description |
|---|---|
docs/Reference/Agent-Configuration.md |
Documents telemetry identifiers. |
crates/trident/src/main.rs |
Passes the installation-ID handle to App Insights. |
crates/trident/src/logging/tracestream.rs |
Emits installation and servicing IDs. |
crates/trident/src/logging/operation_context.rs |
Tracks servicing IDs per invocation. |
crates/trident/src/logging/appinsights.rs |
Adds identifiers to App Insights events. |
crates/trident/src/lib.rs |
Creates and restores identifiers during servicing. |
crates/trident/src/engine/update.rs |
Creates servicing IDs for updates. |
crates/trident/src/engine/manual_rollback/mod.rs |
Handles rollback servicing IDs. |
crates/trident/src/datastore.rs |
Persists installation and servicing IDs. |
.pipelines/templates/stages/testing_common/scripts/upload_metrics.sh |
Uploads metrics without schema validation. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a97f856 to
a2e921d
Compare
c96c0af to
bcab5b0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Servicing IDs are assigned before rollback eligibility is known and are not restored for post-reboot commit metrics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
bcab5b0 to
941e873
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The core servicing-ID serialization path lacks sink-level regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
f6f0da6 to
5e7b215
Compare
941e873 to
6f8944b
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The updated diff addresses the prior lifecycle and serialization gaps, with no remaining blocking correctness issues found.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
5e7b215 to
e6ac4c4
Compare
6f8944b to
7256f43
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Some daemon read requests and the initial CIH bootstrap update still emit telemetry without the intended installation ID.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
fda62f2 to
caaec51
Compare
3c922ef to
58467cb
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
CIH bootstrap telemetry does not attach its newly persisted installation ID to the active trace stream.
Review details
Suppressed comments (1)
crates/trident/src/lib.rs:763
- Issue: The CIH bootstrap persists an installation ID but never attaches it to the shared
TraceStream, so this invocation'supdate_startand later events still omitinstallation_id. Evidence:Trident::newsawNotProvisionedand left the handle empty; this branch only callscreate_installation_id(), while telemetry reads the separate in-memory handle. Suggestion: cloneself.tracestreambefore enteringexecute_and_record_error, then callset_installation_idwith the successfully created ID here (asinstalldoes).
if let Err(e) = datastore.create_installation_id() {
warn!("Failed to create installation ID during CIH bootstrap: {e:?}");
}
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Drops the DMI product UUID (/sys/class/dmi/id/product_uuid) from every telemetry event. It was the one genuinely hardware-derived identifier in PLATFORM_INFO; installation_id (next commit) already gives a stable, random, non-hardware-derived per-host identifier for correlation, making asset_id redundant from a correlation standpoint while being needlessly more identifying. Removes read_product_uuid/PRODUCT_UUID_FILE and their tests entirely. (cherry picked from commit 65a83bd) (cherry picked from commit 309be9a) (cherry picked from commit 1f20777) (cherry picked from commit c536f89) (cherry picked from commit 5f25ac7) (cherry picked from commit 41b6e8f) (cherry picked from commit e9161ad)
…ll start, not datastore-creation); add persistent servicing_id installation_id (renamed from correlation_id): - DataStore::correlation_id() (get-or-create, called from Trident::new on every invocation) split into two: DataStore::installation_id() (read-only) and DataStore::create_installation_id() (get-or-create). - Trident::new() now only reads (Trident::installation_id()); it never creates one. Trident::install() is the only caller of create_installation_id(), at the start of staging (after any multiboot datastore swap, so a multiboot install's own new datastore gets its own ID). Every other command/gRPC request now simply attaches whatever was already stamped at install time, instead of the previous behavior where literally the first-ever Trident::new() call on a fresh datastore (regardless of which command triggered it) would mint the ID. - Added a `tracestream: TraceStream` field to the `Trident` struct so `install()` can attach the newly-created ID to telemetry (previously `Trident::new()` only used its `tracestream` parameter transiently). - TraceStream::set_correlation_id/correlation_id_handle -> set_installation_id/installation_id_handle; AppInsightsSender's correlation_id field/param renamed to match. JSON field renamed correlation_id -> installation_id. - New test: installation_id's read-only getter returns None before create_installation_id is called, Some after. New persistent servicing_id: - DataStore::new_servicing_id() (unconditional overwrite, unlike installation_id's insert-if-absent) + DataStore::servicing_id() (read-only) using the existing set_value/get_value generic key-value API (set_value's overwrite semantics were previously exercised only by tests; this is its first real caller). - Attached via operation_context (not threaded through every engine::* function signature): CURRENT_OPERATION's thread-local state extended from (operation_id, command) to also carry an optional servicing_id, set via the new operation_context::set_servicing_id(). Both TraceSender's and AppInsightsSender's field-merging logic updated to include it when present. - Created at the start of staging in all three places that stage: Trident::install() (alongside installation_id), engine::update::update() (at the same unconditional "start" point update_start already fires from), and engine::manual_rollback::execute_rollback()'s has_stage() branch (naturally gated to genuine staging, unlike install/update). finalize_rollback() reads the persisted ID back (via DataStore::servicing_id()) at its own start, so a resumed finalize-only invocation (e.g. after reboot) still tags its own metrics with the same servicing_id the staging invocation used. - Known simplification: install/update mint a fresh servicing_id on every invocation that reaches their "start" point, including a possible finalize-only re-invocation of update() (which, per its existing design, unconditionally re-runs staging logic on every call regardless of whether it's a true first stage or a resumed one) -- so unlike manual rollback, a genuinely separate stage-then-finalize update sequence would not currently share one servicing_id across both calls. Documented in code comments; not fixed here to avoid a much larger change to update()'s existing stage/finalize semantics. docs/Reference/Agent-Configuration.md updated: asset_id entry removed (handled in the prior commit), correlation_id -> installation_id with its new creation semantics, and a new servicing_id entry. Verified: cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass (new tests: installation_id read-only-getter test in datastore.rs; set_servicing_id tests in operation_context.rs). (cherry picked from commit efd1405) (cherry picked from commit ae5396d) (cherry picked from commit f11fa98) (cherry picked from commit 52fcc53) (cherry picked from commit b1e10a6) (cherry picked from commit e79950b) (cherry picked from commit d777978)
…ation Trident::update() has a case my earlier servicing_id placement in engine::update::update() completely missed: when the Host Configuration for this invocation is unchanged from what is already persisted (datastore.host_status().spec == host_config) and the servicing state is AbUpdateStaged/RuntimeUpdateStaged, it calls ab_update::finalize_update()/ runtime_update::finalize_update() *directly* -- bypassing engine::update::update() (and therefore my new_servicing_id() call) entirely. Every other update path does go through engine::update::update(), which is correctly gated by Trident::update() to only run when staging is actually happening (a changed config with has_stage(), or a retry of a failed/no-op update), so this was the one path a fresh servicing_id was never being created *or* read back for -- meaning a genuine, separate finalize-only continuation of a staged update (the real two-step A/B update flow, rebooting in between) would have no servicing_id at all. Fix: read the persisted servicing_id back (DataStore::servicing_id()) at the top of both direct-finalize match arms in Trident::update(), mirroring what execute_rollback's finalize_rollback already does. This is safe specifically because reaching those arms already required datastore.host_status().spec == host_config -- i.e. this invocation's config is provably identical to whatever was staged, so the persisted servicing_id is guaranteed to belong to *this* operation, not an unrelated one. Also corrected the inline comment on the new_servicing_id() call in engine::update::update() itself, which had incorrectly implied that function handles finalize-only re-invocations too. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass (unchanged -- no test exercises the true separate-process two-step update flow yet; this is a targeted correctness fix for a path today's test suite doesn't cover end-to-end). (cherry picked from commit 671bb3e) (cherry picked from commit bd4766b) (cherry picked from commit 10d3140) (cherry picked from commit ddf2577) (cherry picked from commit 5ba46af) (cherry picked from commit 1e52d68) (cherry picked from commit ba36459)
set_servicing_id() only affects events fired *after* it runs on this thread (operation_context is read at send-time, not retroactively) -- with new_servicing_id()/set_servicing_id() coming after update_start, that metric itself never carried the very servicing_id it establishes. Swapped the order so update_start (and everything else this invocation emits) is tagged with it. cargo build, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass. Note: the same ordering issue exists in engine::manual_rollback::execute_rollback (manual_rollback_start fires before new_servicing_id, unconditionally at the top of the function, before has_stage()/rollback-availability are even known) -- left as a follow-up since fixing it cleanly needs more thought: the metric fires before we know whether a fresh ID will be minted at all, so simply reading back any already-persisted ID first risks tagging the event with a stale ID from a prior, unrelated, already-completed operation. (cherry picked from commit da288b0) (cherry picked from commit 01739c3) (cherry picked from commit 086183f) (cherry picked from commit 58d1955) (cherry picked from commit 6acf33a) (cherry picked from commit 4ff8160) (cherry picked from commit e8f99a6)
…d (same ordering bug as update_start) manual_rollback_start fired unconditionally at the very top of execute_rollback, before has_stage()/rollback-availability were even known -- and the servicing_id mint (previously placed after rollback_type was determined, deep inside the has_stage() branch) happened well after that. Same root cause as the update_start fix: set_servicing_id only affects events emitted after it runs. Harder to fix than update_start though, since the metric fires before we know whether this call will actually stage anything. Restructured so the right servicing_id is determined first, then the metric fires: - has_stage() requested: mint a fresh servicing_id immediately (moved up from after rollback_type is determined). This now also covers the rare "staging requested but nothing available to roll back" early-return case, which previously never reached the old mint call at all -- a fresh "this attempt" ID for that case is reasonable, and a later genuine stage overwrites it again. - finalize-only (no has_stage()): read back whatever servicing_id is already persisted, mirroring finalize_rollback's existing read-back (a harmless redundant read if this same call both stages and finalizes). Removed the now-redundant later new_servicing_id() call inside the has_stage() branch. cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 422/422 pass. (cherry picked from commit 96d371a) (cherry picked from commit 93dd46e) (cherry picked from commit aac18bd) (cherry picked from commit fc8619f) (cherry picked from commit 2c7d8c6) (cherry picked from commit c791100) (cherry picked from commit 93b79e8)
Removing asset_id from the metrics/telemetry payload (earlier commit in this PR) breaks kusto_ingestor.py's schema check against the existing Kusto table mapping, which still expects that column. --ignore-schema skips that check so metrics upload keeps working without needing a coordinated Kusto table-mapping update in lockstep with this PR. (cherry picked from commit 277f9ae) (cherry picked from commit 6efffe0) (cherry picked from commit e7ccbf0) (cherry picked from commit ea153a5) (cherry picked from commit 90b2573) (cherry picked from commit 399be97) (cherry picked from commit 78e15b3)
Artifact of rebasing this branch onto PR 774's daemon pre-warm fix (added after the installation_id rename commit was originally authored, so that rename never touched this new code). Caught by the immediate build failure, not a real code change. (cherry picked from commit 1656ea5) (cherry picked from commit 951ab89) (cherry picked from commit 74e79cf) (cherry picked from commit 31298db) (cherry picked from commit 6d21cae) (cherry picked from commit 498b9f9) (cherry picked from commit 66c6fbd)
…tall_finalize Addresses Copilot review comment on PR 779: install_stage and install_finalize both call Trident::install(), and it unconditionally called new_servicing_id() before determining which of its three cases applied (genuinely new install, resuming a staged install, or retrying a failed/no-op one) -- so a separate install_finalize invocation resuming a CleanInstallStaged host would clobber the servicing_id install_stage had already minted and persisted, making the two halves uncorrelatable. Exactly the same class of bug as the update_finalize one fixed earlier in this PR. Moved servicing_id creation into only the two branches that genuinely start staging a new install (config changed + has_stage(), and NotProvisioned retry); the CleanInstallStaged (finalize-only) branch now reads the persisted servicing_id back via DataStore::servicing_id() instead, mirroring the update_finalize fix and manual rollback's finalize path. Safe for the same reason: reaching the CleanInstallStaged arm already requires datastore.host_status().spec == host_config, so the persisted ID is guaranteed to belong to *this* staged install, not an unrelated one. installation_id creation (get-or-create, idempotent) is unaffected -- still happens unconditionally before branching, since calling it repeatedly for the same datastore is harmless. cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 428/428 pass (unchanged -- no existing test exercises the true separate-process two-step install flow, same caveat as the update_finalize fix). (cherry picked from commit 7a6a6c0) (cherry picked from commit aca1f7a) (cherry picked from commit ce1e1d8) (cherry picked from commit f8c8cc9) (cherry picked from commit c8b0964) (cherry picked from commit 50bf0b3) (cherry picked from commit a9df91b)
Artifact of rebasing this branch onto PR 778's new CLI pre-warm fix (added after the installation_id rename commit was originally authored, so that rename never touched this new code). Caught by the immediate build failure, not a real code change. (cherry picked from commit 66595cb) (cherry picked from commit 011e94d) (cherry picked from commit 2699642) (cherry picked from commit 7bede3d) (cherry picked from commit 98408ed) (cherry picked from commit 5a8e103) (cherry picked from commit 56452c8)
…*_success metrics Addresses Copilot review comment on PR 779: A/B update, clean install, and manual A/B rollback all finalize with ExitKind::NeedsReboot, then Trident::commit() (in a separate, post-reboot process invocation) calls rollback::validate_boot, which fires the corresponding *_success metric (clean_install_success/ab_update_success/manual_rollback_success) -- but commit() never called servicing_id/set_servicing_id, so those completion events could never be correlated back to the servicing operation that produced them (unlike the finalize-only continuation paths for update/install/manual-rollback, which do this). Reads the persisted servicing ID back right after commit()'s early validity check (which already confirms servicing_state is one of the *Finalized/*HealthCheckFailed states), before calling validate_boot. cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 428/428 pass. (cherry picked from commit 98c7c08) (cherry picked from commit 16489a1) (cherry picked from commit da6e1ea) (cherry picked from commit 625dbc4) (cherry picked from commit 7bf6e0a) (cherry picked from commit 2c1c9ab) (cherry picked from commit 8ab67f9)
…ng a real rollback operation Addresses Copilot review comment on PR 779: my earlier fix for manual_rollback_start's missing servicing_id (moving ID determination to the top of execute_rollback, before the metric fires) went too far the other way -- it selected/persisted an ID before confirming a rollback could actually proceed. A stage request rejected for an invalid state, last_error set, or "no rollback available" still minted a fresh ID (and overwrote any previous one) even though nothing was staged. A finalize-only request from Provisioned (nothing staged) attached the *previous* operation's ID before being rejected, misattributing that telemetry. Restructured so validation comes first in both branches: - Stage path: mint the servicing ID and fire manual_rollback_start only after state/last_error validation *and* resolving a real rollback target (right where rollback_type becomes known) -- not before. - Finalize-only path: restore the persisted ID and fire manual_rollback_start only after confirming servicing_state actually matches a real ManualRollback*Staged state -- not before. Skipped entirely when this same call also staged above, since that branch already handled it for the combined operation. cargo build --workspace, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 428/428 pass. (cherry picked from commit 941e873) (cherry picked from commit 6d14023) (cherry picked from commit 01676a6) (cherry picked from commit 7a2733a) (cherry picked from commit 8675ffe) (cherry picked from commit a66aa12) (cherry picked from commit 1f262ba)
…ields Mirrors the existing installation_id regression test: wraps a metric event in run_with_operation + set_servicing_id and asserts the serialized metrics line carries additional_fields.servicing_id, so a future refactor of merge_operation_context cannot silently drop it without a test failure. (cherry picked from commit 6f8944b) (cherry picked from commit 8b35a11) (cherry picked from commit 6392a2c) (cherry picked from commit 17d595b) (cherry picked from commit a756fee) (cherry picked from commit ce1a966)
…nstall events The telemetry field reference implied every event carries installation_id, but the first trident install run's own command_start, any preflight failure, and trident_start all fire before Trident::install reaches the point of creating the ID -- so those specific events are sent without it. (cherry picked from commit a6b2280) (cherry picked from commit c64716d) (cherry picked from commit 9154e85) (cherry picked from commit ca5d718)
A read-only installation_id lookup left already-provisioned hosts whose datastore predates this key permanently without one: create_installation_id is only ever called by Trident::install, but offline initialization and the CIH update bootstrap both create/adopt a datastore directly, without going through install. Mint one as a one-time migration when the datastore is provisioned (servicing_state != NotProvisioned) but has no installation ID yet, while leaving a genuinely unprovisioned datastore (temporary, or not yet installed) read-only as before. (cherry picked from commit fc6a548) (cherry picked from commit 7a4a493) (cherry picked from commit 63ec787)
…onfirmed command_start fires before any operation context exists, and set_servicing_id only runs after staging or finalize validation confirms a real servicing operation -- so command_start, preflight failures, and no-op commands are all sent without this field. (cherry picked from commit a8b1e7f) (cherry picked from commit 3c922ef) (cherry picked from commit 3e8ed47)
Trident::new's migration check ran while a CIH-bootstrapped datastore was still NotProvisioned (this promotion, inside Trident::update, only happens later) so it never fired for this invocation -- the entire bootstrap update (trident_start, update_start, and everything after) ran with no installation ID, which was only minted whenever some later, separate invocation happened to run. Mint it now, synchronously, right after this same promotion, so update_start (fired later in this same invocation) and everything after it carries it. Known remaining gap: this invocation's own command_start/trident_start (fired earlier, in the CLI/daemon dispatch and Trident::new respectively) still can't -- Trident holds no TraceStream handle at this point to attach it to those. (cherry picked from commit 58467cb)
The CIH bootstrap persisted a new installation ID via create_installation_id, but never attached it to the shared TraceStream, so this invocation's update_start and later events still omitted installation_id: Trident::new's own attach-if-present check already ran and found nothing, since this datastore was still NotProvisioned at that point. Clone self.tracestream before entering execute_and_record_error (same pattern install already uses for its own newly-created IDs) and call set_installation_id here. The one remaining, unavoidable gap: command_start/trident_start, fired even earlier in the CLI/daemon dispatch and Trident::new respectively, still can't carry it.
58467cb to
ddd7fda
Compare
|
Addressed the suppressed review finding: the CIH bootstrap persisted a new installation ID via |
There was a problem hiding this comment.
🟡 Changes recommended
Clean-install servicing IDs are persisted before preflight checks confirm that staging will begin.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
| let servicing_id = datastore | ||
| .new_servicing_id() | ||
| .message("Failed to create servicing ID")?; | ||
| info!("Servicing ID: {servicing_id}"); | ||
| set_servicing_id(servicing_id.to_string()); |
asset_idfrom telemetry (the DMI product UUID)correlation_id->installation_id; and change when trident install starts.servicing_id(likeinstallation_id, stored in the datastore); and change when a servicing operation (install, update, or manual rollback) begins staging.Related PRs in stack: