-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fluent-bit: multi-instance support #1294
Conversation
Probably to be discussed, I was not successful at getting I see #1283 is working on thread safety for promtail, which uses pretty much the same code. We're running this code (together with the tenantID change from my other PR) in integration already -- still having a bunch of out-of-order issues (which we also had with a single output), no further issues so far. |
We have #1254 to fix the out of order problem. Not sure if this is possible to add some tests for this multi instance features ? |
35790f9
to
8e5abdd
Compare
To run multiple plugin instances at the same time, the plugin instance must be registered and later retrieved. Additionally, a list of registered plugin instances must be stored for proper disposal during fluent-bit shutdown. Based on the [out_multiinstance example] in the upstream repository. [out_multiinstance example]: https://github.com/fluent/fluent-bit-go/blob/fc386d263885e50387dd0081a77adf4072e8e4b6/examples/out_multiinstance/out.go Signed-off-by: Jens Erat <email@jenserat.de>
8e5abdd
to
ebbe083
Compare
This is an updated version, merging ideas of @cosmo0920 in #1454 into my work. The fluent-bit side configuration of an ID is indeed not required, but the code can be simplified even further by passing the plugin reference as pointer, not the ID. I was able to remove a whole bunch of lines. I had a shot at adding some kind of end-to-end test, but this is not a trivial task. cgo and golang tests do not really go together well, I have to provide some mocks and function wrappers (in a totally reasonable amount, but still work to do). At least for the receiver side there is reusable code in promtail. I will not be able to finish this within the next days though, out of office again for more than a week now. I totally agree we should have tests, but I'd love to see this split apart to another follow-up PR since this blocks at least us and @fmax in #1446 from using upstream releases. I will have another go on the test code as soon as time allows, but for now I just have some vast ideas on how to test the plugin. Unlike cosmo's PR, this also keeps the ID reference for logs. This makes understanding what's going wrong much easier. For example (see the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'll co author our friend @cosmo0920
What this PR does / why we need it:
To run multiple plugin instances at the same time, the global plugin
variable needs to be replaced by a map and lookup of the correct plugin
by reading an ID from the function call's context.
Based on the out_multiinstance example in the upstream repository.
Signed-off-by: Jens Erat email@jenserat.de
Which issue(s) this PR fixes:
Special notes for your reviewer:
Checklist