feat(kvp): add diagnostics layer over KvpPoolStore - #313
feat(kvp): add diagnostics layer over KvpPoolStore#313Peyton Robertson (peytonr18) wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 95.70% 96.04% +0.33%
==========================================
Files 23 24 +1
Lines 7705 8360 +655
==========================================
+ Hits 7374 8029 +655
Misses 331 331 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a DiagnosticsKvp layer in libazureinit-kvp to provide a typed, reusable view over KvpPoolStore for diagnostic telemetry (event key formatting/parsing, UTF-8-safe chunking, and chunk reassembly), and adds new diag CLI subcommands to inspect this decoded event view.
Changes:
- Added
DiagnosticsKvp,DiagnosticEvent, andDiagnosticRecordwith chunking/reassembly and record classification logic. - Added
diagCLI subcommands (dump,events,tail,clear) for decoding and filtering diagnostic events (including JSON output support). - Added integration and CLI tests covering round-trips, chunking, classification, and clear scoping; enabled
uuidv4 generation via crate features.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libazureinit-kvp/tests/diagnostics.rs | New integration tests for diagnostics emit/read, chunking, classification, scoped clear, and concurrency behavior. |
| libazureinit-kvp/tests/cli.rs | New CLI tests validating diag command behaviors for text/JSON output, filtering, tailing, and clear confirmation. |
| libazureinit-kvp/src/lib.rs | Exposes the new diagnostics module and re-exports its public types/constants. |
| libazureinit-kvp/src/error.rs | Adds a dedicated error for rejecting ` |
| libazureinit-kvp/src/diagnostics.rs | Implements the diagnostics layer: event key format/parse, UTF-8 chunking, reassembly, classification, and scoped clearing. |
| libazureinit-kvp/src/cli.rs | Adds diag subcommands and renders diagnostics records/events in text and JSON. |
| libazureinit-kvp/Cargo.toml | Enables uuid v4 feature required for Uuid::new_v4(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| event_prefix: String, | ||
| /// Confirm the removal (required). | ||
| #[arg(long)] | ||
| yes: bool, |
There was a problem hiding this comment.
yes is overkill i think. It's volatile data and likely past the window of collection if someone is using it. Would suggest removing it.
There was a problem hiding this comment.
Removed this
| Clear { | ||
| /// VM identifier whose events to remove. | ||
| #[arg(long)] | ||
| vm_id: String, |
There was a problem hiding this comment.
this seems more like "delete" than "clear" to me.
I would expect clear to nuke all diagnostics-based keys.
There was a problem hiding this comment.
I've adjusted this so clear gets rid of all diagnostic keys!
Append a chunk index to diagnostic event keys so the Hyper-V host retains all chunks. Reassembly and cleanup now operate on the base event key to correctly reconstruct and delete multi-record events.
…d clear --diagnostics
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
libazureinit-kvp/src/diagnostics.rs:455
reassemble()currently concatenates any consecutive records whosebase_event_key()matches, which also merges adjacent raw records that happen to share the same key (e.g., two consecutivePROVISIONING_REPORTrecords appended viaKvpPoolStore::append). That loses record boundaries and contradicts the doc comment that grouping is for event-chunk reassembly.
while dumped
.peek()
.is_some_and(|(next, _)| base_event_key(next) == base)
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
libazureinit-kvp/src/diagnostics.rs:490
records()/reassemble()assumes that all chunks for a given event remain consecutive in on-disk order. Whileemit()writes chunks contiguously under one lock, other store operations (notablyKvpPoolStore::delete_multiple, which swaps deletions with the tail and does not preserve order) can break that contiguity and lead to incorrect reassembly for surviving multi-chunk events. Consider either documenting this invariant explicitly or making reassembly robust to record reordering.
/// Records are returned in on-disk order. Consecutive records that
/// share an event key — ignoring the `|<subevent_index>` suffix — are
/// one event; because [`emit`](Self::emit) writes an event's chunks
/// contiguously under a single lock, reassembly is correct even under
/// concurrent writers.
f53953f to
0d011f2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
libazureinit-kvp/src/cli.rs:698
diagnostics_event_jsonmovesStringfields (name,event_id,message) out of a borrowed&DiagnosticEvent, which won’t compile. Serialize these fields by reference (or clone) instead.
fn diagnostics_event_json(event: &DiagnosticEvent) -> serde_json::Value {
json!({
"kind": "event",
"level": event.level.to_string(),
"name": event.name,
"event_id": event.event_id,
"message": event.message,
})
libazureinit-kvp/src/diagnostics.rs:596
reassemble()compares*next == basewherenextis a borrowed&Stringfrompeek(). Dereferencing attempts to move theStringout of the peeked reference and won’t compile. Compare by reference instead.
while let Some((base, index, value)) = parsed.next() {
let mut indexed = vec![(index, value)];
while parsed.peek().is_some_and(|(next, _, _)| *next == base) {
let (_, next_index, next_value) =
parsed.next().expect("peeked value exists");
|
I tried testing on older cloud-init versions using |
|
Is there a way to write diagnostics via CLI? |
Summary
This PR adds a
DiagnosticsKvplayer that centralizes azure-init's KVP telemetry behavior, including event key formatting, message chunking, and message reassembly. This provides a single tested implementation that can be reused by the follow-up wiring PR and simplifies the existing logic inkvp.rsandlogging.rs.It also extends the existing
dumpcommand with a--parse-diagnosticsmode for decoding and viewing KVP telemetry as readable events, making provisioning diagnostics easier to inspect and troubleshoot.Specifically, this PR adds:
DiagnosticsKvp, a typed view overKvpPoolStorethat implements telemetry-specific behavior while keeping the underlying store format-agnostic:<prefix>|<vm_id>|<level>|<name>|<event_id>.append_multipleunder a single lock. Each chunk gets a unique|<subevent_index>-suffixed key so the Hyper-V host (which keeps one record per key) retains every fragment, and the chunks are reassembled when read.PROVISIONING_REPORT, or malformed event keys.Diagnostics folded into the existing raw KVP commands via flags rather than a separate command tree:
dump --parse-diagnosticsreassembles chunked events and decodes each record. Raw output remains the default when the flag is absent.--include-rawalso prints raw (non-event) records;--level,--name, and-n/--tailnarrow the output to a filtered, events-only view.--level,--name,-n/--tail) require--parse-diagnostics, and--include-rawis mutually exclusive with them — it applies only to the unfiltered view, where raw records can appear.clear --diagnosticsremoves every diagnostic key (events and malformed event keys) while leaving raw records such asPROVISIONING_REPORTintact.Example
A long event is stored as multiple records, each under a unique
|<subevent_index>key. The raw view shows the fragments;--parse-diagnosticsdecodes and reassembles them into a single event: