[bfjelds (bfjelds)]

Add the tracing layer, isolated bounded uploader, and best-effort opt-in wiring.

Related PRs in stack:

• telemetry [1]: add generic key-value storage to datastore #773
• telemetry [2]: introduce typed command classification #880
• telemetry [3]: correlate commands with operation context #881
• telemetry [4]: persist and attach installation IDs #882
• telemetry [5]: correlate staged servicing operations #883
• telemetry [6]: add agent configuration opt-in #884
• telemetry [7]: share background upload transport #885
• [this PR] telemetry [8]: send metrics to Application Insights #774
• telemetry [9]: configure telemetry ingestion destinations #886
• telemetry [10]: enrich platform and startup metrics #888
• telemetry [11]: report effective servicing feature usage #889
• telemetry [12]: preserve local metrics and bound TraceStream requests #890
• telemetry [13]: introduce command error reporting helpers #778
• telemetry [14]: report CLI and reboot command failures #891
• telemetry [15]: report daemon dispatch and request errors #892
• telemetry [16]: report gRPC client transport failures #893
• telemetry [17]: complete servicing outcome metrics #894
• telemetry [18]: remove asset_id #779
• telemetry [19]: enable Telemetry=OptIn by default for test/dev images #781

[azure-pipelines]

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

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

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

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

[Copilot]

Copilot review overview

🟡 Changes recommended

gRPC-client tracing can truncate daemon metrics, and an early CLI error path misattributes installation telemetry.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Apply metrics ownership policy when creating trace sender · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

It combines telemetry transport, persistent identity migration, packaging, and concurrency-sensitive daemon behavior.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Apply metrics ownership policy when creating trace sender

[Copilot]

🔵 Needs a closer look

Cross-cutting changes to persisted IDs, CLI/daemon execution, and uploader shutdown need build and integration validation plus final human review.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The custom ingestion protocol and concurrent shutdown behavior require human validation before approval.

2 open issues

• Measure App Insights spans before blocking trace forwarding · New
• Drain reboot telemetry before requesting system reboot · New

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Asynchronous delivery and reboot-time shutdown interactions need human review and runtime validation before approval.

No open issues 🎉

2 resolved since last review

• Drain reboot telemetry before requesting system reboot
• Measure App Insights spans before blocking trace forwarding

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Outbound host-data delivery and shutdown/reboot integration need final human validation because end-to-end runtime behavior was not verified in this review.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Concurrent uploading and reboot/shutdown integration warrant human validation because execution of the added tests and live ingestion were not verified.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The daemon reboot path does not drain queued telemetry before requesting a reboot, risking loss of the reboot event and servicing metrics.

1 open issue

• Flush telemetry before daemon-initiated reboot · New

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Successful upload responses are buffered without a size limit, creating a memory-exhaustion risk, and the telemetry documentation misstates which commands write metrics.

1 open issue

• Unbounded response buffering can exhaust process memory · New

1 resolved since last review

• Flush telemetry before daemon-initiated reboot

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The test server advertises a persistent HTTP connection but closes it after one request, risking connection resets and intermittent test timeouts.

1 open issue

• Advertise Connection: close for single-request sockets · New

1 resolved since last review

• Unbounded response buffering can exhaust process memory

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The new Application Insights ingestion and reboot-time flushing paths require validation against a real Azure endpoint and systemd; the supplied tests use local mocks.

No open issues 🎉

1 resolved since last review

• Advertise Connection: close for single-request sockets

_{Review effort: Balanced}