plugins: New Azure Kusto Output Plugin #5645
Conversation
papigers
requested review from
fujimotos,
niedbalski,
edsiper,
leonardo-albertovich and
koleini
as code owners
papigers
force-pushed
the
5591_azure_kusto_output_plugin
branch
from
45c9e95 to
f15e783
Compare
|
Sample configuration (https://pastebin.com/B6wT4aPP) Debug output (https://pastebin.com/SWm8tFnL) Valgrind output (https://pastebin.com/gtWJbRzv) |
patrick-stephens
changed the title
|
@papigers it's great to have the new plugin but probably need some help on integration testing as well over at fluent/fluent-bit-ci. Can you link a PR from there with the integration tests as well? Ideally we can test in isolation and not requiring more infrastructure set up but feel free to ping me or @niedbalski to discuss. |
Hopefully will find a time for it today/tomorrow.
Unfortunately, kusto doesn't have an emulator to run against locally, but we could probably utilize their free tier offering. We would also need to create an azure account (no subscription/payment needed AFAIK), and then create an oauth application in that directory, and use its credentials in the plugin's configuration. |
Would it be possible to pull together a test then, assuming we have access, so we can provide it? We can sort out the access separately then and just have your test case hook into it. |
|
Sure thing, I'll update as soon as I have something ready. |
|
Hey @patrick-stephens, sorry for the delay, I've created this CI PR: fluent/fluent-bit-ci#66 Please tell me what you think. |
|
Super awesome is what I think @papigers , really appreciate it! Once we land the CI PR we can run it with this change. We may have to look at how we handle running it for PRs without your plugin though. Don't want failures for previous PRs. |
|
@papigers this is awesome, want to present this at our community meeting this Thursday? https://docs.google.com/document/d/1vJvsn8E0SanLO1R0X3RC1qTw0XQK_7q75sZ8IbWAu-g/edit# |
|
@agup006 wow, thanks a lot for the offer. |
I think that's fine for the one after if we can schedule you in - the dates should be on the meet up page and I'll add you to the agenda. |
| /* Clear any previous oauth2 payload content */ | ||
| flb_oauth2_payload_clear(ctx->o); | ||
|
|
||
| ret = flb_oauth2_payload_append(ctx->o, "grant_type", 10, "client_credentials", 18); |
There was a problem hiding this comment.
Could you please normalize these calls so all of them pass -1 or the precomputed length as the length of the constant strings?
There was a problem hiding this comment.
normalized all constant string but kept variables with -1, should I pass sdslens there?
There was a problem hiding this comment.
That's ok since flb_oauth2_payload_append can do it at no additional cost.
| token = get_azure_kusto_token(ctx); | ||
| if (!token) { | ||
| flb_plg_error(ctx->ins, "cannot retrieve oauth2 token"); | ||
| goto execute_error; |
There was a problem hiding this comment.
At this point neither c nor body have been initialized so this jump causes a check that could return a false positive and call the destructors for the http client or sds depending on the contents of the stack before this function is called.
In general I think if not strictly necessary goto should not be used but if that's not an option then please initialize those variables to NULL at the beginning of the function.
There was a problem hiding this comment.
Done, removed all gotos except those in azure_kusto_load_ingestion_resources, I think goto is helpful with handling mutex unlock on errors
| flb_output_upstream_set(ctx->u, ins); | ||
|
|
||
| /* Get or renew the OAuth2 token */ | ||
| token = get_azure_kusto_token(ctx); |
There was a problem hiding this comment.
Considering that this callback is invoked when the input plugin is initialized (before the event loop is active) then any http requests made by the code that obtains token will be synchronous (as well as the ones made by azure_kusto_load_ingestion_resources).
Do you think this could be delayed until first use? That would shift the delay and minimize the impact, considering that the code does handle token expiration I think this should not be a problem right?
There was a problem hiding this comment.
I agree it's not necessary, I just mimicked the behavior of other plugins (example) that checked "dependencies" are valid before continuing. But considering that failures here will only emit warnings, I don't see harm in removing them.
@leonardo-albertovich Discovered by running valgrind. In flb_upstream_destroy, the tls struct is referenced, so if it's destroyed before calling upstream_destory we get this warning. Edit: just noticed my commit message suggests the tls is being freed twice, which is not true, I'll update the message as soon as I push fixes for the rest of your review comments |
| msgpack_sbuffer_destroy(&mp_sbuf); | ||
| return -1; | ||
| } | ||
| msgpack_pack_array(&mp_pck, records); |
There was a problem hiding this comment.
Since you are writing the array header with the total number of records here wouldn't skipping elements create a problem when flb_msgpack_raw_to_json_sds unpacks the newly created msgpack buffer? I had that happen in a different setting, I'm not sure of what the right way to address it would be other than inserting placeholder entries at the end or saving the buffer index to go back to edit it after the loop which is not very nice.
I'm not aware of a proper mechanism to address this issue but thought it'd be nice to mention it so others can chime in.
There was a problem hiding this comment.
Waiting till we decide what's the best approach here. I'll just note that I saw similar "skips" in other output plugins too, so we might want to create a task for it and also fix it there.
There was a problem hiding this comment.
Yeah, I'll bring it up internally to see if there's anything we can do to fix that at the root.
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
Signed-off-by: Gershon Papiashvili <[email protected]>
papigers
force-pushed
the
5591_azure_kusto_output_plugin
branch
from
313db94 to
9ed4f0c
Compare
|
@leonardo-albertovich conflict was because the flb_hash interface was renamed to flb_hash_table, so I also added another commit to rename references to that in the plugin code. |
|
Great, I'll review the PR again either today or tomorrow so we can wrap it up, thanks for the time and effort you're putting on this! |
|
@leonardo-albertovich thanks, looks like I also need a review from @fujimotos or @niedbalski ? |
|
Yeah, I don't really know what's going on with that, maybe @patrick-stephens can help us, I had the same issue yesterday with a PR of my own. |
|
Nothing has been changed by us as far as I'm aware... I suspect a Github update somewhere. |
|
Ah, I bet it's down to having changes across files you are not a code owner for @leonardo-albertovich so those have to be approved. |
|
We can't build on current master since we removed mbedtls by #5874 If I removed below include , I can build . Error: |
|
I fixed that issue a few hours ago, it was just a lagging include statement, he had already updated the code to use the flb_ version of the base64 encoder / decoder functions (which are a copy of an older version of the mbedtls functions (before they introduced a performance regression)) |
|
@leonardo-albertovich Thank you. |
|
@leonardo-albertovich thanks, sorry for missing that. |
* out_azure_kusto: create azure kusto output plugin Signed-off-by: Gershon Papiashvili <[email protected]> * flb_upstream_node: fix tls free before upstream destroy Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: fix build Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: normalize flb_oauth2_payload_append constant lengths Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: check token sds create succeed Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: remove blocking checks on plugin init Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: debug on msgpack parsing issues Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: avoid goto when possible Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: fix uuid malloc Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: better msgpack inits and cleanups Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: move casting to after declarations Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: use flb_base64_encode Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: remove goto in flb_upstream_node_create_url Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: change defaults Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: use snprintf Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: avoid reprtitive free Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: fix ingestion message snprintf Signed-off-by: Gershon Papiashvili <[email protected]> * out_azure_kusto: use new name for hash table interface Signed-off-by: Gershon Papiashvili <[email protected]> Signed-off-by: Gershon Papiashvili <[email protected]> Signed-off-by: Manal Geries <[email protected]>
Adds an output plugin for Azure Data Explorer (aka Kusto). #5591
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.
Documentation
Will create documentation PR in the upcoming days
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.