feat(kvp): add diagnostics layer over KvpPoolStore - #313
Peyton Robertson (peytonr18) wants to merge 33 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.67% +0.97%
==========================================
Files 23 29 +6
Lines 7705 9962 +2257
==========================================
+ Hits 7374 9631 +2257
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
| | Command | Output | | ||
| |---------|--------| | ||
| | `libazureinit-kvp dump` | JSON array of physical key/value records in pool order, including duplicates | | ||
| | `libazureinit-kvp dump --parse` | JSON array of diagnostics and reports in timestamp order, then raw entries | |
There was a problem hiding this comment.
i think pool ordering should continue to be maintained here.
There was a problem hiding this comment.
Added this in latest commit, reverted the timeline sorting and fixed all the tests and such accordingly.
| | `libazureinit-kvp dump` | JSON array of physical key/value records in pool order, including duplicates | | ||
| | `libazureinit-kvp dump --parse` | JSON array of diagnostics and reports in timestamp order, then raw entries | | ||
| | `libazureinit-kvp dump --text` | Physical records as `KEY=VALUE` lines | | ||
| | `libazureinit-kvp dump --parse --text` | The same timestamp ordering, with binary payloads under `payload_b64` | |
There was a problem hiding this comment.
i'm not sure what this means? binary payloads will have payload_b64=<base64 content>?
There was a problem hiding this comment.
Yes! Sorry, that may not have been obvious enough with that wording, which I've since adjusted a little.
That line is intended to clarify how binary payloads are displayed in --text output.
Internally, decoded diagnostics can contain either text (Text) or raw bytes (Bytes). When the payload is raw bytes, we can't safely print it directly, so the CLI base64-encodes it for display and renders it as payload_b64=. Text payloads continue to be rendered as payload=.
The important detail is that the different key name (payload_b64) signals that the displayed value is a base64 representation of the payload, not the payload itself. This is only a presentation detail for --text output; JSON output preserves the payload structure separately.
| @@ -0,0 +1,512 @@ | |||
| # KVP Diagnostics Specification | |||
There was a problem hiding this comment.
delete before merge? anything of value here that can be moved out? into /doc or docstrings?
There was a problem hiding this comment.
I meant to ask this in our chat -- I could trim this down and put some of the info in kvp.md, but I was also thinking if we choose to ship libazureinit-kvp independently, it might be good to have a /docs section in there as well?
Open to whatever approach but my initial reaction would be to trim it down, put any kvp specific things into kvp.md, and then create a /docs/diagnostics.md or something similar (and possibly move kvp.md into that folder too)
…-kind, drop dev uuid
…al timestamp precisions
| accepts any of those canonical precisions. | ||
| Payloads are plain UTF-8 text or gzip plus base64 (`Encoding::GzB64`). | ||
|
|
||
| Each reader call takes one fresh snapshot and returns `Entry::Diagnostic`, |
There was a problem hiding this comment.
I get why it is referred to as a snapshot, but it's awkward framing. I would suggest scrubbing it and simplifying it some.
| .decode(compact) | ||
| .map_err(|_| DecodeError::Undecodable)?; | ||
| let mut bytes = Vec::new(); | ||
| // cloud-init labels zlib streams and line-wrapped base64 as `gz+b64`. |
There was a problem hiding this comment.
isn't gz+b64 universal here? does zlibdecoder path ever get used?
There was a problem hiding this comment.
Yes!! The zlibdecoder path does get used -- the reason for this is that the gz+b64 label is shared, but the compressed format isn’t always gzip. Cloud-init’s Azure compressed-event producer calls base64.encodebytes(zlib.compress(event_content)) and labels the result gz+b64. That produces a zlib stream, not a gzip stream, despite the label. Base64 turns the compressed bytes into text suitable for KVP storage, but it doesn’t make those compression formats interchangeable.
So, we have to handle zlib too because we read cloud-init records. When cloud-init stores a compressed log, we need the correct decompressor to recover its contents because a gzip-only decoder would fail, leaving those records as Raw with an Undecodable error instead of returning the decoded payload.
| Self { store } | ||
| } | ||
|
|
||
| /// Invalid pool UTF-8 fails the snapshot; entries keep first-seen order. |
There was a problem hiding this comment.
what could cause invalid pool utf-8? is this any underlying pool error?
There was a problem hiding this comment.
Invalid UTF-8 is one specific cause of a pool-read failure, not a catch-all for any underlying pool error.
It could happen if another process writes binary data where the pool expects text, truncates a key or value in the middle of a UTF-8 character, or if the pool contents become corrupted. Those are just plausible causes, not issues we've observed in production. Also, we only validate the actual key/value contents up to the terminating NUL; any padding after that is ignored.
When this happens, our storage API cannot represent the key or value as a Rust string, so the read fails with KvpError::Io(InvalidData). Other underlying pool errors, such as permission failures or file-read errors, are surfaced separately and are not treated as UTF-8 errors.
I mostly introduced that as a safeguard but if you think we don't need to account for any of those scenarios, I can easily remove the invalid poot utf-8 error!
…cumentation. Adopt DIAG and name/VERSION identifiers, add configurable decimal-second durations and zlib+b64 encoding, and update tests while preserving gzip and cloud-init compatibility.
845449e to
6337db3
Compare
|
|
||
| | Condition | Error token | | ||
| |---|---| | ||
| | Unsupported `DIAG_V*` schema | `unsupported_version` | |
There was a problem hiding this comment.
s/DIAG_V/DIAG
There was a problem hiding this comment.
Done in latest!
| /// When this diagnostic was emitted, in UTC. | ||
| #[serde(serialize_with = "serialize_timestamp")] | ||
| pub timestamp: DateTime<Utc>, | ||
| /// Stored payload encoding; `None` means plain text. |
There was a problem hiding this comment.
plaintext -> utf-8 encoded plain-text
There was a problem hiding this comment.
Done in latest!
| pub key: String, | ||
| /// Original value, without payload decoding. | ||
| pub value: String, | ||
| /// Why a recognized record could not be decoded; `None` for unrelated keys. |
There was a problem hiding this comment.
s/unrelated/unknown, or non-diagnmostic, key-pairs
There was a problem hiding this comment.
Done in latest!
| use serde_json::{json, Value}; | ||
|
|
||
| const AGENT: &str = "azure-init-0.1.1"; | ||
| const AGENT: &str = "azure-init/0.1.1"; |
ce8daf1 to
5ee6eeb
Compare
| cannot contain `|` or NUL; there is no key-field escaping. Agent strings remain | ||
| opaque to readers, including unversioned names and older naming conventions. | ||
| UUID spellings are preserved rather than rewritten during reading. |
There was a problem hiding this comment.
| cannot contain `|` or NUL; there is no key-field escaping. Agent strings remain | |
| opaque to readers, including unversioned names and older naming conventions. | |
| UUID spellings are preserved rather than rewritten during reading. | |
| cannot contain `|` or NUL; there is no key-field escaping. |
| carries its own elapsed duration; do not derive it by subtracting wall-clock | ||
| timestamps. Either endpoint remains valid if its counterpart is missing. |
There was a problem hiding this comment.
| carries its own elapsed duration; do not derive it by subtracting wall-clock | |
| timestamps. Either endpoint remains valid if its counterpart is missing. | |
| carries its own elapsed duration to allow caller to accurately measure | |
| the operation of interest without relying on the timestamps of the emitted | |
| diagnostics. |
|
|
||
| Timestamps use RFC 3339 UTC `Z` form, with zero, three, six or nine fractional | ||
| digits. The default is milliseconds, for example `2026-08-31T12:34:56.789Z`. | ||
| Numeric offsets and other fractional widths are invalid in native records. |
There was a problem hiding this comment.
Do we care so long as it conforms to RFC 3339? In this case I would say it MUST conform to RFC 3339, this implementation has options for three forms....
Just a thought. It's fine to keep it constrained.
| Producers can select zero, three, six or nine digits; lower digits are discarded. | ||
|
|
||
| Accept decimal digits with an optional decimal point and one to nine fractional | ||
| digits. The whole-seconds component is at most `18446744073709551615`. Signs, |
There was a problem hiding this comment.
That's a lot of seconds :D
It might be cleaner to state that it MUST conform to IEEE 754 double and >= 0.
|
|
||
| ### Kusto Consumption | ||
|
|
||
| Use `zlib+b64` for new compressed telemetry targeting Kusto's |
There was a problem hiding this comment.
drop this.
| | Agent / name | 32 / 48 bytes | | ||
| | Each UUID | 36 bytes | | ||
|
|
||
| These budgets do not impose equivalent read limits on other producers' records. |
There was a problem hiding this comment.
There isn't really a full accounting here of potential max widths vs. expected/typical widths and how they fit in the available space.
| These budgets do not impose equivalent read limits on other producers' records. | ||
| Encoded size also does not bound decompressed size. | ||
|
|
||
| Invalid metadata, unsupported encodings, duplicate or missing indices, invalid |
There was a problem hiding this comment.
I don't know that this paragraph says much. It's fair to assume that missing/corrupted data will induce errors. Focus on the ones that are worth calling out like the next paragraph (missing trailing chunks) because it was a design consideration.
There was a problem hiding this comment.
and maybe split Limits section from Error Handling or something like that.
| split each decoded field on its first `=`. Field order is not significant for | ||
| reading. | ||
|
|
||
| Required fields are `result`, `agent`, `vm_id`, `pps_type` and an RFC 3339 |
There was a problem hiding this comment.
maybe turn this into a table for easy reading
| uses zlib compression and line-wrapped base64 while labeling the envelope | ||
| `gz+b64`. Remove ASCII base64 whitespace and accept zlib or gzip under that | ||
| source label, using the actual stream format to select the decompressor. | ||
| This exception does not apply to native `gz+b64` records. Other envelope |
There was a problem hiding this comment.
it's implied in here, but maybe call out the reason (cloud-init says gz+b64 but it actually is zlib+b64).
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: