telemetry: compile-in Azure Monitor connection string, best-effort AppInsights tracing by bfjelds · Pull Request #774 · microsoft/trident

bfjelds (bfjelds) · 2026-09-03T19:33:51Z

Adds opt-in tracing of Trident's existing metrics/spans to Azure Monitor / Application Insights.

AppInsights connection string is baked in during compilation, differing for trident repo builds vs azurelinux builds. Default to OptOut, with option to OptIn via the AgentConfig. Utilize existing background uploader to make best-effort to off-thread upload telemetry to app insights.

Related PRs in stack:

azure-pipelines · 2026-09-03T19:34:11Z

Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run.

bfjelds (bfjelds) · 2026-09-03T20:54:17Z

+    /// Telemetry is disabled. Trident will not send any tracing data off
+    /// the host.
+    #[default]
+    OptOut,


do we want OptOut as default?

Copilot

🟡 Changes recommended

The tracing sink can block servicing, sends incomplete HTTP requests, and includes a functional test that cannot observe its subscriber.

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

Pull request overview

Adds opt-in Azure Monitor/Application Insights telemetry for Trident metrics and spans.

Changes:

Adds an Application Insights tracing layer and agent opt-in setting.
Compiles connection strings through build and packaging workflows.
Enables telemetry in test images and documents configuration.

File summaries

File	Description
`tests/images/trident-vm-testimage/base/updateimg-grub.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/updateimg-grub-verity.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/updateimg-grub-verity-azure.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/baseimg-usr-verity.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/baseimg-root-verity.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/baseimg-grub.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/baseimg-grub-verity.yaml`	Installs opt-in config.
`tests/images/trident-vm-testimage/base/baseimg-grub-verity-azure.yaml`	Installs opt-in config.
`tests/images/trident-verity-testimage/usr/host.yaml`	Installs opt-in config.
`tests/images/trident-verity-testimage/usr/container.yaml`	Installs opt-in config.
`tests/images/trident-verity-testimage/base/baseimg.yaml`	Installs opt-in config.
`tests/images/trident-verity-testimage/base/baseimg-container.yaml`	Installs opt-in config.
`tests/images/trident-testimage/base/baseimg.yaml`	Installs opt-in config.
`tests/images/trident-mos/iso.yaml`	Installs opt-in config.
`tests/images/trident-mos/containerhost-iso.yaml`	Installs opt-in config.
`tests/images/trident-installer/base/baseimg.yaml`	Installs opt-in config.
`tests/images/trident-installer/base/baseimg-split.yaml`	Installs opt-in config.
`tests/images/trident-installer/base/baseimg-direct-streaming.yaml`	Installs opt-in config.
`tests/images/trident-functest/base/baseimg.yaml`	Installs opt-in config.
`tests/images/trident-container-testimage/base/baseimg.yaml`	Installs opt-in config.
`tests/images/trident-container-installer/base/baseimg.yaml`	Installs opt-in config.
`tests/images/common/trident.conf`	Defines telemetry opt-in.
`tests/images/azurelinux-direct-streaming-testimage/base/baseimg.yaml`	Installs opt-in config.
`tests/images/azl-installer/installer-iso.yaml`	Installs opt-in config.
`packaging/rpm/trident.spec`	Exports build-time connection strings.
`packaging/docker/Dockerfile.full`	Passes connection string to RPM builds.
`docs/Reference/Agent-Configuration.md`	Documents telemetry configuration.
`crates/trident/src/main.rs`	Conditionally registers telemetry tracing.
`crates/trident/src/logging/mod.rs`	Registers the new logging module.
`crates/trident/src/logging/appinsights.rs`	Implements Application Insights ingestion.
`crates/trident/src/lib.rs`	Exports telemetry symbols and build value.
`crates/trident/src/agentconfig.rs`	Parses the telemetry preference.
`crates/trident/build.rs`	Tracks connection-string changes.
`.pipelines/templates/stages/trident_rpms/release.yml`	Supplies the pipeline connection string.

Review details

Files reviewed: 34/34 changed files
Comments generated: 3
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

🔵 Needs a closer look

Telemetry currently blocks servicing threads during HTTP requests and compiled placeholder endpoints trigger guaranteed delivery failures.

Review details

Suppressed comments (3)

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

crates/trident/src/logging/appinsights.rs:400

Issue: A single TCP read cannot reliably capture one complete HTTP request because TCP has no message boundaries. Evidence: headers and JSON body may arrive in separate reads even for this small request, making the body assertions intermittently fail. Suggestion: read through \r\n\r\n, parse Content-Length, then read the remaining body (and return a valid HTTP response), or use the repository's HTTP test server helper.
packaging/rpm/trident.spec:26
Issue: This non-empty placeholder is accepted as a configured connection string and compiled into every distro build. Evidence: an opted-in host will attempt telemetry against placeholder-public... rather than taking the documented no-connection-string no-op path; the pipeline connection string has the same placeholder pattern. Suggestion: compile an empty value until real endpoints are available, or supply valid connection strings before merging the wiring.

crates/trident/src/logging/appinsights.rs:207

Issue: This performs a blocking HTTP request directly inside the tracing layer callback, so telemetry is not isolated from servicing. Evidence: reqwest::blocking::Client::send() holds the thread that emitted each metric/span for up to REQUEST_TIMEOUT (5 seconds); Trident has many sequential instrumented spans, so an offline or slow endpoint can add repeated delays and can block Tokio worker threads in daemon mode. Suggestion: enqueue envelopes with a non-blocking try_send and let a bounded background worker perform/batch HTTP requests, dropping telemetry when the queue is full.

        let response = match self.client.post(&self.track_url).json(&envelope).send() {

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

Copilot

🟡 Changes recommended

HTTP rejection handling, failure log levels, and nondeterministic TCP test framing remain unresolved.

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

Review details

Suppressed comments (1)

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

crates/trident/src/logging/appinsights.rs:427

Issue: The functional test assumes one TCP read contains the complete HTTP request. Evidence: TCP may return only part of the headers or body even for a small request, making the subsequent JSON assertions nondeterministic. Suggestion: read through the header terminator, parse Content-Length, continue until the full body arrives, and return a valid HTTP response.

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

Copilot

🟡 Changes recommended

Telemetry-only configuration breaks non-default datastore provisioning, and queued telemetry can substantially delay process shutdown.

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

Review details

Suppressed comments (3)

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

crates/trident/src/agentconfig.rs:52

Issue: Adding Telemetry as an independent setting makes a telemetry-only config incompatible with non-default datastore provisioning. Evidence: configure_agent_config treats any existing file without DatastorePath= as explicitly using the default and returns ImageBadAgentConfiguration; the documented Telemetry=OptIn-only file (and every newly opted-in test image) therefore disables the prior path that created a requested non-default datastore configuration. Suggestion: update configure_agent_config to merge a missing DatastorePath entry into an existing mutable config while preserving Telemetry, and cover that combination in its tests.
crates/trident/src/logging/appinsights.rs:430
Issue: This functional test assumes one TCP read contains the complete HTTP request. Evidence: TCP may split headers and body across reads, so the body assertions can fail even though the sender posted a valid request. Suggestion: read through the declared Content-Length (and return a valid HTTP response) before sending the captured request to the assertion thread.

crates/trident/src/logging/appinsights.rs:233

Issue: Enqueuing every telemetry item can still block command termination for an unbounded time. Evidence: BackgroundUploadHandle uses an unbounded channel, while BackgroundUploader::drop closes the sender and joins a worker that deliberately drains every queued request serially; a slow endpoint can therefore add up to five seconds per pending metric/span at process exit. Suggestion: use a bounded/drop-on-full telemetry queue and discard pending telemetry during shutdown, or isolate telemetry in a worker whose shutdown does not drain indefinitely.

        if let Err(e) = self.uploader.upload(
            &self.track_url,
            body,
            REQUEST_TIMEOUT,
            Some(CONTENT_TYPE_JSON),

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

Copilot

🟡 Changes recommended

The new TCP-based functional test can fail nondeterministically because it assumes one read contains the complete HTTP request.

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

Review details

Suppressed comments (1)

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

crates/trident/src/logging/appinsights.rs:430

Issue: This functional test assumes one TCP read returns the entire HTTP request, which TCP does not guarantee. Evidence: the assertions inspect both the request line and JSON body, but a successful read may contain only a partial header or body, making the test flaky. Suggestion: read through the declared Content-Length and return a minimal HTTP response, or use an HTTP mock server that captures the complete request.

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

bfjelds (bfjelds) · 2026-09-04T01:32:19Z

Addressed a suppressed Copilot finding (review summary, not a posted inline thread): test_app_insights_sender_posts_event assumed a single TCP read returns the complete HTTP request, which TCP does not guarantee, making the test potentially flaky for larger/slower requests.

Fixed by reading until the header terminator arrives, then parsing Content-Length and reading until the full body has too, and sending a minimal response so the client request completes cleanly. Fixed in 044618e.

Copilot

🔵 Needs a closer look

Placeholder endpoints activate nonfunctional telemetry, and queued uploads can substantially delay shutdown.

Review details

Suppressed comments (2)

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

crates/trident/src/logging/appinsights.rs:233

Issue: Enqueueing makes callbacks asynchronous, but telemetry can still delay command completion substantially. Evidence: BackgroundUploader::drop joins its worker, and upload_loop drains every queued item sequentially; a slow endpoint returning success just under the 5-second timeout can therefore make shutdown wait up to five seconds per metric/span. Suggestion: give best-effort telemetry a bounded/non-draining shutdown policy, or add a bounded total flush deadline that discards remaining telemetry items.
packaging/rpm/trident.spec:26
Issue: This non-empty placeholder activates telemetry in distro builds even though it cannot identify a real Application Insights resource. Evidence: parse_connection_string accepts it as valid, and the test images opt in, so released/test binaries attempt delivery to a placeholder and then disable that origin after the inevitable failure; the pipeline repeats the same pattern at release.yml:67. This also contradicts the PR description saying population is deferred. Suggestion: keep the compiled value empty until a real resource-specific connection string is supplied, or land the real build-time value atomically with this wiring.

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

bfjelds (bfjelds) · 2026-09-04T01:59:01Z

Replying to the Copilot review summary comment (suppressed findings, no inline thread exists for either):

appinsights.rs:233 — shutdown can wait up to ~5s per queued item

Agreed this is accurate, but it's an accepted tradeoff, not a bug: upload_loop drains the queue on shutdown so that telemetry already enqueued before a graceful stop actually gets a chance to land, rather than being silently dropped. The wait is bounded per-item by REQUEST_TIMEOUT (5s), and in practice the queue is tiny (a handful of lifecycle events per run); a slow/hanging endpoint is the worst case, and test_upload_loop_failed_host_is_ignored_for_future_uploads already ensures a failing origin gets disabled after one failure so it can't repeatedly stall future uploads. We're fine trading a few seconds of shutdown latency for "best effort" telemetry actually being delivered. Not changing this.

trident.spec:26 / release.yml:67 — placeholder connection string is a valid-looking value

Also accurate, but intentional and already flagged: both spots carry an explicit # TODO: replace with the real ... connection string. comment — deliberately calling out state pending a follow-up once the real AI resources exist, not a permanent placeholder. parse_connection_string correctly accepting it as well-formed is expected (it's syntactically valid, just pointing at a non-existent resource); delivery will simply fail once and that origin gets disabled by the same failure-handling logic noted above, so it doesn't loop or block anything. No functional risk — leaving the TODOs as the tracking mechanism.

Copilot

🟡 Changes recommended

Sovereign-cloud connection strings can route to the wrong endpoint, and telemetry can starve existing log uploads.

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

Review details

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

PR #774 (appinsights-telemetry) was re-squashed onto PR #773's actual git tip to fix a duplicate cherry-picked commit ("merge DatastorePath into existing agent config missing it") that had caused PR #773 and #774 to diverge instead of being truly stacked. That re-squash moved #774's tip, which invalidated this branch's (#778, command-error-metric) ancestry relative to #774 a second time. This commit redoes the previous squash-merge of #778's original, unsquashed content (29 commits, tip df17bab) onto the new #774 tip, using `git merge --squash`. Conflicts were resolved by comparing against the previously-validated squash of this same logical content and carrying over identical resolutions where the diff showed only conflict markers, with manual resolution for a genuine import duplication (`datastore::DataStore` vs. the `crate::{ DataStore, ...}` re-export) in server/tridentserver/mod.rs. No functional changes vs. the previously pushed #778 content; verified fmt/clippy(--lib/--bins)/tests(--lib 436 passed/--bins clean).

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).

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.

773 and 774 had diverged in git history (both independently cherry-picked the same "merge DatastorePath into existing agent config missing it" fix commit under different hashes) despite being logically stacked via GitHub base branches. Squashed to one commit rather than replayed commit-by-commit, consistent with how PR 778 and PR 779 were resolved earlier in this same stacked-PR cleanup -- see those PRs' squash commits for the equivalent rationale. No conflicts: PR 774's content applied cleanly on top of PR 773's tip. See PR #774 for the full commit-by-commit history and review discussion.

…cing_id design Replaces the eager "pre-warm the correlation ID" scheme (which created a datastore as a side effect of telemetry setup, before Host Configuration was even parsed) with a simpler, read-only-by-default design: - `operation_id` (already generated for every invocation via `operation_context`) is used as the telemetry fallback identifier for any event that fires before a host has a persisted installation ID -- most notably a host's very first command. No datastore or ID is ever created just to populate a trace/metric field. - `DataStore::installation_id()` is now a read-only getter; the datastore is never implicitly created to serve a read. - `DataStore::create_installation_id()` is the only way a new installation ID is ever persisted, and it seeds the value from the current `operation_id` -- so a host's first-ever install and its first persisted installation ID share the same value by construction (the host is identified by its first operation's ID). - `TraceStream::attach_installation_id_if_present` (read-only) replaces the old pre-warm call at every startup/request entry point (CLI dispatch, daemon startup, each gRPC servicing_request). It never creates a datastore. - `TraceStream::create_and_attach_installation_id` (the only ID-creating call site) runs once, from inside `Trident::install` itself, right after settling on which datastore the install will use -- including the CIH-bootstrap path in `Trident::update`. - `DataStore::may_initialize_datastore_for_command` replaces the ad-hoc per-caller "can this command create a new datastore" checks (both the CLI's enum match and the gRPC server's request-name string match), giving CLI and daemon one shared answer to "is a missing datastore expected for this command". - `servicing_id`/`new_servicing_id` are added alongside installation_id, always minting a fresh value on every call (unlike installation_id, which is created once and persists). - The CLI and gRPC install/update preflight guards that rejected an unparsable Host Configuration *before* the old pre-warm (to avoid leaving a stray datastore behind) are removed: nothing creates a datastore as a side effect anymore, so the hazard they guarded against no longer exists, and `Trident::new` already performs the same parse. - A multiboot install may swap to a different datastore inside `Trident::install`, so its `Trident` is constructed via `Trident::new_deferring_installation_id` instead of `Trident::new`, deferring the attach until `install` has settled on the right datastore. No "correlation_id"/"mint" naming remains in the touched files.

…ated enrichment logic Addresses findings from deep review of PR #774 (multiboot installation-ID mistagging accepted as-is; migration path from correlation-id to installation-id explicitly not needed): - install() no longer aborts the entire install when create_and_attach_installation_id fails -- it now warns and continues, matching the same best-effort invariant update()'s CIH bootstrap path already honors. A telemetry-labeling write failure must never block servicing. - docs/Reference/Telemetry.md corrected: the field actually emitted is installation_id, not the stale correlation_id name left over from before this PR's rename. - attach_installation_id_if_present's doc comment now discloses that it performs a one-time migration write for already-provisioned datastores missing an installation ID (offline-init / CIH bootstrap hosts), instead of overclaiming unconditional read-only/ side-effect-free behavior. - merge_operation_context (operation_id/command/installation_id fallback enrichment) is now shared by both local telemetry sinks (AppInsightsSender::send_event and TraceSender::additional_fields) instead of being hand-duplicated between them, removing a single-source-of-truth risk.

Copilot

🟡 Changes recommended

Multiboot installation attribution and empty-operation classification currently produce incorrect datastore and telemetry behavior.

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

Introduces a separate database_id concept (carried over from PR773s rename of the former correlation_id), distinct from installation_id: - database_id: stable for the entire lifetime of the datastore, get-or-create on first access, independent of any specific install. - installation_id: unchanged from the existing implementation in this PR -- still get-or-create-once, seeded from the current operation_id, created explicitly at the start of Trident::install (and the CIH update-bootstrap path). Both fields are attached to TraceStream/AppInsights telemetry independently (separate JSON keys database_id/installation_id), so existing installation_id-based correlation continues to work unchanged while database_id becomes available for datastore-lifetime correlation. Telemetry.md documents both fields side by side. Note: this commit intentionally reimplements database_id locally rather than merging PR773s actual rename commit, to avoid an unrelated large rebase in this branch; reconciling the two will happen in the follow-up local restack.

Resolves in favor of this branch's existing database_id/installation_id split, which already supersedes 773's standalone rename.

Copilot

🟡 Changes recommended

Multiboot installs can retain the previous datastore’s installation ID and misattribute 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: 3
Review effort level: Balanced

…ocations Introduces OperationSource (Cli/Daemon/GrpcClient), threaded through run_with_operation/CapturedOperation alongside operation_id/command, and merged into every event via merge_operation_context. CLI direct invocations (main.rs) tag Cli; the daemon servicing_request handler tags Daemon. GrpcClient is defined now for PR778 (command-error-metric), which introduces the actual grpc_client dispatch call site, to tag when it wires source through its own run_command_if call. Documents the new source field in Telemetry.md.

Copilot

🟡 Changes recommended

Multiboot attribution and early-event installation/database identifiers are currently incorrect.

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

Review details

Suppressed comments (1)

crates/trident/src/main.rs:203

Issue: This unconditional pre-warm defeats the multiboot deferral. Evidence: on an already-provisioned host it caches the old installation ID before new_deferring_installation_id; create_and_attach_installation_id then returns early because an ID is cached, so the temporary datastore never gets its own ID and the run remains attributed to the old installation. Suggestion: skip this pre-warm for multiboot installs.

                if let Ok(agent_config) = AgentConfig::load() {
                    tracestream.attach_installation_id_if_present(agent_config.datastore_path());

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

…se_id prewarm, uploader status wording, installation_id doc

Copilot

🟡 Changes recommended

The gRPC-client path never installs its declared operation context, and the metadata documentation overstates identifier availability.

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

…l events database_id (and by extension the intro sentence) overclaimed every event includes this metadata. attach_database_id_if_present/ attach_installation_id_if_present both return early if the datastore does not exist yet, so command_start/trident_start events fired before a first install creates the datastore lack database_id.

Copilot

🟡 Changes recommended

The daemon omits the shared datastore-initialization guard, allowing invalid servicing requests to create an empty datastore.

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

Applies the same rename as user/bfjelds/datastore-generic-storage (773) to the additional call sites this branch introduces (main.rs preflight attach, appinsights.rs, server/mod.rs, server/tridentserver/mod.rs, Telemetry.md), for consistency with the DataStore/datastore_path terminology used everywhere else.

…torage' into user/bfjelds/mjolnir/appinsights-telemetry # Conflicts: # crates/trident/src/datastore.rs # crates/trident/src/lib.rs # crates/trident/src/logging/tracestream.rs

Copilot

🟡 Changes recommended

Daemon admission can validate a different datastore path than the request handler subsequently uses.

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

…ig snapshot instead of AgentConfig::load()

Copilot

🔵 Needs a closer look

The cross-cutting persistence, concurrency, networking, privacy, and packaging changes warrant final human validation.

Review details

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

Both attach_*_if_present calls neither create a datastore file, but either can still persist a missing ID onto an existing datastore (datastore_id via get-or-create semantics, installation_id_or_migrate via legacy-ID migration). The prior wording incorrectly claimed both calls are fully read-only/side-effect-free.

Copilot

🔵 Needs a closer look

The generated envelope name is malformed and partial ingestion failures are currently treated as successful delivery.

Review details

Suppressed comments (2)

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

crates/trident/src/logging/appinsights.rs:295

Issue: The raw EventData envelope uses a nonstandard type name. Evidence: Application Insights SDKs serialize custom events as Microsoft.ApplicationInsights.<instrumentation-key>.Event; omitting the key here can cause ingestion to reject or misclassify every event. Suggestion: include the instrumentation key in the envelope name.
crates/trident/src/logging/background_uploader.rs:156
Issue: Application Insights partial failures are treated as successful uploads. Evidence: StatusCode::is_success() includes 206, but the ingestion endpoint uses 206 Partial Success when itemsReceived > itemsAccepted; this branch then removes the origin's failure state even when this single queued event was rejected. Suggestion: allow an upload to define its response policy (or specifically handle 206 for Application Insights), and surface the partial-success response as a delivery failure.

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

… ingestion 1. Envelope name was hardcoded to "Microsoft.ApplicationInsights.Event", omitting the instrumentation key AppInsights expects ("Microsoft.ApplicationInsights.<iKey>.Event"), risking ingestion rejection or misclassification. 2. BackgroundUploader treated any 2xx response as success, including Application Insights 206 Partial Success responses whose body can report itemsAccepted < itemsReceived. For this sender ' s single-envelope requests, a 206 always means that one event was rejected. Added an optional per-upload response_validator (BackgroundUploadHandle:: upload_with_validator) that AppInsightsSender uses to parse the 206 body and treat a rejected item as a failure, going through the existing retry/backoff path instead of being silently discarded. Added unit tests for the new validator.

Copilot

🟡 Changes recommended

Separate configuration snapshots can misattribute telemetry, and the partial-success validator accepts invalid item counts.

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

Review details

Suppressed comments (1)

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

crates/trident/src/main.rs:211

Issue: The pre-warm and servicing path read AgentConfig separately. Evidence: if DatastorePath changes between these reads, IDs from the first path are cached on TraceStream, while the command operates on the second path; the attach helpers then short-circuit on the cached values, misattributing the whole operation (and potentially preventing the new installation ID from being attached). Suggestion: load one AgentConfig result, use it for pre-warming, and consume that same snapshot inside the operation closure.

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

… AgentConfig snapshot on CLI pre-warm path validate_track_response checked mere equality between itemsReceived and itemsAccepted, so impossible responses for this always-single-envelope sender (e.g. 0/0 or 2/2) were mistaken for success. Require exactly 1/1. main.rs pre-warmed tracestream IDs from one AgentConfig::load() call, then the operation closure called AgentConfig::load() again -- if the datastore path changed between the two reads (e.g. a CIH bootstrap swap), the pre-warmed IDs would be attributed to the wrong datastore. Load once, reuse the same snapshot for both.

Copilot

🔵 Needs a closer look

It combines persistence, concurrency, networking, telemetry privacy, and packaging changes that warrant final human validation.

Review details

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

bfjelds (bfjelds) commented Sep 3, 2026

View reviewed changes

Comment thread packaging/rpm/trident.spec Outdated

bfjelds (bfjelds) commented Sep 3, 2026

View reviewed changes

Comment thread .pipelines/templates/stages/trident_rpms/release.yml Outdated

bfjelds (bfjelds) commented Sep 3, 2026

View reviewed changes

Comment thread crates/trident/src/logging/appinsights.rs

bfjelds (bfjelds) requested a balanced review from Copilot September 3, 2026 23:40

Copilot started reviewing on behalf of bfjelds (bfjelds) September 3, 2026 23:41 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread crates/trident/src/logging/appinsights.rs

Comment thread crates/trident/src/logging/appinsights.rs Outdated

Comment thread crates/trident/src/logging/appinsights.rs Outdated

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 00:01

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 00:01 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 00:35

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 00:36 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread crates/trident/src/logging/appinsights.rs Outdated

Comment thread crates/trident/src/logging/background_uploader.rs Outdated

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 00:53

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 00:54 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread docs/Reference/Agent-Configuration.md Outdated

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 01:18

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 01:19 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread docs/Reference/Agent-Configuration.md Outdated

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 01:34

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 01:34 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 02:01

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 02:02 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread crates/trident/src/logging/appinsights.rs

Comment thread crates/trident/src/main.rs Outdated

bfjelds (bfjelds) requested a balanced review from Copilot September 4, 2026 02:14

Copilot started reviewing on behalf of bfjelds (bfjelds) September 4, 2026 02:14 View session

bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/appinsights-telemetry branch from 6c02930 to 7d4e313 Compare September 7, 2026 19:05

bfjelds (bfjelds) added 2 commits September 7, 2026 20:48

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread crates/trident/src/main.rs

Comment thread crates/trident/src/main.rs Outdated

bfjelds (bfjelds) added 3 commits September 8, 2026 18:04

merge: pull in PR773 correlation_id->database_id rename (wt-773 e8ecc9a)

18ae42b

Resolves in favor of this branch's existing database_id/installation_id split, which already supersedes 773's standalone rename.

merge: pull in PR773 rustfmt fix (wt-773 98a22f7)

7c600f7

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread crates/trident/src/logging/tracestream.rs

Comment thread crates/trident/src/main.rs

Comment thread docs/Reference/Telemetry.md Outdated

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread crates/trident/src/logging/tracestream.rs

Comment thread crates/trident/src/main.rs

bfjelds (bfjelds) mentioned this pull request Sep 8, 2026

telemetry: add stream_image_success and clean_install_finalized completion metrics #783

Closed

Merge 773 datastore fix; address review comments: noop suffix, databa…

a1ceb6f

…se_id prewarm, uploader status wording, installation_id doc

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread crates/trident/src/logging/operation_context.rs

Comment thread docs/Reference/Telemetry.md Outdated

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread crates/trident/src/server/tridentserver/mod.rs Outdated

bfjelds (bfjelds) added 2 commits September 9, 2026 01:40

Merge remote-tracking branch 'origin/user/bfjelds/datastore-generic-s…

16cde0d

…torage' into user/bfjelds/mjolnir/appinsights-telemetry # Conflicts: # crates/trident/src/datastore.rs # crates/trident/src/lib.rs # crates/trident/src/logging/tracestream.rs

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread crates/trident/src/server/tridentserver/mod.rs

Fix datastore-existence check and ID re-attach to use self.agent_conf…

1e3dbb3

…ig snapshot instead of AgentConfig::load()

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread crates/trident/src/server/tridentserver/mod.rs Outdated

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread crates/trident/src/logging/appinsights.rs Outdated

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

bfjelds (bfjelds) commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines Bot commented Sep 3, 2026

Uh oh!

bfjelds (bfjelds) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (3)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

bfjelds (bfjelds) commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

bfjelds (bfjelds) commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

bfjelds (bfjelds) commented Sep 3, 2026 •

edited

Loading