Make broker diagnostic logs opt-in#933
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBroker tracing is now opt-in via AGENT_RELAY_BROKER_LOG (or RUST_LOG); init_tracing accepts a broker_id and chooses Off/Stderr/File, PTY tracing targets/syntax were aligned, SDK helpers and CLI ChangesBroker Tracing Configuration
Sequence Diagram(s)sequenceDiagram
participant UserCLI as User CLI
participant CLI as registerLogCommands
participant SDK as broker-logs SDK
participant FS as Filesystem
participant Broker as runtime::init_tracing
UserCLI->>CLI: run `log view|list|rotate|clear`
CLI->>SDK: call listBrokerLogs / tailBrokerLog / pruneBrokerLogs / clearBrokerLogs
SDK->>FS: read dir / stat / unlink / tail file
CLI->>Broker: start broker with broker_id -> init_tracing(broker_id)
Broker->>FS: resolve/create log dir when File destination selected
Broker->>Broker: install tracing EnvFilter and writer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Previously AGENT_RELAY_BROKER_LOG=off was the default. Move the default to a per-broker log file under the user's home so diagnostics are captured without contaminating PTY streams, and reshape the env var as a destination switch: `off` to disable, `stderr`/`print` to keep the legacy on-terminal behavior, anything else (including unset) routes to the file. `RUST_LOG` continues to control the level filter and now defaults to `info` when logging is enabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/src/runtime/util.rs`:
- Around line 295-304: The test log_file_path_uses_agentworkforce_logs_dir is
asserting a Unix-style string slice against path.to_string_lossy(), which fails
on Windows; change the assertion to be platform-agnostic by constructing the
expected suffix with PathBuf joins or comparing Path components instead of a raw
string. Locate the test function log_file_path_uses_agentworkforce_logs_dir and
replace the contains(".agentworkforce/relay/logs/my-broker.log") check with a
comparison that builds
PathBuf::from(".agentworkforce").join("relay").join("logs").join("my-broker.log")
(or iterates and compares the tail components of the returned Path) so the test
passes on all OSes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba8ed168-b062-44bd-995f-82ec7b3b6334
📒 Files selected for processing (3)
CHANGELOG.mdcrates/broker/src/cli/mod.rscrates/broker/src/runtime/util.rs
The broker now writes its tracing log to a platform-standard directory
with daily rotation via tracing-appender:
- macOS: ~/Library/Logs/agent-relay/{brokerId}.log.YYYY-MM-DD
- Linux: ~/.local/state/agent-relay/logs/{brokerId}.log.YYYY-MM-DD
- Windows: %LOCALAPPDATA%\agent-relay\Logs\{brokerId}.log.YYYY-MM-DD
Rotation alone does not bound disk use, so add an `agent-relay log`
command group (path / list / view / rotate / clear) backed by new
@agent-relay/sdk helpers (`getBrokerLogDir`, `listBrokerLogs`,
`tailBrokerLog`, `pruneBrokerLogs`, `clearBrokerLogs`). `log rotate
--keep-days N` provides explicit retention; the active un-suffixed file
is always kept.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/broker/src/runtime/util.rs (1)
156-158: ⚡ Quick winConsider logging or falling back to stderr when directory creation fails.
When
create_dir_allfails at line 156,init_tracingreturns without installing any subscriber, silently disabling all broker diagnostics. Users won't know why logs aren't appearing. Consider either:
- Falling back to stderr when the file destination is unavailable, or
- Writing a diagnostic to stderr before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/runtime/util.rs` around lines 156 - 158, The init_tracing function currently returns silently if std::fs::create_dir_all(&log_dir) fails, losing diagnostics; update init_tracing to handle that error by either falling back to a stderr-based subscriber or at minimum writing a diagnostic message to stderr (e.g., using eprintln! or tracing_subscriber::fmt::writer::BoxMakeWriter::new(std::io::stderr)) before returning; ensure the code references the existing log_dir and create_dir_all call and installs a subscriber that logs to stderr as a fallback so broker diagnostics remain visible when file-based logging cannot be created.packages/sdk/src/broker-logs.ts (1)
213-213: 💤 Low valueMinor inefficiency in loop condition.
Line 213 uses
newlines <= lines, meaning the loop reads one extra chunk even after collecting enough newlines. Changing tonewlines < lineswould stop as soon as sufficient newlines are found. Not a correctness issue, just a small performance optimization when tailing large files.♻️ Suggested change
- while (position > 0 && newlines <= lines) { + while (position > 0 && newlines < lines) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/broker-logs.ts` at line 213, Update the loop condition in the tailing logic to avoid reading one unnecessary chunk: in the function where variables position, newlines, and lines are used (the while loop currently written as "while (position > 0 && newlines <= lines)"), change the condition to "newlines < lines" so the loop exits as soon as we've collected the requested number of newlines; keep the rest of the logic (position decrement, buffer read, and newlines counting) unchanged.CHANGELOG.md (1)
78-79: ⚡ Quick winSimplify changelog entries to be more concise and impact-first.
These entries include detailed implementation specifics (platform directory paths, filename suffixes, individual subcommand enumeration) which make them verbose. As per coding guidelines, changelog entries should be "concise and impact-first, with one short bullet per user-visible change" and "drop implementation backstory."
Consider condensing to:
- Line 78: "Broker diagnostic logs now write to platform-standard directories with daily rotation. Set
AGENT_RELAY_BROKER_LOG=offto disable or useRUST_LOG=...for filtering."- Line 79: "Added
agent-relay logcommands (path, list, view, rotate, clear) to inspect and manage broker logs."As per coding guidelines: "Changelog entries should be concise and impact-first, with one short bullet per user-visible change. Name the command, API, or schema touched and the practical effect. Drop issue/PR links, internal review notes, implementation backstory, and 'foundation for...' phrasing unless that text clearly explains the shipped impact."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 78 - 79, The two CHANGELOG bullets are overly detailed; replace the long platform/path-specific first bullet with a concise, impact-first line stating that "Broker diagnostic logs now write to platform-standard directories with daily rotation" and mention the environment toggles `AGENT_RELAY_BROKER_LOG=off` and `RUST_LOG=...` for disabling and filtering, and replace the second verbose line with a short note that "Added `agent-relay log` commands to inspect and manage broker logs" (you may list the command group name `agent-relay log` but drop the long subcommand enumeration and implementation details).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 78-79: The two CHANGELOG bullets are overly detailed; replace the
long platform/path-specific first bullet with a concise, impact-first line
stating that "Broker diagnostic logs now write to platform-standard directories
with daily rotation" and mention the environment toggles
`AGENT_RELAY_BROKER_LOG=off` and `RUST_LOG=...` for disabling and filtering, and
replace the second verbose line with a short note that "Added `agent-relay log`
commands to inspect and manage broker logs" (you may list the command group name
`agent-relay log` but drop the long subcommand enumeration and implementation
details).
In `@crates/broker/src/runtime/util.rs`:
- Around line 156-158: The init_tracing function currently returns silently if
std::fs::create_dir_all(&log_dir) fails, losing diagnostics; update init_tracing
to handle that error by either falling back to a stderr-based subscriber or at
minimum writing a diagnostic message to stderr (e.g., using eprintln! or
tracing_subscriber::fmt::writer::BoxMakeWriter::new(std::io::stderr)) before
returning; ensure the code references the existing log_dir and create_dir_all
call and installs a subscriber that logs to stderr as a fallback so broker
diagnostics remain visible when file-based logging cannot be created.
In `@packages/sdk/src/broker-logs.ts`:
- Line 213: Update the loop condition in the tailing logic to avoid reading one
unnecessary chunk: in the function where variables position, newlines, and lines
are used (the while loop currently written as "while (position > 0 && newlines
<= lines)"), change the condition to "newlines < lines" so the loop exits as
soon as we've collected the requested number of newlines; keep the rest of the
logic (position decrement, buffer read, and newlines counting) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 591dee0c-717d-49b8-ad93-ab74d0932000
📒 Files selected for processing (8)
CHANGELOG.mdcrates/broker/src/runtime/util.rspackages/sdk/src/__tests__/broker-logs.test.tspackages/sdk/src/broker-logs.tspackages/sdk/src/index.tssrc/cli/bootstrap.test.tssrc/cli/bootstrap.tssrc/cli/commands/log.ts
Add a Broker diagnostic logs section to the broker-lifecycle page covering the platform-standard paths, the AGENT_RELAY_BROKER_LOG values, and the new agent-relay log subcommands. Mirror the SDK surface (getBrokerLogDir / listBrokerLogs / tailBrokerLog / pruneBrokerLogs / clearBrokerLogs) in the TypeScript SDK reference, and add a `log` section to the CLI reference plus a mention in the CLI overview. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Tests