openvmm: add optional OpenTelemetry tracing - #4468
mayank-microsoft wants to merge 7 commits into
Conversation
Add opt-in OTLP trace export, reserve performance spans from normal logging, and instrument VM startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8" | ||
|
|
||
| [[package]] |
There was a problem hiding this comment.
Definitely not a fan of all these new deps getting pulled in. Are there any features we can turn off or anything?
There was a problem hiding this comment.
Yeah, I did investigate that, aws crates get in because of rustls support in reqwest. We can do away with them for initial integration if we are ok with skipping HTTPS support. We can consider writing a bespoke HTTPS client subsequently.
There was a problem hiding this comment.
Let's go minimal to start then.
There was a problem hiding this comment.
Definitely better, but is there anything else we can remove?
There was a problem hiding this comment.
working on getting rid of tokio, hyper, etc being pulled by reqwest. We will need to implement a basic http client for it. I am looking at what could be a basic implementation.
There was a problem hiding this comment.
we can use HTTP/gRPC, by default the crate opentelemetry-otlp ships reqwest/hyper and tonic as http and grpc client implementation respectively. We can pass a custom implementation for the http/gRPC client by implementing a trait. But that's where we are. I am looking at mesh_rpc, if we can pick anything from there, but it seems we dont have a gRPC client implementation.
There was a problem hiding this comment.
I see we already use tokio with mesh_rpc, do you think its ok to use hyper as well then?
There was a problem hiding this comment.
This would be a good John Starks (@jstarks) question
There was a problem hiding this comment.
trying out hyper-client implementation for opentelemetry-otlp, we need to run it from a tokio runtime context, that does adds some complexity, we will have to create a runtime and create a wrapped hyper http client which uses that tokio runtime. It would be great to great to know if we have some thoughts on how we should be handling scenarios where we need a gRPC/HTTP Client in OpenVMM.
There was a problem hiding this comment.
Yeah this is definitely something John will have thoughts on.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues remain around exporter shutdown and telemetry filtering.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds opt-in OpenTelemetry OTLP/HTTP tracing to OpenVMM, including VM resume instrumentation and logging documentation.
Changes:
- Adds feature-gated exporter support and dependencies.
- Integrates tracing layers and excludes performance spans from normal logging.
- Documents configuration and updates dependency manifests.
Review findings:
openvmm/openvmm_entry/src/lib.rs(moderate, 3 votes): The mesh-host exit path can bypassTracerProviderGuard::drop, preventing final spans from being flushed.openvmm/openvmm_entry/src/tracing_init.rs(moderate, 1 vote): The registry-wide filter can suppress telemetry whenOPENVMM_LOGis restrictive.
File summaries
| File | Description |
|---|---|
support/otel_tracing/src/lib.rs |
Initializes the OTLP exporter and provider guard. |
support/otel_tracing/Cargo.toml |
Declares OpenTelemetry dependencies. |
openvmm/openvmm/Cargo.toml |
Exposes the optional otel feature. |
openvmm/openvmm_entry/src/tracing_init.rs |
Configures tracing layers and filters. |
openvmm/openvmm_entry/src/lib.rs |
Retains the tracing guard during execution. |
openvmm/openvmm_entry/Cargo.toml |
Adds the optional tracing dependency. |
openvmm/openvmm_core/src/worker/dispatch.rs |
Instruments VM resume. |
Guide/src/reference/openvmm/logging.md |
Documents OTLP setup. |
Cargo.toml |
Adds workspace dependencies. |
Cargo.lock |
Locks new dependency versions. |
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/tracing_init.rs:114
- Because
filteris attached directly to the registry above, it is the subscriber-wideEnvFilter, not just a normal-output filter. AnOPENVMM_LOGsetting such aswarnorofftherefore prevents theINFOresume_vmspan and its events from being enabled, so this layer cannot consume all events as described. Apply the existing filter only to the fmt/ETW layers if telemetry is intended to be independent of normal logging.
(sub.with(otel_layer), otel_guard)
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pal::windows::disable_hard_error_dialog(); | ||
|
|
||
| tracing_init::enable_tracing()?; | ||
| let _tracing_guard = tracing_init::enable_tracing()?; |
Keep the OTLP exporter HTTP-only to avoid pulling Rustls and AWS-LC dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain involving HTTPS support, exporter shutdown, and OTel event filtering.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/lib.rs:2657
- With
OPENVMM_OTEL=1, this guard is created beforerun_vmm_mesh_host(). A mesh child reachesstd::process::exit(0)insupport/mesh/mesh_process/src/lib.rs:141, which skips destructors, soTracerProviderGuard::dropnever callsshutdown()and the batch exporter can lose the child's spans on exit. Please add an explicit flush/shutdown on that child exit path, or otherwise keep the exporter alive until it drains.
let _tracing_guard = tracing_init::enable_tracing()?;
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
Replace direct OTLP transport with ETW on Windows and Linux user_events, preserve performance-span routing, and document the native collection requirements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
There was a problem hiding this comment.
🟡 Changes recommended
Critical portability and ETW span-handling issues remain, and the promised OTLP export is not implemented.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Guide/src/reference/openvmm/logging.md:37
OTEL_RESOURCE_ATTRIBUTESis shown as settingservice.instance.id, but the implementation builds the resource with onlyservice.nameandservice.versionand never reads that environment variable. The documented attribute is therefore ignored; either merge standard resource environment attributes during initialization or remove it from this example.
OPENVMM_OTEL=1 \
OTEL_RESOURCE_ATTRIBUTES="service.instance.id=boot-test-42" \
openvmm/openvmm_entry/src/lib.rs:2657
- When OpenVMM is launched as a mesh worker,
run_vmm_mesh_host()eventually callsstd::process::exit(0), which skips Rust destructors. This guard therefore is not dropped in that process, so the native processor is never shut down/flushed and buffered spans can be lost; add an explicit shutdown path before the mesh-host exit or make that exit run the guard cleanup.
let _tracing_guard = tracing_init::enable_tracing()?;
openvmm/openvmm_entry/src/tracing_init.rs:69
- This
Layerwrapper does not forward the defaulton_enter/on_exit(or related lifecycle) callbacks toTracelogSubscriber. The wrapped ETW layer therefore cannot maintain span activation/context for non-performance spans; delegate those callbacks while applying the performance-span policy.
#[cfg(windows)]
impl<S> tracing_subscriber::Layer<S> for PerfFilteredEtwLayer
where
S: tracing::Subscriber + for<'span> LookupSpan<'span>,
{
support/otel_tracing/src/lib.rs:61
- This initialization installs only the platform-native ETW/user_events processors; there is no OTLP/HTTP exporter, collector endpoint, or protobuf transport configured anywhere in the new code. As a result,
OPENVMM_OTEL=1cannot provide the OTLP export promised by the PR description. Please either add the OTLP/HTTP exporter or correct the advertised scope before merging.
let provider = SdkTracerProvider::builder()
.with_span_processor(native_processor(service_name)?)
- Files reviewed: 9/10 changed files
- Comments generated: 3
- Review effort level: Lite
Reserve performance-target filtering for formatted stderr and use the standard ETW layer without a custom wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
There was a problem hiding this comment.
🟡 Changes recommended
The promised OTLP export is absent, ETW filtering is incomplete, and worker shutdown can lose pending spans.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
support/otel_tracing/src/lib.rs:61
- This constructs only the ETW/
user_eventsnative processors; no OTLP exporter or HTTPS/protobuf endpoint is configured anywhere in the new path. Consequently,OPENVMM_OTELcannot provide the OTLP collector export promised by the PR description. Either implement the OTLP HTTP exporter/configuration or update the PR contract and documentation to describe native-event export instead.
Guide/src/reference/openvmm/logging.md:37 - This example sets
OTEL_RESOURCE_ATTRIBUTES, butinit_native_layerconstructs the resource with onlyservice.nameandservice.versionand does not read that environment variable.service.instance.idis therefore silently ignored, making the documented configuration ineffective; either wire environment resource attributes into the resource or remove this variable from the example.
openvmm/openvmm_entry/src/lib.rs:2657
- Mesh worker children do not return through
do_main:try_run_mesh_hostterminates them withstd::process::exit(0), which bypassesDrop. Therefore_tracing_guardnever shuts down the provider in worker processes and pending native spans can be lost when OTEL is enabled. Add an explicit flush/shutdown path before that exit (or avoid direct exit while the guard is live).
let _tracing_guard = tracing_init::enable_tracing()?;
openvmm/openvmm_entry/src/tracing_init.rs:108
- The
otelfeature currently callsinit_native_layer, whose only processors are ETW on Windows anduser_eventson Linux; there is no OTLP exporter or HTTPS/protobuf transport in this change. As a result, the collector export described in the PR is not implemented. Either add the promised OTLP path or update the PR scope/description to describe native-event export only.
let (layer, guard) = otel_tracing::init_native_layer("openvmm", build_info.version())
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
Populate the ETW Part C resource allowlist from the fully detected OpenTelemetry Resource instead of maintaining hardcoded keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
Avoid the tracepoint crate on musl targets and complete the no-op processor implementation used by unsupported platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 992a46b3-3fdb-4ef2-a0fb-45f53f497ed9
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings include missing OTLP export, ignored resource attributes, incomplete shutdown and filtering behavior, and non-Windows compilation failure.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
support/otel_tracing/src/lib.rs:63
- This resource is constructed with only
service.nameandservice.version; no standard resource detector or parsing ofOTEL_RESOURCE_ATTRIBUTESis used. Consequently the documentedservice.instance.id=boot-test-42value is ignored and cannot appear in the emitted trace resource. Please load the environment resource attributes or remove the non-functional configuration from the guide.
support/otel_tracing/src/lib.rs:66 - The implementation wires the SDK only to the native ETW/
user_eventsprocessors; it does not configure an OTLP exporter, HTTPS endpoint, or protobuf transport. As written,OPENVMM_OTELcannot provide the OTLP trace export described in the PR, so either the exporter/configuration needs to be added or the PR scope and description should be corrected.
Guide/src/reference/openvmm/logging.md:37
Resource::builder()here only addsservice.nameandservice.version; it does not consumeOTEL_RESOURCE_ATTRIBUTES. Therefore the documentedservice.instance.id=boot-test-42setting has no effect, and the emitted spans omit that identity. Parse the environment resource attributes during initialization or remove this configuration example.
OPENVMM_OTEL=1 \
Guide/src/reference/openvmm/logging.md:31
- This implementation does not provide the OTLP export described by the PR: it only writes completed spans to ETW or Linux
user_events, with no OTLP HTTP/protobuf exporter or collector endpoint configuration. Either add the promised HTTPS/protobuf exporter and its configuration, or revise the PR description/title and scope to describe native trace emission instead.
Build OpenVMM with the `otel` feature, then set `OPENVMM_OTEL=1` to emit
enabled spans through the platform's native tracing subsystem: ETW on Windows
and `user_events` on GNU/Linux. Other platforms do not currently have a native
openvmm/openvmm_entry/src/lib.rs:2657
- This guard is not dropped on the mesh-worker path:
run_vmm_mesh_hosteventually reachesmesh_process::try_run_mesh_host, which callsstd::process::exit(0), bypassing Rust destructors. The provider therefore never runs its shutdown/flush for the normal worker-host exit, so pending native telemetry can be lost; make that path return before terminating or explicitly shut down the provider before exit.
let _tracing_guard = tracing_init::enable_tracing()?;
openvmm/openvmm_entry/src/tracing_init.rs:114
- The Windows ETW layer is attached after this new OpenTelemetry layer, but it is still added with
sub.with(etw)and does not useexclude_perf_targets(). As a result,openvmm::perfspans remain in the existing ETW logging stream even though the PR says normal fmt/ETW logging should exclude them; apply the same per-layer filter toetw.
(sub.with(otel_layer), otel_guard)
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved span flushing, ETW filtering, OTLP export, and resource attribute issues remain.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
support/otel_tracing/src/lib.rs:63
- The Guide example sets
OTEL_RESOURCE_ATTRIBUTES, but this resource is built only fromservice.nameandservice.version;Resource::builder()does not run the environment resource detector. Consequently attributes such asservice.instance.id=boot-test-42are silently absent from exported spans. Merge the SDK's environment resource detector (or explicitly parse the variable) before constructing the provider, while preserving the existing platform-specific resource handling.
support/otel_tracing/src/lib.rs:52 - This implementation only creates platform-native ETW/user_events processors; it never configures an OTLP exporter, HTTPS endpoint, or protobuf encoding. That does not implement the PR's stated opt-in OTLP export contract. Either add and configure the OTLP HTTP exporter, or revise the feature's contract and documentation to explicitly make native tracing the intended behavior.
openvmm/openvmm_entry/src/lib.rs:2657
_tracing_guardis never dropped in the mesh-host path:run_vmm_mesh_host()eventually callsstd::process::exit(0)when it recognizes a child, which bypassesTracerProviderGuard::drop. The provider's shutdown/flush is therefore skipped, so completed spans buffered by a native processor can be lost. Add an explicit shutdown before the mesh-host exit, or provide a shutdown hook to that path.
let _tracing_guard = tracing_init::enable_tracing()?;
openvmm/openvmm_entry/src/tracing_init.rs:98
- Applying
exclude_perf_targetsonly tofmt_layerleaves the existing WindowsTracelogSubscriberunfiltered (the ETW layer is attached later without this filter). As a result,openvmm::perfspans remain eligible for the normal ETW output, contrary to the stated separation. Apply the same layer filter to the ETW subscriber before attaching it.
.with_filter(exclude_perf_targets());
openvmm/openvmm_entry/src/tracing_init.rs:109
- This initialization path only installs the native ETW/
user_eventsprocessor; it never creates an OTLP exporter or configures HTTPS/protobuf collector transport. As a result,OPENVMM_OTEL=1cannot provide the OTLP trace export described by the PR, and the new documentation explicitly says traces are not sent to a collector. Please either add the promised OTLP/HTTP exporter (including its endpoint/configuration) or update the PR scope and description to the native-sink design.
let (layer, guard) = otel_tracing::init_native_layer("openvmm", build_info.version())
.context("failed to initialize OpenTelemetry tracing")?;
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add opt-in OTLP trace export, reserve performance spans from normal logging, and instrument VM startup.
Design decisions: