out_parseable: Parseable plugin for fluentbit#9872
out_parseable: Parseable plugin for fluentbit#9872AdheipSingh wants to merge 1 commit intofluent:masterfrom
Conversation
AdheipSingh
requested review from
celalettin1286,
edsiper,
fujimotos,
koleini,
leonardo-albertovich,
niedbalski and
patrick-stephens
as code owners
|
Hi @niedbalski @patrick-stephens @celalettin1286 @edsiper @leonardo-albertovich @fujimotos @koleini, Apologies for tagging you all together, I truly appreciate the time and effort you put into maintaining the project. I wanted to check in on the status of this PR that has been open for a while. Please let me know if any additional information is needed from our side to facilitate the review process or if it’s already in the review queue and just needs more time. Thanks again for all the great work you do! |
leonardo-albertovich
left a comment
leonardo-albertovich
left a comment
There was a problem hiding this comment.
I did not find any issues other than the coding style violations I've pointed.
Additionally I'd appreciate it if you revisited the variable names to ensure that they err on the side of verbosity since that makes it easier for people to understand the code and avoid accidents.
In terms of cleanup, I think that since cb_parseable_flush is comprised of a few nested scopes (with most of the "time" being spent in the while loop that spans lines 125 to 283) it would be great if instead of having the same (or similar) cleanup block at every step you refactored it to signal the failure through a variable, ensured that the pointers were all pre initialized to NULL values at the beginning of the loop and aborted the loop when an error is found.
That way the code would be much shorter (even if each "free" in the cleanup part is wrapped by its own conditional) and easier to follow and validate.
plugins/out_parseable/parseable.c
Outdated
| FLB_IO_TCP | FLB_IO_ASYNC, | ||
| NULL); | ||
|
|
||
| ctx->upstream->base.net.connect_timeout = ctx->connect_timeout; |
There was a problem hiding this comment.
Is there a specific reason why you are creating your own connection timeout setting instead of relying on the standard net.connect_timeout? That would be the preferred way of doing it.
plugins/out_parseable/parseable.c
Outdated
| NULL); | ||
|
|
||
| ctx->upstream->base.net.connect_timeout = ctx->connect_timeout; | ||
| ctx->upstream->base.net.accept_timeout = ctx->accept_timeout; |
There was a problem hiding this comment.
Your plugin does not accept any connections (it does not expose any endpoints and it doesn't use the flb_downstream component) so this parameter is not required and should be removed.
plugins/out_parseable/parseable.c
Outdated
| flb_plg_info(ctx->ins, "Timeouts - Connect: %ds, Accept: %ds", ctx->connect_timeout, ctx->accept_timeout); | ||
|
|
||
| /* Set retry limit */ | ||
| char retry_limit_str[16]; |
There was a problem hiding this comment.
Please move this (and any other) variable definitions to the beginning of the functions.
plugins/out_parseable/parseable.c
Outdated
|
|
||
| /* Set retry limit */ | ||
| char retry_limit_str[16]; | ||
| snprintf(retry_limit_str, sizeof(retry_limit_str), "%d", ctx->retry_limit); |
There was a problem hiding this comment.
You do not need to manually set the retry limit so please remove this code.
|
|
||
| flb_plg_info(ctx->ins, "Retry limit set to: %d", ctx->retry_limit); | ||
|
|
||
| if (!ctx->upstream) { |
There was a problem hiding this comment.
You need to move this conditional to be right after the call to flb_upstream_create.
| flb_http_basic_auth(client, ctx->username, ctx->password); | ||
|
|
||
| /* Perform request */ | ||
| ret = flb_http_do(client, &b_sent); |
There was a problem hiding this comment.
Check the value returned by flb_http_do to ensure that the response context is valid.
|
|
||
| /* Final cleanup */ | ||
| if (ra) { | ||
| flb_ra_destroy(ra); |
There was a problem hiding this comment.
Please modify this to explicitly compare against NULL.
plugins/out_parseable/parseable.c
Outdated
| if (ra) { | ||
| flb_ra_destroy(ra); | ||
| } | ||
| if (ns_ra) { |
There was a problem hiding this comment.
Please modify this to explicitly compare against NULL.
| msgpack_pack_str_with_body(&pk, "fluent bit parseable plugin", 25); | ||
|
|
||
| /* Convert to JSON */ | ||
| body = flb_msgpack_raw_to_json_sds(sbuf.data, sbuf.size); |
There was a problem hiding this comment.
Move all of those msgpack_sbuffer_destroy(&sbuf); calls to the line between body = flb_msgpack_raw_to_json_sds(sbuf.data, sbuf.size); and if (!body) { so you release the resource as soon as possible and don't have to worry about it later.
| return 0; | ||
| } | ||
|
|
||
| if (ctx->exclude_namespaces) { |
There was a problem hiding this comment.
I think (and unless every plugin I've checked is leaking) that you do not need to destroy ctx->exclude_namespaces as it's handled by the system just like any other config map based option.
|
Overall this is a very small plugin and it would be great if we could ensure that it's as simple as possible. Once those issues are addressed get back to me and I'll review the PR again. |
|
Thank you for the review @leonardo-albertovich this is very useful. We'll take a look at the comments and get back soon. |
patrick-stephens
left a comment
patrick-stephens
left a comment
There was a problem hiding this comment.
On the packaging side, we should be explicit on what targets this should build for? Should it build you all including Windows and macOS? It looks like we're only enabling it for the container image but ideally all Linux targets should have the same plugins.
Unless I missed something I didn't see any features that should limit the target set, the only one (which is obvious but might be worth explicitly stating in the build file) is the requirement of having the record_accessor enabled. I think that might just be a mistake, was it @nitisht? |
Signed-off-by: AdheipSingh <adheip1222@gmail.com>
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
5 participants
Send kubernetes logs to https://www.parseable.com/
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.