feat(telemetry): fix escapes, add alerts, and add test utilities - #1753
feat(telemetry): fix escapes, add alerts, and add test utilities#1753jbuckmccready wants to merge 3 commits into
Conversation
46fd4c1 to
37823ad
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the telemetry logging system by fixing logfmt escaping/quoting, adding a first-class “alert” field for targeted operational signaling, and introducing test utilities to capture and assert on structured logs in in-process tests.
Changes:
- Added
Alertsupport and.alert(.dev | .operator)logging fields, plus test-time panic policies for alert emission. - Refactored log encoding to use precomputed encoding plans (byte lengths + quoting decisions) to avoid repeated formatter passes and improve correctness for escaping.
- Introduced
TestLogStoreand integrated it into the gossipTestNodeharness to capture logs and enforce “no alerts emitted” behavior by default.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| v2/lib/telemetry/tests/TestLogStore.zig | New in-memory structured log capture utility for tests, with optional panic-on-alert behavior. |
| v2/lib/telemetry/tests.zig | Ensures TestLogStore tests are included in the telemetry test suite. |
| v2/lib/telemetry/log.zig | Adds Alert, introduces encoding plans, and implements logfmt quoting/escaping fixes with new tests. |
| v2/lib/telemetry.zig | Exposes TestLogStore, adds .alert() to the logger API, and uses encoding plans during logging. |
| v2/lib/gossip/tests/TestNode.zig | Integrates TestLogStore into the gossip test harness and routes node logs into it. |
Comments suppressed due to low confidence (1)
v2/lib/telemetry/log.zig:1024
LogfmtEscapingWriter.drainassumesdata.len >= 1and will underflow / OOB if invoked with an empty slice list. Add an assertion (or early return) to guard against that case.
const self: *LogfmtEscapingWriter = @alignCast(@fieldParentPtr("writer", w));
const rest = data[0 .. data.len - 1];
const pattern = data[data.len - 1];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const self: *LogfmtValueAnalyzer = @alignCast(@fieldParentPtr("writer", w)); | ||
| const rest = data[0 .. data.len - 1]; | ||
| const pattern = data[data.len - 1]; |
There was a problem hiding this comment.
Added assertions (it's required by the std Writer.drain contract to be non-zero): 7eddd95
| /// Formatted output from function provided must be valid UTF-8 and identical on every call (as | ||
| /// visible by this formatter). Encoding-plan computation and output formatting invoke the formatter | ||
| /// separately (to produce a byte length prefix), so differing output between calls corrupts that | ||
| /// length-prefix. |
There was a problem hiding this comment.
Adjusted the comment for clarity: 9d827ca
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
9d827ca to
478a33f
Compare
244acdf to
b1bf333
Compare
478a33f to
e24e290
Compare
b1bf333 to
5f924fb
Compare
e24e290 to
a81c79f
Compare
| const AlertValue = struct { | ||
| const value = @tagName(audience); | ||
| }; | ||
| return self.field("alert", &AlertValue.value); |
There was a problem hiding this comment.
Did you try this already and it had a problem?
| const AlertValue = struct { | |
| const value = @tagName(audience); | |
| }; | |
| return self.field("alert", &AlertValue.value); | |
| return self.field("alert", @tagName(audience)); |
tagName already returns a pointer. You don't need a pointer to a pointer. I think you can eliminate this struct too since the lifetime of the pointer returned by tagName should be static.
510face to
33e65e7
Compare
a81c79f to
841ddd5
Compare
33e65e7 to
ba65023
Compare
Note Writer.drain contract requires data.len != 0, that's why this is an assert and not an early return.
841ddd5 to
bddebca
Compare
Summary
.alert(.dev | .operator)logging fields with configurable panic policies in tests (logged alert concept is described in this PR: test(gossip): pilot for component tests #1744)TestLogStorefor capturing, iterating, inspecting, and resetting structured logs.NOTE: no tests inspect logs in this PR since there isn't really a good use case currently in gossip, but it still asserts there is no logged alerts (using
TestLogStorein the TestNode gossip harness). It would look something like this: