telemetry: compile-in Azure Monitor connection string, best-effort AppInsights tracing - #774
Draft
bfjelds (bfjelds) wants to merge 25 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
| /// Telemetry is disabled. Trident will not send any tracing data off | ||
| /// the host. | ||
| #[default] | ||
| OptOut, |
Member
Author
There was a problem hiding this comment.
do we want OptOut as default?
Contributor
There was a problem hiding this comment.
🟡 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.
Contributor
There was a problem hiding this comment.
🔵 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
readcannot 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, parseContent-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 toREQUEST_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-blockingtry_sendand 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
Contributor
There was a problem hiding this comment.
🟡 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
readcontains 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, parseContent-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
Contributor
There was a problem hiding this comment.
🟡 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
Telemetryas an independent setting makes a telemetry-only config incompatible with non-default datastore provisioning. Evidence:configure_agent_configtreats any existing file withoutDatastorePath=as explicitly using the default and returnsImageBadAgentConfiguration; the documentedTelemetry=OptIn-only file (and every newly opted-in test image) therefore disables the prior path that created a requested non-default datastore configuration. Suggestion: updateconfigure_agent_configto merge a missingDatastorePathentry into an existing mutable config while preservingTelemetry, 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:
BackgroundUploadHandleuses an unbounded channel, whileBackgroundUploader::dropcloses 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
Contributor
There was a problem hiding this comment.
🟡 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-Lengthand 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
Member
Author
|
Addressed a suppressed Copilot finding (review summary, not a posted inline thread): Fixed by reading until the header terminator arrives, then parsing |
Contributor
There was a problem hiding this comment.
🔵 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::dropjoins its worker, andupload_loopdrains 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_stringaccepts 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 atrelease.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
Member
Author
|
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: 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 |
Contributor
There was a problem hiding this comment.
🟡 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
Fix two follow-up issues Copilot flagged on the background-uploader telemetry change: - BackgroundUploader::upload_loop() only checked for a transport-level send() error; a 4xx/5xx response was treated as success, so a rejected Application Insights event (or any rejected upload) was neither logged nor did it mark that origin as failing. Call error_for_status() on the response so non-2xx responses go through the same failure handling as network errors. - Corrected the send_event() doc comment, which claimed all failures are logged at trace level: enqueue-time failures (serialization, a closed uploader) are logged at trace level in appinsights.rs, but network/HTTP failures are logged by the background uploader itself (at error level, same as any other background upload).
The doc claimed all telemetry failures log at trace level, but only serialization/enqueue failures do -- BackgroundUploader logs actual delivery failures (network errors, non-2xx responses) at error level so operators can find remote-delivery problems in normal logs.
The background uploader uses an unbounded mpsc channel, so enqueueing cannot fail due to a full queue -- it only fails once the uploader has shut down and the receiver is dropped. Correct the doc to describe that scenario instead of a queue-full example that cannot occur.
…ull request test_app_insights_sender_posts_event did a single stream.read() and assumed it returned the complete HTTP request. TCP is a byte stream, not message-oriented, so a large/slow request can arrive across multiple reads, making the test flaky. Fix: read until the header terminator has arrived, then parse Content-Length and keep reading until the full body has too. Also send a minimal 200 response so the client request completes cleanly instead of hitting a connection reset.
…metry uploader - parse_connection_string now derives the ingestion endpoint from EndpointSuffix/Location (the documented sovereign-cloud connection string form) when IngestionEndpoint is not given explicitly, instead of silently falling back to the public Application Insights endpoint. Add regression tests covering EndpointSuffix with/without Location, and that an explicit IngestionEndpoint still wins. - Give Application Insights telemetry its own dedicated BackgroundUploader/queue in main(), separate from the one used for real log forwarding. Both uploaders drain sequentially on a single background thread each, so sharing one meant a slow-but-successful telemetry endpoint could build a backlog that delays operational log uploads; a dedicated queue means telemetry can only ever delay itself. Uploader creation failure disables telemetry rather than failing startup, consistent with telemetry being best-effort.
…elemetry setting is case-insensitive - EndpointSuffix without a Location must use the global dc prefix (https://dc.<suffix>), matching the public endpoint shape (dc.services.visualstudio.com); the previous https://in.<suffix> form is not a valid Application Insights host and every upload to it would fail. - Agent-Configuration.md now notes the Telemetry setting is case-insensitive (OptIn/optin/OPTIN all opt in), matching AgentConfig which lowercases the value before matching.
from_connection_string now rejects a track URL whose scheme is not https. Events sent through AppInsightsSender include host identifiers (see PLATFORM_INFO), and Azure Monitor ingestion endpoints require HTTPS, so a build-time typo or misconfiguration that produces an http:// endpoint must disable telemetry rather than silently send that data unencrypted. The private from_parts helper is left unchanged so the existing local-HTTP functional test (which talks to a non-TLS local test server) keeps working. Add a regression test.
configure_agent_config previously treated an existing agent config file with no DatastorePath= line as implicitly configured to the default path, erroring with ImageBadAgentConfiguration if a non-default path was actually expected. This agent config file can now also carry a Telemetry= line (see this PR), so a telemetry-only config -- entirely plausible once operators start pre-populating it to opt in -- would incorrectly break non-default datastore setups. Missing DatastorePath= still only means "use the default"; when a non-default path is expected, merge a DatastorePath= line into the existing file instead (preserving Telemetry= and any other existing lines), keeping the same root-verity restriction as the file-does-not-exist case. Add a regression test.
The opt-in telemetry section only described the locally-recorded metrics/spans being forwarded, but every event also carries PLATFORM_INFO: the DMI product UUID (asset_id), os_release, kernel_version, total_cpu, and total_memory_gib. List these explicitly before instructing operators to opt in.
Repo-build paths that define rpm_ver but do not pass
--define trident_azmon_conn_str (e.g.
packaging/docker/Dockerfile.full.public) previously compiled the
literal, undefined %{trident_azmon_conn_str} text into the binary as
AZURE_MONITOR_CONNECTION_STRING, instead of the documented empty
no-telemetry fallback. %{?trident_azmon_conn_str} expands to an empty
string when the macro is not defined, matching the intended default.
…in prod conn string - packaging/rpm/trident.spec: fix trident_azmon_conn_str_public -- contained a duplicated "InstrumentationKey=InstrumentationKey=..." prefix, which would have caused every distro-build telemetry event to be silently rejected by Application Insights (the ikey sent would never match a real one). Verified fixed macro expansion with `rpmspec -P`. - crates/trident/src/main.rs: setup_tracing() now returns a TelemetryStatus alongside the TraceStream, recording exactly why the Application Insights layer was or wasn't added (NotApplicable / OptedOut / NoConnectionString / UploaderUnavailable / Enabled). TelemetryStatus::log() is called once real logging is available (after each setup_logging() call site: Daemon, GrpcClient, and the general command path), so operators can tell from Trident's own logs -- without reading source -- whether telemetry should be expected to actually reach Application Insights. Previously this was completely silent in every case (see wiki/projects/trident/telemetry.md in mjolnir for the gap analysis that prompted this). cargo build --workspace, cargo clippy -p trident --all-targets, cargo fmt -p trident -- --check all clean. cargo test -p trident --lib: 405/405 pass (no new tests needed -- TelemetryStatus is a thin startup-logging wrapper around already-tested decision points in AppInsightsSender / AgentConfig).
…correlate operation_id + command on every metric
Stacked on PR 773 (persistent per-host correlation ID) via a clean rebase
(user/bfjelds/mjolnir/appinsights-telemetry-stacked), so correlation_id
and the new operation_id/command context can be included together on
every telemetry event.
New metrics:
- manual_rollback_start (engine/manual_rollback/mod.rs::execute_rollback):
fired unconditionally on every invocation (stage-only/finalize-only/
combined), mirroring update_start's placement/semantics. Required
adding #[derive(Debug, Clone, Copy)] to ManualRollbackRequestKind (had
no derives at all).
- runtime_update_success (engine/runtime_update.rs::finalize_update):
fired synchronously on success, since runtime update needs no reboot
and so never goes through engine::rollback's post-reboot
boot-validation flow where clean_install_success/ab_update_success/
manual_rollback_success are fired.
- manual_rollback_runtime_success (engine/manual_rollback/mod.rs::
finalize_rollback): closes a parallel gap found while adding the
above -- manual rollback of a *runtime* update also never reached the
boot-validation flow, so it had no success metric at all (only the A/B
rollback case did, via manual_rollback_success).
operation_id + command correlation (crates/trident/src/logging/
operation_context.rs, new module):
- A thread-local (not a tracing span -- simpler and sufficient, since
both places that set it run the whole command on one dedicated thread
for its duration) holds a fresh operation_id (UUID v4) + the command
name for the lifetime of a single command invocation.
- Wired at both funnel points so it works for CLI and gRPC/daemon alike:
* CLI: main.rs's run_trident() dispatch wraps each Install/Update/
Commit/Rollback/RebuildRaid arm in run_with_operation(). A new
command_name() helper derives "install"/"install_stage"/
"install_finalize" etc. from the requested Operations, matching
gRPC's existing naming.
* gRPC/daemon: server/tridentserver/mod.rs's servicing_request()
wraps its closure the same way, using its existing `name` parameter
(already "install"/"install_stage"/.../"rollback_finalize"/
"rebuild_raid"/"commit"/"check_root"/"stream_disk") -- covers every
gRPC operation for free from one call site.
* Fires a command_start metric immediately on entry, then every
subsequent metric/span fired on that thread (TraceSender in
tracestream.rs, AppInsightsSender in appinsights.rs) has
operation_id/command merged in automatically, without any
individual call site needing to pass them explicitly.
correlation_id propagation into AppInsightsSender (appinsights.rs):
- PR 773 added TraceStream::set_correlation_id/correlation_id_handle
for TraceSender only. AppInsightsSender now takes the same shared
Arc<RwLock<Option<String>>> handle (via a new
TraceStream::correlation_id_handle() accessor) and merges it into
every outgoing event, same as TraceSender already does.
Verified: cargo build --workspace, clippy --all-targets --features
functional-test, fmt --check all clean. cargo test -p trident --lib:
417/417 pass (4 new: operation_context unit tests). Functional test in
appinsights.rs extended to assert correlation_id/command/operation_id
all reach the mocked Application Insights endpoint.
bfjelds (bfjelds)
force-pushed
the
user/bfjelds/mjolnir/appinsights-telemetry
branch
from
September 4, 2026 19:57
c0bacd7 to
17ceb02
Compare
bfjelds (bfjelds)
changed the base branch from
main
to
user/bfjelds/datastore-generic-storage
September 4, 2026 19:58
…tion.md The host-metadata disclosure list only covered PLATFORM_INFO fields (asset_id/os_release/kernel_version/total_cpu/total_memory_gib/ trident_version). It was missing correlation_id (persistent per-host ID, propagated to AppInsightsSender in the previous commit) and the new operation_id/command fields (per-command-invocation correlation), even though both now ride along on every telemetry event.
…a separate PR Moving these out of this PR (correlation_id/operation_id infrastructure) into user/bfjelds/mjolnir/command-error-metric, which also adds a new command_error metric -- keeping all new-metric additions grouped together in one place rather than split across two PRs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Worker-thread metrics lose operation metadata, and the first daemon command event lacks its correlation ID.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 40/40 changed files
- Comments generated: 2
- Review effort level: Balanced
Addresses Copilot review comment on PR 774 (MonitorMetrics threads): CURRENT_OPERATION is a per-thread thread-local, so a thread spawned mid-command (MonitorMetrics::start_monitoring spawns its own OS thread via thread::spawn, emitting CPU/memory/network summary events from it at the end) starts with no context of its own -- those events were silently missing operation_id/command. Added operation_context::snapshot() (captures the calling threads current context, if any) and run_with_captured_operation() (installs a captured context on a different thread for the duration of a closure, clearing it afterwards -- unlike run_with_operation, does not mint a new operation_id or fire command_start, since it is reusing an existing operations identity, not starting a new one). Wired into MonitorMetrics::start_monitoring: snapshot() on the spawning thread before thread::spawn, run_with_captured_operation() inside the spawned closure. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 423/423 pass (4 new tests for snapshot/run_with_captured_operation).
Addresses Copilot review comment on PR 774: BackgroundUploader::drop drains every queued request and joins its worker unboundedly. ignored_servers (see start_upload_task) bounds this for an origin that outright fails -- only the first request to it is ever actually attempted -- but a slow-but-successful endpoint is not bounded that way: every queued request still gets its own attempt, each up to that request's own timeout, so draining a large backlog could still delay process shutdown for a while. Telemetry explicitly documents "must never meaningfully delay Trident's actual work" as a design goal, so this matters specifically for it (bg_uploader, which carries real log delivery, keeps its existing unbounded-drain guarantee unchanged). Added BackgroundUploader::shutdown_with_deadline(), backed by a new join_with_deadline() helper (moves the actual JoinHandle::join onto a throwaway thread and applies the timeout via a channel receive, since JoinHandle::join has no built-in timeout) -- abandons the background thread if the deadline elapses rather than waiting further. main.rs: added TelemetryUploaderGuard, a thin wrapper whose Drop impl calls shutdown_with_deadline(5s) -- wired in immediately after telemetry_uploader is constructed, so every one of main's many return paths shuts it down with a bounded deadline instead of BackgroundUploader's own unbounded Drop. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 423/423 pass (2 new tests). join_with_deadline is tested directly with a plain std::thread::spawn rather than a real BackgroundUploader + slow HTTP mock, deliberately -- a genuinely abandoned background thread doing real network I/O would keep running past its own test's scope, in a process shared with every other test under `cargo test`'s default parallelism, risking cross-test port/resource collisions (confirmed this empirically: an earlier version of this test using mockito caused a real, intermittent failure in an unrelated test using an overlapping mock path).
Addresses Copilot review comment on PR 774: the daemon's very first servicing request's command_start event (fired by run_with_operation, before that request's own Trident::new() call runs) was emitted before the persistent correlation ID had ever been read/created for this process -- Trident::new() calls the same get-or-create lookup on every request, but only that first request would find it missing. Every later request was already unaffected, since the shared TraceStream keeps whatever was set by the first successful lookup for the rest of the daemon's lifetime. server_main now performs the same get-or-create lookup up front, right after agent_config loads and before accepting any RPCs, so it is already warm by the time the first real request's command_start fires. cargo build -p trident, clippy --all-targets --features functional-test, fmt --check all clean. cargo test -p trident --lib: 423/423 pass (unchanged -- no existing test exercises server_main's startup sequence directly).
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
CLI startup metrics are emitted before operation context is established and therefore lack the documented command metadata.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/trident/src/main.rs:198
- Issue: CLI startup telemetry is emitted outside the new operation context, so not every event from a command receives
command/operation_id. Evidence:Trident::newruns at lines 175–181 and emitshc.feature_tracing()plustrident_start(lib.rs:241–261), while this wrapper begins only around the later servicing method. The daemon path wraps the closure containingTrident::new, so CLI and gRPC telemetry also diverge. Suggestion: establish the operation context around CLI initialization and dispatch (while preserving the same generated ID), rather than only aroundinstall/update/etc.
- Files reviewed: 42/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
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.
Update tests to OptIn.
Related PRs in stack: