telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id - #779
Draft
bfjelds (bfjelds) wants to merge 6 commits into
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. |
bfjelds (bfjelds)
changed the title
| --filepath "$METRICS_FILE" \ | ||
| --mapping "$KUSTO_TABLE_MAPPING" | ||
| --mapping "$KUSTO_TABLE_MAPPING" \ | ||
| --ignore-schema |
Member
Author
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
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 00:50
be44dde to
a67b099
Compare
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.
…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).
…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).
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.
…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.
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.
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 01:36
1599407 to
c96c0af
Compare
Contributor
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.
Comment on lines
+580
to
+584
| 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()); |
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.
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: