Repository navigation
Fix stdout configuration format handling and defaults - #1153
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
ApprovabilityVerdict: Needs human review This PR changes the default stdout format from Text to Json and modifies how unsupported formats are handled. Since the default behavior change could affect existing configurations that don't explicitly specify a format, human review is recommended to confirm the intended behavior change. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
This PR addresses configuration/validation issues around the stdout output sink by tightening which format values are accepted, changing the default stdout format, and introducing an explicit text format variant in config parsing.
Changes:
- Update
stdoutsink construction to reject input-only formats (cri,raw,auto), accepttext, and default tojson. - Add
Format::Textto the config model and adjust related config tests. - Minor test-only tweaks (ignore reasons, float literals) and a small signature change in a sink stub.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/logfwd-output/src/lib.rs | Adjusts stdout format selection/erroring and adds/updates unit tests around build_sink_factory. |
| crates/logfwd-config/src/lib.rs | Introduces Format::Text and updates the text-format parsing test. |
| crates/logfwd-output/src/sink.rs | Tweaks the OnceAsyncFactory test stub implementation (currently introduces a trait signature mismatch). |
| crates/logfwd-output/src/arrow_ipc_sink.rs | Updates a test float literal (3.14 → 3.5). |
| crates/logfwd-output/tests/it/elasticsearch_arrow_ipc.rs | Adds explicit ignore reasons for ES-dependent integration tests. |
| } | ||
|
|
||
| fn name(&self) -> &str { | ||
| fn name(&self) -> &'static str { |
There was a problem hiding this comment.
StubSink is implementing Sink::name(&self) -> &str, but this impl changes the return type to &'static str, which does not match the trait signature and will fail to compile. Please change it back to -> &str (you can still return a string literal).
| fn name(&self) -> &'static str { | |
| fn name(&self) -> &str { |
| Some(Format::Cri) | Some(Format::Raw) | Some(Format::Auto) => { | ||
| return Err(OutputError::Construction(format!( | ||
| "output '{name}': format '{}' is only supported for inputs", | ||
| cfg.format.as_ref().unwrap() |
There was a problem hiding this comment.
This error branch uses cfg.format.as_ref().unwrap() in production code. It’s safe in this match arm, but it still introduces an unwrap() in a hot path/config path and can be avoided by binding the matched format (e.g. Some(f @ (Format::Cri | ...))) and formatting f directly.
| Some(Format::Cri) | Some(Format::Raw) | Some(Format::Auto) => { | |
| return Err(OutputError::Construction(format!( | |
| "output '{name}': format '{}' is only supported for inputs", | |
| cfg.format.as_ref().unwrap() | |
| Some(f @ (Format::Cri | Format::Raw | Format::Auto)) => { | |
| return Err(OutputError::Construction(format!( | |
| "output '{name}': format '{f}' is only supported for inputs" |
| let factory = build_sink_factory("test", &cfg, Arc::new(ComponentStats::new())).unwrap(); | ||
| assert_eq!(factory.name(), "test"); | ||
| } |
There was a problem hiding this comment.
This test is intended to cover the new stdout default format behavior, but it only asserts the factory name. As written it won’t catch regressions where the default format changes back to text. Consider asserting the chosen format (e.g. by factoring the stdout-format selection into a small pure helper and unit-testing that, or otherwise making the selected StdoutFormat observable in tests).
| "; | ||
| let cfg = | ||
| Config::load_str(yaml).expect("format: text should be accepted as alias for console"); | ||
| let cfg = Config::load_str(yaml).expect("format: text should be accepted"); | ||
| let pipe = &cfg.pipelines["default"]; | ||
| assert_eq!(pipe.inputs[0].format, Some(Format::Console)); | ||
| assert_eq!(pipe.inputs[0].format, Some(Format::Text)); | ||
| } |
There was a problem hiding this comment.
This test currently asserts pipe.inputs[0].format == Some(Format::Text), but the PR description is about stdout/output format handling. To actually validate the intended behavior, set output.format: text in the YAML and assert against pipe.outputs[0].format (and consider renaming the test since text is no longer an alias for console).
Triage ReviewStatus: CHANGES_REQUESTED — mostly correct but one behavior change needs discussion, and conflicts to flag. What's good
Behavior change: default stdout format changed from Text → Json// Before
_ => StdoutFormat::Text,
// After
Some(Format::Json) | None => StdoutFormat::Json,The previous default for Is this intentional? If so it needs to be called out in the PR description and changelog. If not, change the default arm to Conflict with PR #1148 (CHANGES_REQUESTED)Both PRs modify CITests failing — pre-existing |
- Reject input-only formats (`cri`, `raw`, `auto`) for `stdout` sink. - Add missing `Text` variant to `Format` and remove alias from `Console` for `text` mapping. - Set default `stdout` output format to `Json`. - Add regression tests to prevent reappearance. - Address a few lingering clippy complaints. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
8198351 to
bd18e9c
Compare
Fixes 3 configuration bugs related to the
stdoutsink.stdoutvalidation now correctly rejects input-only formats (cri,raw,auto).stdoutdefaults tojsoninstead oftext.textformat is now properly accepted as a format option and mapped correctly.Also fixes a couple small clippy warnings.
PR created automatically by Jules for task 10764699487710943234 started by @strawgate
Note
Fix stdout sink format handling to reject input-only formats and default to JSON
Format::Textvariant in lib.rs, replacing the serde alias that previously mappedtexttoConsole.build_sink_factoryin lib.rs to explicitly rejectCri,Raw, andAutoformats for stdout sinks with a descriptive error, and mapsFormat::TexttoStdoutFormat::Text.format: textin config now yieldsFormat::Textinstead ofFormat::Console.Macroscope summarized bd18e9c.