out_prometheus_exporter: apply HTTP server options - #12111
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe Prometheus exporter now forwards configured HTTP buffer and worker options to its HTTP server. A runtime test selects a free port, starts the exporter with platform-specific settings, validates the server configuration, and registers the test with CMake. ChangesPrometheus HTTP options
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes two explicitly configured HTTP server options effective without altering defaults, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c43d302a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4c43d30 to
23f280e
Compare
cosmo0920
left a comment
There was a problem hiding this comment.
Looks reasonable for me but I found a nitpick on this PR.
| if(NOT FLB_SYSTEM_WINDOWS) | ||
| FLB_RT_TEST(FLB_OUT_PROMETHEUS_EXPORTER "out_prometheus_exporter.c") | ||
| endif() |
There was a problem hiding this comment.
You are right, thanks for the pointers to #12092 and #12098. I have dropped the Windows skip and the runtime test now builds and runs on Windows too (commit a2506bb).
Changes:
tests/runtime/CMakeLists.txt: registersflb-rt-out_prometheus_exporterunconditionally again.get_free_port(): callsWSAStartup()under#ifdef _WIN32beforesocket(), matching the idiom inin_http.candcore-timeout.c.- The multi-worker assertion is kept portable: shared listener ports (
SO_REUSEPORT) are rejected byflb_net_socket_share_port()on Windows, so the test useshttp_server.workers=2on POSIX and1on Windows. Thebuffer_chunk_sizepass-through (the actual regression from out_prometheus_exporter ignores HTTP server worker and buffer options #12110) is verified on both platforms; theruntime != NULLcheck is guarded to the multi-worker path only, since a single worker runs on the caller event loop.
Let me know if you would prefer a different split.
23f280e to
a2506bb
Compare
|
Follow-up after the latest push ( The original Linux/UBSAN failure this branch was addressing is now fixed. In particular, the previously relevant The remaining red jobs do not appear to be caused by this change:
Those failures are outside the |
Propagate buffer_chunk_size and workers from the parsed HTTP server configuration when creating the exporter HTTP server. Signed-off-by: Stefano Tondo <stondo@gmail.com>
Verify that the configured buffer chunk size and worker count reach the embedded HTTP server and create a worker runtime. Signed-off-by: Stefano Tondo <stondo@gmail.com>
53a16a1 to
15d84f8
Compare
|
Rebased onto current Dropped one commit as redundant. On the two failing jobs from the previous run, both are unrelated to this change:
Local verification on the rebased head: @cosmo0920 the Windows runtime-test point you raised is addressed as of |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
Propagate
buffer_chunk_sizeandworkersfrom the parsed HTTP server configurationwhen
out_prometheus_exportercreates its embedded HTTP server.The plugin already applies
idle_timeout,buffer_max_size, andmax_connections.Omitting the other two parsed options makes valid configuration ineffective,
particularly
http_server.workers.Fixes #12110
Changes
http_server.buffer_chunk_sizehttp_server.workersThe test uses the actual Fluent Bit engine and verifies that:
It fails before the fix and passes afterward.
Compatibility
The default behavior is unchanged. The patch only makes explicitly configured HTTP
server options effective.
The same partial option propagation appears in
out_vivo_exporter; this PR remainsscoped to the reproduced Prometheus exporter defect.
Testing
Example configuration:
Commands:
These ran in the documented Debian 12 rootless Podman environment. The explicit AVX2
flag is a GCC 12 compatibility workaround for bundled
simdutf, not part of thischange.
Results:
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Packaging
Documentation
Backporting
5.0backport after the master change is acceptedFluent Bit is licensed under Apache 2.0. By submitting this pull request I understand
that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests