-
Notifications
You must be signed in to change notification settings - Fork 1.8k
router: extend router interface to support metrics (v3) #11108
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
base: master
Are you sure you want to change the base?
Changes from all commits
c76f9ad
d4ad577
e9d4ae3
4094f8d
a56488e
953d22b
046e0dc
8b50dd3
a1d1bbb
7eba8fc
aad9038
e934e60
9e176f4
a31157e
781aedf
5c65379
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -304,7 +304,36 @@ struct flb_config *flb_config_init() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Routing */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_routes_mask_set_size(1, config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config->router = flb_router_create(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!config->router) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_error("[config] could not create router"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config->kernel) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_kernel_destroy(config->kernel); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #ifdef FLB_HAVE_HTTP_SERVER | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config->http_listen) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_free(config->http_listen); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config->http_port) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_free(config->http_port); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_cf_destroy(cf); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_free(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return NULL; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ret = flb_routes_mask_set_size(1, config->router); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ret != 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_error("[config] routing mask dimensioning failed"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_router_destroy(config->router); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config->kernel) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_kernel_destroy(config->kernel); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_cf_destroy(cf); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_free(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return NULL; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+326
to
+336
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previous review concern addressed, but memory leak remains. The return value of Apply this diff to fix the leak: ret = flb_routes_mask_set_size(1, config->router);
if (ret != 0) {
flb_error("[config] routing mask dimensioning failed");
flb_router_destroy(config->router);
+#ifdef FLB_HAVE_HTTP_SERVER
+ if (config->http_listen) {
+ flb_free(config->http_listen);
+ }
+ if (config->http_port) {
+ flb_free(config->http_port);
+ }
+#endif
if (config->kernel) {
flb_kernel_destroy(config->kernel);
}
flb_cf_destroy(cf);
flb_free(config);
return NULL;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config->cio = NULL; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config->storage_path = NULL; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -613,7 +642,8 @@ void flb_config_exit(struct flb_config *config) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* release task map */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_config_task_map_resize(config, 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_routes_empty_mask_destroy(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_router_destroy(config->router); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Clean up router input routes */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flb_router_routes_destroy(&config->input_routes); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Over-allocation: using slots (bits) instead of size (elements) for mask operations.
flb_routes_mask_get_slots() returns bit-count; you’re allocating/clearing 64× more than needed. Use flb_routes_mask_get_size() for element count.
Apply this fix:
📝 Committable suggestion
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, it could be an optimized way. But I have a question. If we use this way to allocate heap memories, it'll need to reallocate to fix the size of heap memories for route masks? This is because it could be using a possibly bigger size of allocation here, but the re-allocate costs could be high or any other re-allocating places are implemented to extend/shrink the size of heap memories that are using for route masks?
If there's no code path to re-allocate/adjust places for them, it could be worthless suggestion.
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 84
🏁 Script executed:
Length of output: 444
🏁 Script executed:
Length of output: 316
🏁 Script executed:
Length of output: 2028
🏁 Script executed:
Length of output: 856
@cosmo0920, great question! Let me clarify - there's no reallocation happening here, and this is actually a bug fix, not just an optimization.
The Issue
The semantic difference between the two functions:
route_mask_slots= number of bits (i.e., max number of outputs/routes)route_mask_size= number of elements (uint64_t) =ceil(slots / 64)Since each
flb_route_mask_element(uint64_t) holds 64 bits, if you have 3 outputs, you need:route_mask_slots= 3 (bits)route_mask_size= 1 (element, since 3 bits fit in one uint64_t)Current Bug
The storage_backlog code currently:
slotselements (3 elements = 192 bytes) when it only needssizeelements (1 element = 8 bytes)slotselements in the loop when it only needs to clearsizeelementsThis is a ~64× over-allocation and over-clearing, not a design choice.
No Reallocation Occurs
The lifecycle is simple:
cb_sb_init(line 671)sb_append_chunk_to_segregated_backlogs(line 289) — called in a loopcb_sb_exitThe mask size is determined at router initialization and never changes. There's no code path that reallocates or adjusts this buffer.
Consistency with Rest of Codebase
Every other allocation/clear operation in the refactored code uses
get_size():src/flb_input_chunk.cline 964 & 2024: allocations useget_size()src/flb_input_log.cline 182: memset usesget_size()src/flb_routes_mask.cline 67: memset usesget_size()The storage_backlog plugin is the only place still using
get_slots()incorrectly.Recommendation
Apply the fix to match the rest of the codebase. It will:
🧬 Code Graph Analysis Results
[src/flb_routes_mask.c] lines 36-43
[src/flb_routes_mask.c] lines 49-88
[src/flb_routes_mask.c] lines 154-173
[src/include/fluent-bit/flb_mem.h] lines 84-96
[src/include/fluent-bit/flb_mem.h] lines 126-128
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.
@edsiper This @coderabbitai's point of view could be correct for this patch. So, could you take a look on it?
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.
@cosmo0920, thank you for the review! I appreciate you confirming the analysis.
@edsiper, the issue is that both the allocation (line 671) and memset (line 289) in the storage_backlog plugin are using
flb_routes_mask_get_slots()where they should useflb_routes_mask_get_size(). This causes a 64× over-allocation sinceslotscounts bits whilesizecounts uint64_t elements.The fix is straightforward and makes the code consistent with the rest of the router refactoring. Happy to clarify anything if needed!