refactor(tracing): add lazy typed trace events#228
Conversation
|
Note Complete: Audit complete. V12 did not find any issues that need review. Open the full results here. Analyzed 23 files, diff |
evan-forbes
left a comment
There was a problem hiding this comment.
net gain LoC, mainly due to typing, isolation, and testing
however imo we're getting a massive readability gain
if we wanted to pursue macros we could likely remove most of the tracing files as well
| fn trace_body_submitted(&self, height: block::Height, token: BlockApplyToken) { | ||
| self.trace.emit_with(BLOCK_SYNC_TABLE, |row| { | ||
| row.insert( | ||
| bs_trace::EVENT.to_string(), | ||
| serde_json::Value::String(bs_trace::BLOCK_BODY_SUBMITTED.to_string()), | ||
| ); | ||
| bs_insert_height(row, bs_trace::HEIGHT, height); | ||
| bs_insert_u64(row, bs_trace::APPLY_TOKEN, token); | ||
| }); | ||
| } |
There was a problem hiding this comment.
followed the refactor we did in headersync where we move the tracing code to it's own file
| ) { | ||
| trace.emit_with(DISCOVERY_TABLE, |row| { | ||
| row.insert( | ||
| d_trace::EVENT.to_string(), | ||
| serde_json::Value::String(d_trace::DISCOVERY_DIAL_RESULT.to_string()), | ||
| ); | ||
| row.insert( | ||
| d_trace::RESULT.to_string(), | ||
| serde_json::Value::String(result.label().to_string()), | ||
| ); | ||
| let peer = ZakuraPeerId::new(node_id.as_bytes().to_vec()) | ||
| .map(|peer_id| peer_label(&peer_id)) | ||
| .ok(); | ||
| row.insert( | ||
| d_trace::PEER.to_string(), | ||
| peer.map_or(serde_json::Value::Null, serde_json::Value::String), | ||
| ); | ||
| }); | ||
| trace.emit_event(|| DiscoveryDialResultEvent::new(node_id.as_bytes(), result)); |
There was a problem hiding this comment.
while net gain in LoC
we're getting imo massive readability gain here using a single line to do the same work as before
| self.trace_get_headers_sent( | ||
| &identity.peer, | ||
| range, | ||
| range.count, | ||
| peer_cap, | ||
| GetHeadersTraceMeta { | ||
| request_id, | ||
| session_id: identity.session_id, | ||
| stream_version: ZAKURA_HEADER_SYNC_STREAM_VERSION, | ||
| }, | ||
| ); | ||
| self.trace_get_headers_sent(range, peer_cap, &identity, request_id); |
There was a problem hiding this comment.
another example
our goal here is just be sane with readability
evan-forbes
left a comment
There was a problem hiding this comment.
net gain LoC, mainly due to typing, isolation, and testing
however imo we're getting a massive readability gain
if we wanted to pursue macros we could likely remove most of the tracing files as well
|
main's history was rewritten to remove the 68 MB Sprout-history artifact from git (old tip To fix: This branch contains a merge commit; add |
f9ddb4b to
d1a42d9
Compare
d1a42d9 to
4cdb79e
Compare
Motivation
Zakura's production JSONL trace events were assembled through mutable raw JSON maps at call sites. This scattered schema ownership across reactors and drivers, made required fields implicit, and performed some peer hashing and diagnostic projection work before knowing whether the trace channel could accept an event.
Solution
JsonlEventEmitterwhile preserving the public raw-map compatibility APItrace_*calls while leaving metrics and ordinarytracinglogs beside the state changes they observeThe block-sync sparse row projection remains internal to block-sync trace modules; it is no longer imported or constructed by reactor, peer-routine, or sequencer core logic.
Size
The initial PR state was GitHub
+3,798/-2,309(net+1,489) and+754production Rust LoC by the counter below. The current PR is GitHub+6,344/-3,960(net+2,384) and+1,289production Rust LoC.Production LoC is defined as non-blank, non-comment physical Rust lines excluding
tests,testkit,benches,bench.rs,tests.rs, and brace-delimited items immediately preceded by#[cfg(test)]; strings and comments are stripped before brace counting. The semantic owner split was kept despite the increase because it removes serialization policy and mutable field assembly from production control flow.Compatibility results
mainTesting
cargo fmt --all -- --checkcargo test -p zakura-jsonl-tracecargo test -p zakura-network --lib trace::testsCXXFLAGS='-include cstdint' cargo test -p zakura --lib trace::testscargo clippy -p zakura-jsonl-trace --all-targets -- -D warningscargo clippy -p zakura-network --lib -- -D warningsCXXFLAGS='-include cstdint' cargo clippy -p zakura --lib -- -D warningspython3 docker/zakura-regtest-e2e/trace_oracle.py --self-testCXXFLAGS='-include cstdint' ZAKURA_E2E_MODE=smoke docker/zakura-regtest-e2e/run.shpython3 .agents/skills/zakura-trace-plots/scripts/plot_zakura_traces.py /tmp/zakura-regtest-e2e-traces-zakura-smoke/node2 --out-dir /tmp/zakura-regtest-e2e-trace-plotsgit diff --checkSpecifications & References