feat: layered text output format (render::text + --format text) - #333
Merged
Conversation
Add a human-readable, record-level rendering of MRT records: - render::text::format_record: pure function of &MrtRecord producing one indented block per record — session context, UPDATE withdrawn/announced sections (folding in MP_REACH/MP_UNREACH prefixes), every path attribute, OPEN capabilities, session states, RIB entries, peer tables, and full legacy type-5 records. RFC 7606 findings render under WARNINGS:. Format is our own design on the crate's Display vocabulary; inspired by bgpdump's human-readable output, not byte-compatible. - CLI: --format text, record-level (implies --level records; other formats are elem-level, documented on the enum variants) - Filter semantics made explicit: no-elem records (KEEPALIVE/OPEN/state changes) never match elem filters and drop from record iteration while filters are active, now noted with a debug! line; behavior covered by integration tests
Contributor
There was a problem hiding this comment.
Pull request overview
Adds layered, record-level human-readable MRT rendering to the library and CLI.
Changes:
- Adds
render::text::format_record. - Adds CLI
--format textand documents record filter semantics. - Adds rendering and filtering tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/render/text.rs |
Implements layered text rendering and unit tests. |
src/render/mod.rs |
Documents and exports rendering. |
src/lib.rs |
Exposes the render module. |
src/bin/main.rs |
Adds CLI text output. |
src/parser/iters/mod.rs |
Logs filtered records without elems. |
tests/render_text.rs |
Tests stream rendering and filters. |
CHANGELOG.md |
Documents the feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+168
to
+172
| AttributeValue::MpUnreachNlri(nlri) => { | ||
| withdrawn.extend(nlri.prefixes.iter()); | ||
| } | ||
| AttributeValue::MpReachNlri(nlri) => { | ||
| announced.extend(nlri.prefixes.iter()); |
Comment on lines
+346
to
+354
| if iter.peek().is_none() { | ||
| return; | ||
| } | ||
| out.push_str(&format!("{}ATTRIBUTES:\n", INDENT.repeat(depth))); | ||
| for attr in iter { | ||
| if let Some(line) = render_attribute(attr) { | ||
| out.push_str(&format!("{pad}{line}\n")); | ||
| } | ||
| } |
Comment on lines
+210
to
+211
| /// One line per attribute; MP reachability is folded into the prefix lists | ||
| /// above and not repeated here. |
|
|
||
| ### Added | ||
|
|
||
| * **Layered text output format** (`render::text::format_record`, `--format text`): one human-readable, indented block per MRT record — session context (`TIME`/`TYPE`/`FROM`/`TO`), `UPDATE:` sections with withdrawn/announced prefixes (including those carried in MP_REACH/MP_UNREACH) and every path attribute, `OPEN:` capabilities, session states, RIB entries, the peer table, and full legacy type-5 records. RFC 7606 validation findings render under `WARNINGS:` when present. The format is designed around this crate's own models and `Display` vocabulary — inspired by bgpdump's human-readable output, not byte-compatible with it. Rendering is a pure function of the record. In the CLI, `--format text` is **record-level** (implies `--level records`; all other formats are elem-level). |
Comment on lines
+23
to
+24
| /// Layered human-readable text, one block per MRT record (record-level; | ||
| /// implies `--level records`, all other formats are elem-level). |
Comment on lines
+1
to
+3
| //! Behavior of `--format text` building blocks: record-level rendering and | ||
| //! the documented filter semantics (non-UPDATE records are dropped when | ||
| //! filters are active). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
- Coverage 90.96% 90.82% -0.14%
==========================================
Files 100 102 +2
Lines 24891 25567 +676
==========================================
+ Hits 22642 23222 +580
- Misses 2249 2345 +96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- fold labeled (MPLS) announcements into an ANNOUNCED (labeled) section and surface link-state / flowspec NLRI counts, so no MP routes vanish from the transcript - render the warnings block even when the attribute list is empty (RIB entries can carry missing-attribute findings with no attributes) - RIB_GENERIC header includes the route prefix - fix the render_attribute doc comment: prefixes are folded, the attribute summary line is retained - docs: text always uses record-level output; other formats follow --level (elem-level by default) — enum variant, changelog, and test docs corrected - tests: labeled/flowspec/link-state sections (incl. no-sections for empty collections) and warnings-without-attributes rendering
Member
Author
|
All 7 review comments addressed in 4f7b5a2:
Gates: |
Contributor
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.
Suppressed comments (7)
Previously missed (5) — in code that hasn't changed since the last review.
src/bin/main.rs:266
--format text -enow takes the record pipeline even though-eis a count-only operation. With filters,run_recordscounts every elem in any record having one match, whereas the elem pipeline counts only matching elems; an UPDATE with two prefixes and a filter matching one therefore reports 2 instead of 1. Keep the format override limited to actual output runs.
let use_elem_stream = ((opts.elems_count && !opts.records_count)
|| (!opts.elems_count && !opts.records_count && matches!(opts.level, OutputLevel::Elems)))
&& output_format != OutputFormat::Text;
src/render/text.rs:120
- This tests whether any optional parameter exists, not whether any capability exists. An OPEN containing only
ParamValue::Rawtherefore prints an emptyCAPABILITIES:section. Collect capability values first and emit the section only when that collection is nonempty.
if open.opt_params.is_empty() {
return;
src/render/text.rs:192
NetworkPrefix'sDisplayimplementation omitspath_id, so ADD-PATH withdrawals lose the identifier that distinguishes multiple paths for the same prefix. Use itsDebugrepresentation (which includes#<path_id>) or renderpath_idexplicitly.
This issue also appears on line 196 of the same file.
for prefix in withdrawn {
out.push_str(&format!("{INDENT}{INDENT}{prefix}\n"));
src/render/text.rs:356
- For
Rib*AddPathrecords, the parser stores the path identifier inRibEntry::path_id, but this RIB rendering drops it. Distinct paths from the same peer can consequently produce identical blocks; renderPATH_IDwhen present.
This issue also appears on line 369 of the same file.
out.push_str(&format!(
"{INDENT}RIB_ENTRY:\n{INDENT}{INDENT}PEER_INDEX: {}\n{INDENT}{INDENT}ORIGINATED: {}\n",
entry.peer_index, entry.originated_time
));
src/render/mod.rs:5
- “Full-fidelity” overstates this API: notification payload bytes, raw attribute bytes, peer-table metadata, and geo-peer details are summarized or omitted. Describe this as a human-readable transcript unless those fields are also rendered.
The [`text`] module renders one [`MrtRecord`](crate::MrtRecord) as a layered, indented text
block — a full-fidelity transcript of the record: BGP4MP session context,
withdrawn and announced prefixes, every path attribute, and RFC 7606
src/render/text.rs:197
- ADD-PATH announcement identifiers are silently discarded here because
NetworkPrefix::fmt(Display)prints only the IP prefix. This can make two distinct advertised paths render identically; preserve the identifier in the text output.
for prefix in announced {
out.push_str(&format!("{INDENT}{INDENT}{prefix}\n"));
src/render/text.rs:372
- The generic RIB entry has the same optional ADD-PATH identifier, but it is not represented in this block. Preserve
entry.path_idwhen present soformat_recorddoes not collapse distinct generic paths.
out.push_str(&format!(
"{INDENT}RIB_ENTRY:\n{INDENT}{INDENT}PEER_INDEX: {}\n{INDENT}{INDENT}ORIGINATED: {}\n",
entry.peer_index, entry.originated_time
));
Comment on lines
+210
to
+214
| out.push_str(&format!( | ||
| "{INDENT}{INDENT}{} labels=[{}]\n", | ||
| labeled.prefix, | ||
| labels.join(", ") | ||
| )); |
Member
Author
|
Addressed in 82534f4: labeled prefixes now render their ADD-PATH identifier when present — |
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.
Summary
Adds a layered, human-readable text output format for MRT records: a new
rendermodule in the library (render::text::format_record) and a--format textvariant in the CLI. One indented block per record — session context, prefixes, every path attribute, and RFC 7606 findings. Inspired by bgpdump's human-readable output; the format itself is our own design, built from the crate's existingDisplayvocabulary rather than replicating bgpdump byte-for-byte.render::text::format_record(&MrtRecord) -> String— a pure function of the record (no iterators, no I/O). Covers BGP4MP messages and state changes, legacy type-5 records, TABLE_DUMP / TABLE_DUMP_V2 RIB entries, and the peer index table. UPDATE blocks fold MP_REACH/MP_UNREACH prefixes into theANNOUNCED:/WITHDRAWN:sections so routes are never silently dropped from the transcript, and validation findings render underWARNINGS:.--format text— record-level; implies--level records. All other formats are elem-level, documented on the enum variants (and in--help).debug!line. A matching UPDATE still passes as a whole record (attributes print once — the opposite of elem-level fan-out).CLI output samples
Updates stream (real 1999 RIS rrc00 capture — legacy type-5 records rendered in full):
With a prefix filter (records carrying a matching elem pass as a whole):
RIB dump (bview file; legacy TABLE_DUMP batches render entry by entry):
Malformed record — RFC 7606 findings surface under
WARNINGS:(crafted record: ORIGIN with wrong flags plus a duplicate ORIGIN):Testing
render::text: golden block for a full UPDATE, warnings section ordering, MP_REACH prefix folding, state change / OPEN / KEEPALIVE, legacy type-5 message and state change, peer table + RIB entries.tests/render_text.rs: record stream rendering and the filter semantics (KEEPALIVEs dropped from record iteration while filters are active, UPDATE survives).cargo test --all-features: 950 passed, 0 failed;cargo clippy --all-targets --all-features -- -D warningsclean;cargo fmt --checkclean; MSRV 1.87 verified.