fix: CLI, OTLP receiver, and diagnostics server bugs#757
Conversation
Fixes #682, #681, #708, #686, #687, #715, #728. - main.rs: --generate-json 0 no longer prints "avg NaN bytes/line" (#682) - main.rs: --blackhole validates address with parse::<SocketAddr>() so invalid addresses exit 1, not 0 (#708); diagnostics port uses 0 so it never collides with an in-use port (#681) - otlp_receiver: /v1/logsFOO and /v1/logs/extra now return 404 — only exact /v1/logs path is accepted (#686) - otlp_receiver: Content-Type matching is now case-insensitive per RFC 7231 — Application/JSON is treated as JSON (#687) - diagnostics: non-GET requests return 405 Method Not Allowed (#728) - diagnostics: /metrics returns 410 Gone with pointer to /api/pipelines instead of a generic 404 (#715) New tests for every fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd/src/main.rs`:
- Around line 230-232: The runtime prints a dashboard URL using
config.server.diagnostics verbatim which becomes useless when diagnostics was
set to ephemeral port 0 in the YAML; update the startup path that handles the
blackhole/diagnostics flow (where yaml is built and the dashboard URL is
printed) to either suppress printing the URL when config.server.diagnostics has
port 0 (i.e. detect ":0") or, preferable, after creating the tiny_http server,
call tiny_http::Server::server_addr() to get the actual bound address and
propagate that address into the value used for the printed dashboard URL so
users see the real host:port; change code around the yaml/config usage and the
dashboard print logic to use the resolved address when available.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9d725bda-78fa-48d3-9363-9204d9e86640
📒 Files selected for processing (3)
crates/logfwd-io/src/diagnostics.rscrates/logfwd-io/src/otlp_receiver.rscrates/logfwd/src/main.rs
| // Use port 0 for diagnostics so it never collides with an in-use port. | ||
| let yaml = format!( | ||
| "input:\n type: otlp\n listen: {addr}\noutput:\n type: null\nserver:\n diagnostics: 127.0.0.1:9090\n" | ||
| "input:\n type: otlp\n listen: {addr}\noutput:\n type: null\nserver:\n diagnostics: 127.0.0.1:0\n" |
There was a problem hiding this comment.
Ephemeral port prevents collisions but the printed dashboard URL becomes useless.
Using port 0 solves #681, but the dashboard URL printed at runtime (line 471) will show http://127.0.0.1:0 since config.server.diagnostics is used verbatim. Users won't know the actual listening port.
For --blackhole (testing/benchmarking), this may be acceptable, but consider either:
- Suppressing the dashboard URL when using ephemeral ports, or
- Retrieving the actual bound address from
tiny_http::Server::server_addr()and propagating it
The current behavior is confusing but non-blocking for the blackhole's primary function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/src/main.rs` around lines 230 - 232, The runtime prints a
dashboard URL using config.server.diagnostics verbatim which becomes useless
when diagnostics was set to ephemeral port 0 in the YAML; update the startup
path that handles the blackhole/diagnostics flow (where yaml is built and the
dashboard URL is printed) to either suppress printing the URL when
config.server.diagnostics has port 0 (i.e. detect ":0") or, preferable, after
creating the tiny_http server, call tiny_http::Server::server_addr() to get the
actual bound address and propagate that address into the value used for the
printed dashboard URL so users see the real host:port; change code around the
yaml/config usage and the dashboard print logic to use the resolved address when
available.
Summary
Seven bugs fixed in one conflict-free PR:
--generate-json 0printedavg NaN bytes/line— zero-line case now prints a sensible message--blackhole notanaddrexited 0 — address now validated withparse::<SocketAddr>(), exits 1 on bad input--blackholefailed when port 9090 was already in use — diagnostics port now uses0(OS picks a free port)/v1/logsFOO,/v1/logs/extraetc. with 200 — only exact/v1/logspath is now acceptedApplication/JSONnow treated as JSON per RFC 7231Allow: GET/metricsreturned generic 404 with no explanation — now returns 410 Gone pointing to/api/pipelinesTest plan
cargo clippy -- -D warningscleancargo check --all-targetsclean🤖 Generated with Claude Code