telemetry: remove asset_id - #779
Draft
bfjelds (bfjelds) wants to merge 15 commits into
Draft
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
2 times, most recently
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.
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/command-error-metric
branch
from
September 5, 2026 16:21
a97f856 to
a2e921d
Compare
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 16:31
c96c0af to
bcab5b0
Compare
Contributor
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
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 17:02
bcab5b0 to
941e873
Compare
Contributor
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
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/command-error-metric
branch
2 times, most recently
from
September 5, 2026 19:51
f6f0da6 to
5e7b215
Compare
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 19:58
941e873 to
6f8944b
Compare
Contributor
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
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/command-error-metric
branch
from
September 5, 2026 20:36
5e7b215 to
e6ac4c4
Compare
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 5, 2026 20:41
6f8944b to
7256f43
Compare
bfjelds (bfjelds)
added a commit
that referenced
this pull request
Sep 7, 2026
PR #778 (command-error-metric) was amended a second time to remove the tests/images Telemetry=OptIn opt-in changes, which had come back in through 778's own original commit ancestry (778 was originally branched from a point in history that predated the tests/images split-out on PR #774). Those changes now belong in a new, dedicated PR stacked on #779, so this squash-merge excludes them the same way, keeping #779 free of tests/images changes as well. Otherwise this is the same squash-merge of #779's original, unsquashed content (the Option A TraceStream/servicing-id refactor) onto the new #778 tip as before -- Rust source resolutions confirmed byte-identical to the previously-validated pr779-squash-v2. No functional changes vs. the previously pushed #779 content aside from dropping tests/images; verified fmt/clippy(--lib/--bins)/tests (--lib/--bins).
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/installation-servicing-id
branch
from
September 7, 2026 18:45
e8a0cd1 to
0568a97
Compare
bfjelds (bfjelds)
added a commit
that referenced
this pull request
Sep 7, 2026
Adds a shared tests/images/common/trident.conf with Telemetry=OptIn and references it from every test/dev image definition, enabling Application Insights telemetry opt-in by default across test/dev images. Originally part of PR #774 (appinsights-telemetry); split out to its own PR, stacked on PR #779 (installation-servicing-id), since these image-definition changes are unrelated to the core telemetry plumbing and servicing-ID work in the earlier PRs in the stack.
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/command-error-metric
branch
from
September 7, 2026 18:49
a76c89f to
558c732
Compare
Also passes --ignore-schema to the Kusto ingestor in .pipelines/templates/stages/testing_common/scripts/upload_metrics.sh, since this PR changes the local metrics schema (installation_id rename, servicing_id addition, asset_id removal) and the pipeline must not fail ingestion on the resulting schema drift.
…fixes) # Conflicts: # docs/Reference/Telemetry.md
…client) Auto-merged cleanly -- PR779 does not independently touch any operation_context/run_command call sites, so no manual conflict resolution was needed here.
…or-metric' into user/bfjelds/mjolnir/installation-servicing-id
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unconditional schema bypass can conceal future telemetry contract regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
…or-metric' into user/bfjelds/mjolnir/installation-servicing-id
…or-metric' into user/bfjelds/mjolnir/installation-servicing-id
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared platform map also removes the UUID from diagnostic reports, exceeding the stated telemetry-only scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
…or-metric' into user/bfjelds/mjolnir/installation-servicing-id
…or-metric' into user/bfjelds/mjolnir/installation-servicing-id
…hot + rollback --check fixes)
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)Related PRs in stack: