feat!: structured attr logging - #391
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end structured log attributes to Bedrock’s logging pipeline (including UniFFI host logger delivery), enabling better filtering/analysis in log backends while preserving hex-secret redaction and stamping every line with a bedrock_version attribute.
Changes:
- Updates the UniFFI
Logger::logcontract to include anattributesmap, and routes Bedrock macros through a new__bedrock_logpath that supports leadingkey = valuefields. - Introduces a single delivery choke point (
deliver) that redacts secrets in both message and attribute values and always attachesbedrock_version(overriding any caller-supplied value). - Adds Rust/Swift/Kotlin tests and demo helpers to verify attribute round-tripping across the FFI boundary.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| swift/tests/BedrockTests/BedrockToolingTests.swift | Adds a capturing Swift Logger plus a new test validating structured attributes and bedrock_version. |
| kotlin/bedrock-tests/src/test/kotlin/bedrock/BedrockToolingTests.kt | Adds a capturing Kotlin Logger plus a new test validating structured attributes and bedrock_version. |
| bedrock/src/primitives/tooling_tests.rs | Adds ToolingDemo::log_with_attributes to emit fielded logs for foreign tests to assert against. |
| bedrock/src/primitives/logger.rs | Implements structured attributes through the logger trait, delivery path, macros, and dependency log forwarding via tracing subscriber. |
| bedrock/src/backup/manifest.rs | Adds a structured designator field to a critical error log. |
| bedrock/src/backup/backup_format/v0.rs | Adds a structured path field to a deserialization error log. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d2bbfc2. Configure here.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2bbfc2793
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut attributes = ::std::collections::HashMap::new(); | ||
| $( | ||
| attributes.insert( | ||
| ::core::stringify!($key).to_owned(), | ||
| ($val).to_string(), |
There was a problem hiding this comment.
When no logger has been installed, a structured invocation still builds the map and calls Display::to_string for every attribute before log_message_with_attributes reaches its LOGGER_INSTANCE check. This makes supposedly disabled logging allocate and potentially perform expensive or side-effecting formatting in hot paths, unlike ordinary message arguments, whose formatting is skipped. Move the logger-enabled check ahead of attribute conversion or preserve the attribute values lazily until delivery.
Useful? React with 👍 / 👎.
| path = path, | ||
| "Failed to deserialize backup file {path}: {e}" |
There was a problem hiding this comment.
| path = path, | |
| "Failed to deserialize backup file {path}: {e}" | |
| path = path, | |
| error = e.to_string(), | |
| "Failed to deserialize backup file" |
how about this?
| designator = entry.designator, | ||
| "[Critical] Decoded checksum has invalid length for file with designator: {}. Manifest entry is invalid.", | ||
| entry.designator |
There was a problem hiding this comment.
| designator = entry.designator, | |
| "[Critical] Decoded checksum has invalid length for file with designator: {}. Manifest entry is invalid.", | |
| entry.designator | |
| designator = entry.designator, | |
| "[Critical] Decoded checksum has invalid length for file with designator. Manifest entry is invalid." |
and here the designatore is logged in attributes and message
Introduces optional structured attribute logging. This is a breaking change because the function signature of
logchanged. This will let us better filter logs for debugging.Note
Medium Risk
The breaking UniFFI
LoggerAPI requires every host implementation to update at compile time; runtime impact on core logic is limited to observability, with redaction/version stamping centralized in one delivery path.Overview
Adds structured log attributes end-to-end: the UniFFI
Logger::logsignature now takes anattributesmap (breaking change for Swift/Kotlin bridges), and Bedrock macros (info!,error!, etc.) accept leadingkey = valuefields that are delivered as attributes instead of being folded into the message.All records go through a shared
deliverpath that hex-redacts message and attribute values, forwardstracingdependency fields as attributes (notkey=valuesuffixes), and always attachesbedrock_version. A few backup error logs start using structured fields (e.g.path,designator). Rust unit tests plus Kotlin/Swift foreign tests verify attributes and version survive the FFI round-trip.Reviewed by Cursor Bugbot for commit d2bbfc2. Bugbot is set up for automated code reviews on this repo. Configure here.