plugin_proxy: enable event_type specification for proxy plugins#11011
plugin_proxy: enable event_type specification for proxy plugins#11011jmccormick7 wants to merge 5 commits intofluent:masterfrom
Conversation
jmccormick7
requested review from
edsiper,
fujimotos and
koleini
as code owners
📝 WalkthroughWalkthroughAdded an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
If we proceed to address this issue, we need to send another PR into here https://github.com/chronosphereio/calyptia-plugin to handle event_type as well. |
@cosmo0920 I can open a PR in that repository tonight! |
|
@cosmo0920 How would you prefer I do testing beyond the unit tests? I've done some manual testing that I can document, but is there any formalized regression testing or tests for proxy plugins? |
|
How about adding runtime shell test cases with precompiled Golang shared objects here? https://github.com/fluent/fluent-bit/tree/master/tests/runtime_shell |
|
@cosmo0920 I have added a runtime test that build and test a go plugin shared object. Please let me know if there are any improvements or other additions I can provide to speed up the timeline! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/runtime_shell/go_plugins/build_test_plugins.sh (1)
98-98: Consider using FLB_BUILD variable instead of hardcoded path.The CMakeLists.txt now exports FLB_BUILD to the environment. Consider using
$FLB_BUILDinstead of constructing$BUILD_DIRto ensure consistency with the test environment.Apply this diff:
- CGO_ENABLED=1 GO111MODULE=on go build -buildmode=c-shared -v -ldflags="-s -w" -o $BUILD_DIR/test_logs_go.so logs_output.go + CGO_ENABLED=1 GO111MODULE=on go build -buildmode=c-shared -v -ldflags="-s -w" -o ${FLB_BUILD:-$BUILD_DIR}/test_logs_go.so logs_output.gotests/runtime_shell/proxy_logs_expect.sh (2)
19-19: Use FLB_BUILD variable for consistent path construction.The script hardcodes
$FLB_ROOT/build/test_logs_go.so, but the CMakeLists.txt now providesFLB_BUILDin the environment for this purpose.Apply this diff:
- $FLB_BIN -e $FLB_ROOT/build/test_logs_go.so -c $FLB_RUNTIME_SHELL_CONF/proxy_logs_test.conf > "$STDOUT_OUTPUT_FILE" 2>&1 & + $FLB_BIN -e $FLB_BUILD/test_logs_go.so -c $FLB_RUNTIME_SHELL_CONF/proxy_logs_test.conf > "$STDOUT_OUTPUT_FILE" 2>&1 &
22-22: Replace hardcoded sleep with signal file polling.The hardcoded
sleep 3is fragile and can cause flakiness in tests. Consider polling for the SIGNAL_FILE_PATH or checking the process output instead of using a fixed delay.tests/runtime_shell/go_plugins/test_logs_go.h (1)
1-92: Consider excluding auto-generated CGO headers from version control.This file is marked as auto-generated by cmd/cgo and should not be manually edited. Consider adding
*.hor specificallytest_logs_go.hto .gitignore in this directory, and generate it during the build process instead.tests/runtime_shell/go_plugins/logs_output.go (1)
28-28: Consider using the timestamp from GetRecord.The timestamp returned by
GetRecord(second return value) is currently ignored. While this is acceptable for a test plugin, consider whether timestamp information would be useful for test verification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/runtime_shell/go_plugins/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
tests/runtime_shell/CMakeLists.txt(2 hunks)tests/runtime_shell/conf/proxy_logs_test.conf(1 hunks)tests/runtime_shell/go_plugins/build_test_plugins.sh(1 hunks)tests/runtime_shell/go_plugins/go.mod(1 hunks)tests/runtime_shell/go_plugins/logs_output.go(1 hunks)tests/runtime_shell/go_plugins/test_logs_go.h(1 hunks)tests/runtime_shell/proxy_logs_expect.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime_shell/CMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
tests/runtime_shell/CMakeLists.txt
📚 Learning: 2025-09-04T07:28:37.083Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10832
File: src/wasm/CMakeLists.txt:112-131
Timestamp: 2025-09-04T07:28:37.083Z
Learning: In fluent-bit CMake files, the user cosmo0920 prefers treating Git as a command rather than a package, emphasizing that Git is not a pkg-config retrievable package but just a command.
Applied to files:
tests/runtime_shell/CMakeLists.txt
🧬 Code graph analysis (1)
tests/runtime_shell/go_plugins/test_logs_go.h (1)
tests/runtime_shell/go_plugins/logs_output.go (4)
FLBPluginRegister(12-15)FLBPluginInit(18-20)FLBPluginFlushCtx(23-39)FLBPluginExit(42-44)
🔇 Additional comments (7)
tests/runtime_shell/CMakeLists.txt (2)
18-18: LGTM!The addition of the proxy logs test to the test suite is appropriate.
31-32: LGTM!Adding the FLB_BUILD environment variable enables tests to reference the build directory, which is useful for locating built artifacts like the test plugin shared object.
tests/runtime_shell/conf/proxy_logs_test.conf (1)
1-21: LGTM!The test configuration is well-structured for validating the proxy logs functionality. The use of environment variables for the signal file path enables flexible test execution.
tests/runtime_shell/go_plugins/logs_output.go (2)
11-15: LGTM!The registration function correctly delegates to the fluent-bit-go output package with appropriate plugin name and description.
22-39: LGTM!The flush implementation correctly decodes records and outputs them to stdout for test verification. The error handling is appropriate for a test plugin.
tests/runtime_shell/go_plugins/go.mod (2)
3-3: The Go version 1.25.1 is valid and exists.The latest stable Go release is Go 1.25.3 (released October 13, 2025), and Go 1.25.1 was released prior to that. The code's specification of
go 1.25.1is legitimate, though not the latest version available. No changes are required.Likely an incorrect or invalid review comment.
6-6: Review comment is incorrect — PR 80 is open, not merged.PR 80 ("Enable metric output plugins") in fluent-bit-go is currently open as of October 10, 2025 and has not been merged. Additionally, the plugin code (
logs_output.go) does not use or requireevent_typesupport. The current dependency version is appropriate for the code's functionality.Likely an incorrect or invalid review comment.
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
Looks good. With the newly added unit test case for confirming backward compatibility, this PR ensures backward compatibility.
jmccormick7
force-pushed
the
enable-metric-proxy-plugins
branch
from
e08bf8e to
2dd986c
Compare
This commit adds event_type to the flb_plugin_proxy_def struct. This allows for the setting of event_type to a value other than the default log only initialization. In the flb_plugin_proxy_register function default behaviour is enforced by checking for unset event_type vlaues. When unset (a value of 0) then log behaviour is defaulted. No need for incorrect value checking since this was not a set field in the past. Unset fields are set as current behavior dicates and future usecases using incorrect values can be treated as user error. Input use case does not use event-type and is unaffected by this change. Signed-off-by: jmccormick7 <jcm258@case.edu>
… malloc Changing intialization to use calloc instead of malloc so that emtpy fields are 0 initialized. Specifically for the event_type addition. Signed-off-by: jmccormick7 <jcm258@case.edu>
This commit adds a runtime shell test for the go plugin interface. A dummy go output plugin is added to the directories, and a test that builds the plugin shared object and then tests that it works and writes the output. This test ensures that the go proxy interface is working as expected. This was tested in the devcontainer using ctest. Signed-off-by: jmccormick7 <jcm258@case.edu>
Signed-off-by: jmccormick7 <jcm258@case.edu>
This commit makes the go proxy a test that only runs on Linux using the CMAKE_SYSTEM_NAME. It also removes an autogenerated header file from the go compile that should not have been commited. Signed-off-by: jmccormick7 <jcm258@case.edu>
jmccormick7
force-pushed
the
enable-metric-proxy-plugins
branch
from
2dd986c to
b6eb627
Compare
2 participants
This commit adds event_type to the flb_plugin_proxy_def struct. This allows for the setting of event_type to a value other than the default log only initialization.
In the flb_plugin_proxy_register function default behavior is enforced by checking for unset event_type values. When unset (a value of 0) then log behavior is defaulted. No need for incorrect value checking since this was not a set field in the past. Unset fields are set as current behavior dictates and future use cases using incorrect values can be treated as user error.
Input use case does not use event-type and is unaffected by this change.
Addresses #10995
This corresponding PR in the fluent-bit-go library enables the ability to allow Go plugins to register their event types PR 80
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent 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
Chores